Problem/Motivation

#2533498: Update jQuery to version 3 is a Critical task to update jQuery to version 3, ideally before 8.4-alpha.

In jQuery 3, $('#') throws a syntax error. This is responsible for the test failure in #2533498-79: Update jQuery to version 3.

The failure is triggered by the code and test added in #2531700: Fragment links to children elements in closed grouping elements don't work. Specifically, this line in form.js:

$target = $('#' + location.hash.substr(1));

When the location hash can be simply '#', which appears to be set to that in FormGroupingElementsTest::testDetailsChildVisibility()'s:

$reset_js = "location.replace('#'); jQuery('details').removeAttr('open')";

Proposed resolution

Not sure if the test is wrong for setting that location or if form.js is wrong for not checking for that before invoking $().

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#19 interdiff-16-19.txt643 byteseffulgentsia
#19 2898261-19.patch3.78 KBeffulgentsia
#16 interdiff-7-16.txt853 byteseffulgentsia
#16 2898261-16.patch3.78 KBeffulgentsia
#7 core-js-form-error-2898261-7.patch3.91 KBnod_
#5 simplify-hashchange-event.patch2.65 KBdroplet
#3 form.patch2.03 KBdroplet
#3 test.patch588.92 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Parent issue: » #2533498: Update jQuery to version 3
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Active » Needs review
FileSize
test.patch588.92 KB
form.patch2.03 KB
dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt as a volunteer and at Synetic commented

I agree catching this use case in form.js.
Often enough I see an empty fragment in URLs.

Patch looks good, just one question, if we add these conditions shouldn't we also do the same for:
$target = e.currentTarget.location ? $(e.currentTarget.location.hash) : $(e.currentTarget.hash);?
I'm not sure it can potentially be #. Could someone check?

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
simplify-hashchange-event.patch2.65 KB

It should look like this if you ask me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Combined this with #2533498: Update jQuery to version 3 in comment 83 to see if it fixes fails with jQuery 3.x.
Added @droplet to issue credits over there.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
FileSize
core-js-form-error-2898261-7.patch3.91 KB

Put back the support for click on links when the hash is already in the URL. Used trigger instead of the .focus() alias function. Removed the extra parameter to the settimeout, it's not used and not crossbrowser.

droplet’s picture

droplet CreditAttribution: droplet commented

Hmm.. So #5 contains a bug. It takes the old hash and the test case didn't catch it.

Just my own curiosity, when will `e.currentTarget.location` has value?

** Another bug, we should verify the href is current URL. (can be follow-ups)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

Not sure, didn't want to track it down so left it there. Possibly we could get rid of it.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

For what it's worth, I like #7 as the proper scope for this issue. I think #8 and #9 can be left to a follow-up issue. Anything else needed for this to be RTBC'd?

cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented 
+++ b/core/misc/form.es6.js
@@ -264,29 +264,31 @@
+      url = location;

Would location have a value here?

cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented

