Problem/Motivation

Now that jQuery 3.0 has been released jQuery 2.x will only be receiving security updates. I know what we've had discussions about updating specific libraries in the past. But there are a few compelling reasons why we should consider updating jQuery to 3.x

Support for JavaScript Promises: This could potentially make a large change in the readability / programming of deferred actions / behaviors.
Better error reporting: Apparently jQuery has add a number of actions that silently failed in the past, now fewer of those silently fail.
Let's make a statement that Drupal doesn't get stuck in the past with JavaScript libraries any more: A key problem with Drupal in the past is that we didn't want to break core by updating jQuery but that led to all kinds of hacks that we had to rely on in cases where we needed to update the library. Let's move forward now that we have semantic versioning.
References:
https://blog.jquery.com/2016/06/09/jquery-3-0-final-released/

Proposed resolution

- A POC with jQuery3 naively applied can be made to actually evaluate the issues with it. (it may take to wait for jQueryUI to be compatible ?)
- Considering to remove deprecated APIs in current 2.x before other libs supports catch up.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Dom.’s picture

Wim Leers’s picture

Issue tags: +JavaScript
nod_’s picture

Title: [policy] Should we embrace Jquery 3 for the future ? » Prepare D8 for jQuery3
Category: Plan » Task

Updating issue metadata to reflect the situation :)

Since we update to latest lib available, and jQuery has a good track record of shipping before us we should get ready. They don't have a 1 year beta cycle :þ

Looking at the changelog I don't think that many things will break (haven't tried it yet though).

Dom.’s picture

Status: Active » Needs review
FileSize
606.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,232 pass(es). View

Actually it does not seems to break much indeed. Too bad we don't have JS-enabled test coverage to know for sure on any feature.

cilefen’s picture

Issue summary: View changes
Dom.’s picture

Thanks for corrections @cilefen ! I'm not a native you have guessed, and I know bad grammar can be a spike in the eye while reading for native readers !

nod_’s picture

Issue tags: -jQuery Version

Probably need a jquery ui update at the same time.

Just got a "Uncaught TypeError: f.getClientRects is not a function" error on some quickedit dialog. It's still functional but the placement is wrong. Looked at jqueryui source, it's fixed in the git repo but no release with said fix. Need to wait for a jquery3 compatible jquery ui to update this.

In our code, I haven't seen anything breaking. Looks like we did a good job dodging bullets.

catch’s picture

Priority: Normal » Major
Issue tags: +rc target
catch’s picture

It'd be great to ship Drupal 8 with jQuery 3. Does someone have a contact at jQuery that could help us coordinate that?

nod_’s picture

I'll ping scott_gonzalez. In the meantime the github milestone shows it's not there yet. https://github.com/jquery/jquery/milestones

webchick’s picture

I'm thinking this is something we could call out in the release notes as "not yet, but targeting before release" maybe. It doesn't seem wise to replace a stable, working jQuery with an alpha jQuery in RC1. OTOH, I would hate to ship 8.0.0 with jQuery 2 if 3 is ready before we are. And even if it's merely close (at least beta, ideally RC), we might want to move 8.0.0 to it, or at least 8.1.0.

Dom.’s picture

I can't find info on jQuery3 roadmap at the moment. But indeed, it would require to be included as soon as possible (ie stable) in Drupal 8.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Postponed
Issue tags: -rc target +minor version target

This isn't possible until there's a new tag for jQuery 3, since it's been months since the last one and there's no information anywhere about it.

Bumping to 8.1.x. If that new tag does arrive during release candidate, we can move it back.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mirom’s picture

Status: Postponed » Needs work
droplet’s picture

Title: Prepare D8 for jQuery3 » Update jQuery to version 3
Issue summary: View changes
Wim Leers’s picture

http://blog.jquery.com/2016/06/09/jquery-3-0-final-released/ lists BC breaks. For example .load() has been removed. devel and panels_ipe use that, for example. It's easy to fix, but it is a break.

OTOH, there are still relatively few D8 contrib modules and sites, so the pain to fix these relatively minor breaks will be fairly small, and will have the benefit that we're not left behind on jQuery 2.x for years to come.

Tough call. Does semver apply to APIs of the 3rd party libraries shipped with software?

effulgentsia’s picture

Does semver apply to APIs of the 3rd party libraries shipped with software?

Technically, I think, yes, it does. If something is a break to the "public API" of a library, then I think it's also a break to the public API of the Drupal core package, if Drupal core's composer.json or bundled JS files are changed to use the later version of the library. https://www.drupal.org/core/d8-allowed-changes#minor lists patch- and minor- level library updates as allowed during Drupal minor updates, but does not mention how to handle major- level library updates, because those need to be evaluated case by case.

In the case of updating to jQuery 3, we might have an out: the .load() method mentioned in #17 and the other event aliases removed in jQuery 3, were all marked as deprecated in jQuery 1.8, and jQuery 2 was released as being API-compatible with 1.9. Which means these methods could be interpreted as not in the public API of jQuery 2 for semver purposes. Note that by the jQuery team bumping the major version number from 2 to 3, they're signaling that that isn't their interpretation, but as Drupal, I think we're free to interpret differently than them.

I don't know if there are other breaks in jQuery 3, which we'd need to also evaluate with respect to semver.

Something else to consider is that jQuery Migrate can be used to restore (some? all?) legacy APIs. So we could potentially either ship with this in 8.2 and then remove in 8.3, or leave it out of 8.2 and leave it to contrib modules that need it to add it as a dependency.

Tagging for release manager feedback.

catch’s picture

With Symfony 3.0 we posted https://www.drupal.org/node/2403135

#2712647: Update Symfony components to ~3.2 shows that in practice Symfony 3.x has bc breaks which aren't just dropping deprecations from Symfony 2.8. We're trying to minimize those in the issue but even then, on impact vs. disruption it makes sense to update the version.

For me the same goes for jQuery 3.0 - I do think would be worth adding jQuery migrate in 8.2.x and removing it in either 8.3.x or 8.4.x, but unless there was serious breakage, staying on 2.* for the next 3-8 years would feel like shooting ourselves in the foot.

droplet’s picture

effulgentsia’s picture

Removing "Needs release manager review" tag, because that was provided in #19.

I do think would be worth adding jQuery migrate in 8.2.x and removing it in either 8.3.x or 8.4.x

Given that comment, how about making this issue add jquery-migrate to core/assets/vendor and adding that as a JS asset within the jquery entry of core.libraries.yml? That way, it's always included when jquery.js is included.

We can then open a follow-up issue to either make it a separate library within core.libraries.yml or to remove it entirely from core, and make corresponding decisions for when to do either or both of those. But meanwhile, I think that would allow this issue to proceed right away with minimal (possibly 0, depending on how complete jquery-migrate is) BC breakage.

andypost’s picture

I think better to make jQuery library dependent on that new "migrate" library and change record to describe how contrib themes can disable that

Maybe for 8.2.x add new experimental module #2042165: Add a 'deprecated' module that includes deprecated functions only when enabled

Wim Leers’s picture

and adding that as a JS asset within the jquery entry of core.libraries.yml

IMO it would be better to add jquery.migrate as a separate library, and let the jquery library depend on jquery.migrate. That provides the same guarantee. But then it's conceptually separate, which makes it a bit easier for sites with "better jQuery code" to omit the jquery.migrate library.

… which apparently is exactly what @andypost wrote :)

effulgentsia’s picture

I think better to make jQuery library dependent on that new "migrate" library and change record to describe how contrib themes can disable that

+1. I agree that's better than #21. However, do we need a way to prevent someone from adding the jquery.migrate library directly without adding the jquery library? Not that there's a use case for that, just what code is needed to prevent it happening by mistake? I don't think we can have each library depend on the other, can we?

Maybe for 8.2.x add new experimental module #2042165: Add a 'deprecated' module that includes deprecated functions only when enabled

Yes. If/when we have that module, we can make that the module that creates the dependency. Until then, let's proceed with the dependency created in core.libraries.yml for now.

Wim Leers’s picture

However, do we need a way to prevent someone from adding the jquery.migrate library directly without adding the jquery library?

I don't see why. More importantly: we can't. We don't have the concept of "private libraries" like we have "private services".

effulgentsia’s picture

I don't see why.