Duh - ignore.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented 
+++ b/core/misc/form.es6.js
@@ -264,29 +264,31 @@
+  function handleFragmentLinkClickOrHashChange(e) {

Any reason we're changing this? Feels out of scope for this patch.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Re #13, I can see the argument for it being out of scope for this issue, but it also looks like a no-op cleanup to me...

Within ES6:

+++ b/core/misc/form.es6.js
@@ -264,29 +264,31 @@
-  const handleFragmentLinkClickOrHashChange = (e) => {
+  function handleFragmentLinkClickOrHashChange(e) {

Is there any difference between a const that's a function and a function?

Within the transpiled JS:

+++ b/core/misc/form.js
@@ -125,23 +125,27 @@
-  var handleFragmentLinkClickOrHashChange = function handleFragmentLinkClickOrHashChange(e) {
+  function handleFragmentLinkClickOrHashChange(e) {

This looks like a pure coding style cleanup to me, or is there any actual functionality to assigning a function to a variable of the same name?

So long as it's a no-op or has a justification, I'm fine with it staying in this patch. I'd also be fine with that bit getting moved to a separate issue. Either way, I'd love to still commit both this and #2533498: Update jQuery to version 3 before the alpha freeze (in 10 hours) if they can still make it to RTBC by then.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Needs work

@effulgentsia

We already had this discussion in #2531700: Fragment links to children elements in closed grouping elements don't work.102. I'd personally prefer we not attempt to make this change here and if we really want to have a discussion, let's move it to another issue.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Needs work » Needs review
FileSize
2898261-16.patch3.78 KB
interdiff-7-16.txt853 bytes

Re #15, yes, if there's any contention around it, then we should leave it alone and discuss it elsewhere. So this reverts the out of scope line.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

Unticking the credit checkbox for myself, because all I did in #16 was revert a line to how it is in HEAD.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

For getting a test run with #16 in the presence of jQuery3, I posted a combined patch in #2533498-88: Update jQuery to version 3.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
FileSize
2898261-19.patch3.78 KB
interdiff-16-19.txt643 bytes

This also restores HEAD's semicolon at the end of the function assignment. Tests pass with or without the trailing semicolon, but it just makes sense to add the semicolon back in, since that's how it is in HEAD and elsewhere in multiline assignments. Sorry to have missed that when uploading #16.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented

Changed it for consistency, the rest of the file has regular function declaration.

As a general rule I dislike anonymous functions and seriously question the use of arrow function when the function body is multi-line (and needs to be wrapped in {}).

@drpal: I don't see the discussion all i see is #2531700-106: Fragment links to children elements in closed grouping elements don't work.

Anyway, doesn't matter.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented
Status: Needs review » Reviewed & tested by the community

All feedback given on this issue has been addressed, and my only contributions were to revert pieces, not write any original code; therefore, RTBC! I'm about to go to sleep, so leaving it to committers in other timezones to decide if this can still make it in before alpha freeze.

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented
Assigned: Unassigned » andrewmacpherson
Issue tags: +Needs manual testing

Taking this for manual testing. I think it's good idea to double check patches like this, because well-intentioned JS changes have led to a11y regression in the past.

droplet’s picture

droplet CreditAttribution: droplet commented

well-intentioned JS changes have led to a11y regression in the past.

Any examples? I afraid this message will confuse committers in the future JS changes. Most of JS usually the additions to new changes in UI which provides extra functions but not remove existing features.

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt as a volunteer and at Synetic commented

@droplet #23, one example would be keyboard navigation for drag-and-drop ordering situations, that broke in the past.

@nod_ #20, I had a discussion with @drpal about function declarations in that issue and it continued on Slack. It was based on this:
https://github.com/airbnb/javascript#functions--declarations and https://github.com/airbnb/javascript/issues/794

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented
Issue tags: -Needs manual testing

Done manual testing. I went through the same scenarios as we tested in #2531700: Fragment links to children elements in closed grouping elements don't work recently where this fragment link handler was introduced. Still working fine, so still RTBC.

@droplet - I was thinking of #2574917: Regression: vertical tabs are not keyboard accessible. and #2711907: [regression] Some ajax-enabled buttons are not keyboard operable. Both of these regressions happened after issues aimed at modernizing or simplifying existing JS code. We didn't have FunctionalJavascript tests back then (and we've started simulating keyboard interaction in some) so hopefully we'll see fewer regressions like that.

Edit: The issue @dmsmidt mentioned is #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys

andrewmacpherson’s picture

andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented
Assigned: andrewmacpherson » Unassigned
droplet’s picture

droplet CreditAttribution: droplet commented

Thanks @dmsmidt & @andrewmacpherson.

  • lauriii committed cfff720 on 8.4.x 
    Issue #2898261 by droplet, effulgentsia, nod_, andrewmacpherson, cilefen...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Thanks for taking the time to manually test this and confirming that this works. The code changes look good for me. We will also get some test coverage for this once #2533498: Update jQuery to version 3 gets committed. So this is now committed cfff720 and pushed to 8.4.x. Thanks!

cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Status: Fixed » Reviewed & tested by the community

What about 8.5.x?

  • cilefen committed 2174766 on 8.5.x authored by lauriii 
    Issue #2898261 by droplet, effulgentsia, nod_, andrewmacpherson, cilefen...
cilefen’s picture

cilefen CreditAttribution: cilefen at Institute for Advanced Study commented
Status: Reviewed & tested by the community » Fixed

I pushed it to 8.5.x.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.