See https://code.jquery.com/jquery-migrate-3.0.0.js. The JS does in fact depend on the jQuery object existing. Therefore, jquery.migrate should list jquery as a dependency, both for conceptual accuracy, and because including the former in a web page without including the latter results in a JS error.

I don't think we can have each library depend on the other, can we?

I checked, and currently we cannot. We get infinite recursion in LibraryDependencyResolver::doGetDependencies().

I therefore think we need to either add support for mutual (circular) dependencies, or make it all part of a single library, per #21.

which makes it a bit easier for sites with "better jQuery code" to omit the jquery.migrate library

Even if part of the jquery library, can't these sites hook_library_info_alter() out the jquery-migrate JS file?

webchick’s picture

Just noting that jquery.com promotes v3.1.0 as the recommended release on the home page. We should try and figure this out for 8.3.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

frob’s picture

I checked, and currently we cannot. We get infinite recursion in LibraryDependencyResolver::doGetDependencies().

I therefore think we need to either add support for mutual (circular) dependencies, or make it all part of a single library, per #21.

So what is the next step? Add support for circular dependency?

nod_’s picture

Title: Update jQuery to version 3 » Update jQuery to version 3 and jQuery UI to 1.12

jQuery 1.12 got out this month, and does support jQuery 3, we're good to go (but need to update both at the same time).
http://blog.jqueryui.com/2016/09/jquery-ui-1-12-1/

nod_’s picture

Patch with jquery 3, no jquery ui update. Dialog are opening but not placed correctly on the page.

nod_’s picture

Problems with jquery ui latest version, upstream bug: https://bugs.jqueryui.com/ticket/15052#ticket

nod_’s picture

Title: Update jQuery to version 3 and jQuery UI to 1.12 » Update jQuery to version 3
Related issues: +#2809427: Update jQuery UI to 1.12

Split the jquery ui in it's own issue since it might be headache-y

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

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

effulgentsia’s picture

We didn't get this done in time for Drupal 8.3 beta. Updating a library to a new major version is risky to do after beta and not allowed at all in a patch release. So this issue is now likely an 8.4 only issue.

From http://blog.jquery.com/2016/06/09/jquery-3-0-final-released/:

While the 1.12 and 2.2 branches will continue to receive critical support patches for a time, they will not get any new features or major revisions.

Hopefully "for a time" will be at least 7 more months or so, long enough for Drupal 8.4 to be released.

I opened #2852636: Update jQuery to version 2.2.4 to at least get Drupal 8.3 onto the latest 2.2 patch release in the meantime.

gnuget’s picture

Should we postpone this one until #2809427: Update jQuery UI to 1.12 be fixed?

catch’s picture

Priority: Major » Critical

Yes let's postpone. If there's a good reason to combine the issue we could recombine, but this can't go in first as it is, so it's postponed for now.

Also bumping to critical since if we don't get this into 8.4.x we'll be shipping stable 8.x releases with an unsupported jQuery version for 6+ months.

catch’s picture

Title: Update jQuery to version 3 » [PP-1] Update jQuery to version 3
Status: Needs work » Postponed
cilefen’s picture

droplet’s picture

Status: Postponed » Active
Issue tags: +Needs manual testing

I reopen it. Pre-testing & Identify the problem also part of patching and help it move forward.

- jQuery UI isn't the only problem I believed. The code involved show() / hide() / attr() / prop() / ..etc / may be broken also. Many of them we can fix it now without jQuery 3. We able to support jQuery 3 before we upgrade it in CORE! It's possible! For example, Bootstrap theme replaced jQuery UI. They don't need jQuery UI stuff..

- Find out the compatible states of 3rd parties in CORE

The whole world is changing. In the old day, when jQuery release a new version, all 3rd parties push an update. Now what I feel, many libs just deferred the fixes. Or a totally rewritten into Plain JavaScript.

We need to make our CORE compatible with a range of jQuery (2.x ~ 3.x+) when we can. Let's fine-grained it now. (Sometimes, bugs exposed we have bad code, not just jQuery specified)

EDITED:

I don't know how. But I think it's good to announce jQuery 3 is coming into D8.4 or so. Drupal Contribs can support it before jQuery 3 get into CORE also.

Manuel Garcia’s picture

Status: Active » Needs work
Issue tags: +Needs reroll
Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
586.91 KB

Here's a patch upgrading to the latest recommended version on jquery.com, 3.2.1

Status: Needs review » Needs work

The last submitted patch, 42: 2533498-42.patch, failed testing. View results

martin107’s picture

Looking at the test fails there are lots of comments relating to getClientRects()

and referencing the migration guide

https://jquery.com/upgrade-guide/3.0/

Selector

link Breaking change: Behavior of :hidden and :visible

An element is considered now visible if it has a layout box returned from the DOM getClientRects() method, even if that box has a height and/or width of zero. This means that elements such as
or an empty element that don't have height are considered to be visible.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
1.19 MB

Looks like the failures are related to jquery ui, see https://github.com/jquery/jquery/issues/3157

I have ran a couple of the failing tests, Drupal\FunctionalJavascriptTests\Ajax\AjaxTest, and
Drupal\FunctionalJavascriptTests\Dialog\DialogPositionTest on my local machine, with both this patch and the one for jquery ui applied (#2809427: Update jQuery UI to 1.12), and both tests pass, so perhaps there is hope :)

Uploading here both of them combined just to see what the bot finds out.

Status: Needs review » Needs work

The last submitted patch, 45: 2533498-42-and-2809427-28-combined.patch, failed testing. View results

Manuel Garcia’s picture

Tried a few of these just now, AjaxCssTest, AjaxTest and CKEditorIntegrationTest pass locally, but fail on here so I must be missing something...

tacituseu’s picture

...missing indeed:

exception: [Warning] Line 1134 of core/modules/locale/locale.module:
file_get_contents(core/assets/vendor/jquery.ui/ui/data-min.js): failed to open stream: No such file or directory
...
exception: [Warning] Line 1134 of core/modules/locale/locale.module:
file_get_contents(core/assets/vendor/jquery.ui/ui/widgets/sortable-min.js): failed to open stream: No such file or directory
Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
1.45 MB

OK that patch came out wrong, thanks @tacituseu for helping clear it out. Let's try it again...

Wim Leers’s picture

The non-combined patch that we'll eventually want to review looks great (the one in #42).

I don't see any fixes for the BC breaks though (see #17).

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Working on manual testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Postponed
Issue tags: -Needs manual testing

I applied the combined patch and manually tested:

  • /node/add/article (editor+ckeditor+dialogs+file uploads)
  • /node/17/edit (same)
  • /node/17 (contextual + quick edit + ckeditor)
  • /admin/config/content/formats/manage/basic_html (filter + editor + ckeditor)
  • /admin/config/content/formats/add (filter + editor + ckeditor)
  • /admin/structure/taxonomy/manage/tags/overview (table drag)

Found no problems.


I also went over everything in http://jquery.com/upgrade-guide/3.0/. Good news: all of the things that stand out, the things that used to be pretty common … we already fixed those in Drupal core a long time ago. Here are some key examples:

  1. .toggleClass() with either no arguments or a boolean is no longer supported — but we never used that anyway, precisely because it was confusing in the first place
  2. .load() + .unload() + .error() + .bind() + .delegate() are gone, .on() is the successor — we have only been using the latter since long before D8 shipped
  3. .andSelf() is gone, .addBack() is the successor — we have zero uses of the former, half a dozen of the latter

I'd RTBC this (well, the non-combined patch in #42), but this is blocked on #2809427: Update jQuery UI to 1.12 landing first, so marking postponed.

Manuel Garcia’s picture

That's awesome news, thanks a ton @Wim Leers !

drpal’s picture

Assigned: Unassigned » drpal
drpal’s picture

Assigned: drpal » Unassigned

Some notes,

  1. if (typeof response === 'string') {
      response = $.parseJSON(response);
    }
    

    From ajax.es6.js. jQuery's parseJSON method is being deprecated.

  2. From ajax.es6.js, autocomplete.es6.js, ckeditor.admin.es6.js, plugin.es6.js, KeyboardView.es6.js, FieldDecorationView.es6.js, ToolbarVisualView.es6.js, there is a breaking change around how jQuery stores data attributes, camelCase vs. kebab-case.
  3. .width(), .height(), .css("width"), and .css("height") may not always return a numeric value. These are used in a few places,
    dialog.position.es6.js, contextual.es6.js, off-canvas.es6.js, EntityToolbarView.es6.js, dialog.views.es6.js,
     tableheader.es6.js, color.es6.js
  4. .outerWidth(), and .outerHeight() now include the scrollbar width and height. While probably not a massive issue, this can introduce subtle breaks when comparing these values with fixed items.
  5. :hidden, and :visible have subtle changes that might effect tabledrag.es6.js. I haven't fully tested this, and it might be fine, or not immediately obvious.
Wim Leers’s picture

  1. Good catch, but deprecated !== breaking change :)
  2. I checked them, they're not affected.
  3. I checked those calls, they're not affected.
  4. I missed that one.
  5. I tried to test this manually, but couldn't even figure out what the :visible and :hidden were used. There be dragons in tabledrag.js.
drpal’s picture

#56.1 - We should probably still stop using that, but you are correct, not critical.
#56.4 - I don't even know if this is really an issue, some manual testing is probably required.
#56.5 - Yep, without some manual step-through debugging we won't know what those selectors return and if the changes to :hidden, and :visible break things.

SKAUGHT’s picture

#56.5
i did some work toward #2102677: Port tabledrag_example module to Drupal 8.. it may have to do with roots and leaves in tabledrag's

droplet’s picture

- Toolbar is broken (Toggle open/close/verticle/horizontal)
- Tour is broken (Tested: Language)

Wim Leers’s picture

@droplet++
@droplet++
@droplet++
@droplet++

Thanks! That obviously means this can't be RTBC yet.

droplet’s picture

#2894283: Improve Toolbar Height calculation to accommodate jQuery 3.0 changes.

Well, I wonder if it's a bug in jQuery 3. This change makes jQuery less productive.

That means every selector involved height(), width(), outerWidth(), outerHeight() has a chance to give an error.

I understood `non-integer values` is may return floating, not "NaN"

Wim Leers’s picture

Title: [PP-1] Update jQuery to version 3 » Update jQuery to version 3
Status: Postponed » Needs work
Issue tags: +Needs reroll
Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
586.91 KB

#42 still applies fine.
Re-uploading it here to make it clear what to work on (and see what the bot says) =)

EclipseGc’s picture

Status: Needs review » Needs work

Ok, so in the layout-builder work (we're getting a patch together) we have a Drag and Drop layout utility which leverages jQuery UI to move blocks around on the page. This is great, but with core's current version, it returns inaccurate widths which causes draggables to move back to the left-most pixel baseline when dragging if they are a fractional width (imagine a block in a second column jumping over to the first column as soon as you try to drag it). We looked into solving this and decided we'd have to rewrite jQuery's width handling so rather than do that, we looked into updating to a newer version of jQuery since the width handling appeared to have been rewritten. Upon doing so, our problem went away (YAY) however, this patch breaks Settings Tray (which we are also using extensively).

Post application of this patch, settings tray appears on the left side of the screen instead of the right, its title and close buttons are hidden under the administrative menu, and it doesn't stay affixed to the side when you scroll. Probably need to solve all those things before we commit this patch.

Eclipse

drpal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.11 MB
2.35 MB

Moving this back to needs review.

- There is #2894283: Improve Toolbar Height calculation to accommodate jQuery 3.0 changes. which will fix the odd height calculation behavior present in jQuery 3.0
- @EclipseGc, I tested the patch from #63 against ba8156144b and didn't see the described behavior.

nod_’s picture

Didn't see anything weird with outside in either. Tabledrag with keyboard is broken though.

drpal’s picture

@nod_ Can you open an issue and I can take a look? Thanks 😀

effulgentsia’s picture

Tabledrag with keyboard is broken though.

It is in HEAD as well. See #2725259: [regression] Table Drag handles no longer respond to up/down arrow keys. Looks to me like the patch in that issue fixes it equally both for HEAD (jQuery 2.2.4) and with #63 (jQuery 3.2.1) applied. So I don't think that needs to hold up this patch, unless someone is seeing an additional regression introduced by the jQuery upgrade.

effulgentsia’s picture

Tour is broken (Tested: Language)

Is it still? I applied #63, installed Language module, went to /admin/config/regional/language, and the tour button appears in my toolbar, and when I click it, it gives me a tour of that page. What's broken about it?

effulgentsia’s picture

  1. +++ b/core/assets/vendor/jquery/jquery.min.map
    @@ -1 +1 @@
    -{"version":3,"sources":["jquery.js"],"names":["global","factory",...
    +{"version":3,"sources":["jquery-3.2.1.js"],"names":["global","factory",...
    

    Is it a problem that this references jquery-3.2.1.js, but we don't have that file? In our repo, it's named just jquery.js.

  2. +++ b/core/core.libraries.yml
    @@ -341,10 +341,10 @@ html5shiv:
    -    url: https://github.com/jquery/jquery/blob/2.2.4/LICENSE.txt
    +    url: https://raw.githubusercontent.com/jquery/jquery/3.2.1/LICENSE.txt
    

    Is there something that requires us to switch the host name like this? Could we just bump the version number in this patch and have a separate issue for moving the license links from github.com to raw.githubusercontent.com?

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

Since this is a major version upgrade of a library, with known BC breaks, I think a change record would be good. Could someone write one? It could potentially be as minimal as just pointing contrib developers to http://jquery.com/upgrade-guide/3.0/. However, I think information from #55 and/or other tips we can offer contrib developers to watch out for, would be appreciated.

effulgentsia’s picture

Once #70 and #71 are addressed, my inclination is to just commit this in the next day or so, so that:

  1. We have a few days before alpha in which to discover if there's something really bad that needs fixing and/or this reverted before alpha.
  2. People testing 8.4-alpha1 could help us find miscellaneous things that need fixing, and we can get those fixed by beta.

Any objections to that?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
476 bytes
586.9 KB

Not sure about the impact of #70.1

Addressing #70.2.

Re #72: +1

Manuel Garcia’s picture

Status: Needs review » Needs work

The last submitted patch, 73: 2533498-73.patch, failed testing. View results

effulgentsia’s picture

I queued #63 for a retest to see if it has the same failure as #73.

effulgentsia’s picture

Not sure about the impact of #70.1

I'm pretty sure we need to change it to match our actual source filename. See #2485575-20: Update jQuery to 2.1.4 and the comments below that.

effulgentsia’s picture

I queued #63 for a retest to see if it has the same failure as #73.

Indeed it does. So the failure is not an artifact of the reroll, but reflects some change in HEAD that has occurred since #63 was initially posted.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
257.43 KB
586.87 KB

Ah, thanks @effulgentsia for the explanation, doing #70.1 now.

The interdiff is not going to be very useful, but here are the two things I changed:

  • "sources":["jquery-3.2.1.js" -> "sources":["jquery.js"
  • "file":"jquery-3.2.1.min.js" -> "file":"jquery.min.js"

Status: Needs review » Needs work

The last submitted patch, 79: 2533498-79.patch, failed testing. View results

droplet’s picture

Is it still? I applied #63, installed Language module, went to /admin/config/regional/language, and the tour button appears in my toolbar, and when I click it, it gives me a tour of that page. What's broken about it?

It should open in the centre and have gray overlay as BG.

effulgentsia’s picture

For the test failures in #79, I opened a child issue: #2898261: In jQuery 3, $('#') throws syntax errors. Help with fixing that would be most appreciated!

larowlan’s picture

Status: Needs work » Needs review
FileSize
589.5 KB

Combined patch with #2898261: In jQuery 3, $('#') throws syntax errors

Adding @droplet to issue credits (who wrote the patch over there).

Will remove myself straight after, as only uploading combined patch.

larowlan’s picture

effulgentsia’s picture

I opened #2898808: [regression] With jQuery3, modal tour tips lose their grey background and centering for #81. If that can be addressed before this issue gets committed, that would be great. But I'm also fine with not blocking this on that, and leaving it for a follow-up.

effulgentsia’s picture

I drafted a small change record in https://www.drupal.org/node/2898818. If anyone wants to expand it for #71, go for it.

effulgentsia’s picture

Issue tags: -Needs change record
effulgentsia’s picture

Updated combined patch. This combines this issue's #79 with #2898261-16: In jQuery 3, $('#') throws syntax errors.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Per #72, I still think this is worth getting into the alpha if possible. I do have some concerns that jQuery BC breaks like the one in #2898261: In jQuery 3, $('#') throws syntax errors may still be lurking in parts of core that don't have JS test coverage. And because of that, I feel that if this is to make it into alpha at all, it really needs to go in before the freeze, so there's the full freeze window in which to discover a regression bad enough that requires a fix or a revert.

Once #2898261: In jQuery 3, $('#') throws syntax errors lands, the patch in #79 is the one to commit. It looks great to me, and I believe that every comment in this issue for problems discovered has been addressed or has a non-blocking child/related issue open for. Therefore, RTBC, and just as in #2898261-21: In jQuery 3, $('#') throws syntax errors, leaving it to committers in other timezones to make the call on whether to commit this prior to the freeze or not.

Thanks all!

The last submitted patch, 31: core-js-update-jquery-2533498-31.patch, failed testing. View results

larowlan’s picture

Issue tags: +8.4.0 release notes

preemptive tag with 8.4.0 release notes

updating issue credits to Wim Leers for manual testing, reviews, mentoring etc

embarking on a big round of manual testing

larowlan’s picture

Manual testing results on Firefox Nightly 56.0a1 on Linux

Works
- states (tested in installer)
- password strength indicator/match (installer)
- toolbar
- node add/edit forms ckeditor, details element, autocomplete
- contextual links (node page, front page)
- views UI (dialogs, auto preview, preview)
- taxonomy term overview (tabledrag, drop buttons)
- type ahead table filter (simpletest UI, admin/modules)
- filter formats editing (updates allowed html when adding ckeditor buttons)
- drag ordering on filter plugins (filter formats edit)
- placing a block (dialog, highlighting the updated row)
- field ui manage display (changing widgets, formatters, visibility, ajax update of settings)

Doesn't work
- tours - existing issue already opened - I agree this can be a follow up.
- settings tray - at first I thought this was horribly broken, but then I span up a HEAD install and the same issues exist. So putting that down to experimental status - e.g. Go to home page, click 'edit' button to show contextual links. Leave it enabled. Visit modules page and enable settings tray. Everything is broken. Navigate (via url because links don't work) to homepage. Turn off edit and then you can go back to admin pages again. Once you get out of that rabbit hole, things seem to work - although clicking 'advanced block options' when editing a block gets you back into the same rabbit hole (again exists in HEAD).

Adding myself to issue credits for this manual testing (at least an hour).

I agree with @effulgentsia in #89, let's get this into the alpha so we can expose bugs (many eyes make shallow bugs).

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

This needs a reroll because #2898261: In jQuery 3, $('#') throws syntax errors went into 8.4.x (still needs to go into 8.5.x).

cilefen’s picture

We need the uncombined patch. Is #79 the one?

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.

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
586.87 KB

Yes, #79 is the one. Here's a reupload of it.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 96: 2533498-79_0.patch, failed testing. View results

effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community

The failure in the 8.5.x test for #96 is due to that test having started prior to #2898261-31: In jQuery 3, $('#') throws syntax errors being pushed to 8.5.x. I queued a retest. Optimistically setting back to RTBC.

  • cilefen committed 2983a0d on 8.5.x
    Issue #2533498 by Manuel Garcia, effulgentsia, nod_, Dom., larowlan,...

  • cilefen committed ce16619 on 8.4.x
    Issue #2533498 by Manuel Garcia, effulgentsia, nod_, Dom., larowlan,...
cilefen’s picture

Status: Reviewed & tested by the community » Fixed

I double-checked with nod_ and drpal on IRC for signoff on this one. Thank you everyone—the tedious manual testing is appreciated!

I'll publish the change record. Thanks!

  • Committed 2983a0d and pushed to 8.5.x.
  • Committed ce16619 and pushed to 8.4.x.
cilefen’s picture

@xjm and I are concerned by the fact that #2898808: [regression] With jQuery3, modal tour tips lose their grey background and centering impacts Tour's usability more than we understood at the time this was committed. Consequently, we have decided to revert this issue if #2898808 is not fixed before 8.4.0-beta1. So, please help out on #2898808! Thank you, as always.

I am cross-posting this.

xjm’s picture

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

Status: Fixed » Closed (fixed)

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