WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 2 hours ago

#51812 reviewing task (blessed)

Update jQuery step three

Reported by: azaozz Owned by: SergeyBiryukov
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: External Libraries Keywords: has-patch early needs-testing needs-dev-note
Focuses: javascript Cc:

Description

Follow up from #37110 and #50564.

Remove jQuery Migrate 3.3.x.

This ticket represents step 3 in the following roadmap for updating jQuery to 3.x in Core: https://make.wordpress.org/core/2020/06/29/updating-jquery-version-shipped-with-wordpress/.

Change History (31)

#1 @azaozz
2 months ago

To be able to disable jQuery Migrate, all JQMIGRATE warnings that are outputted to the browser console (when SCRIPT_DEBUG is enabled) will have to be fixed in core, themes and plugins.

This is a very large task that likely will need some workarounds for older and/or unsupported themes and plugins.

A first step would be to fix all of the (old) js in core. At the same time the core js can be updated to use the new features and syntax from jQuery 3.5+.

#2 @mgol
2 months ago

If you decide to call jQuery.UNSAFE_restoreLegacyHtmlPrefilter() as part of #50564 then the Migrate warnings will include violations related to self-closed tags. Before you remove Migrate, you'll need to remove that call (or you can do it all in one go).

#3 @SergeyBiryukov
2 months ago

  • Milestone changed from Future Release to 5.7

Moving to 5.7, per the linked roadmap.

This ticket was mentioned in PR #812 on WordPress/wordpress-develop by Clorith.


6 weeks ago

  • Keywords has-patch added

Trac ticket: https://core.trac.wordpress.org/ticket/51812

This is an initial patch, with many minor adjustments to a wide array of core files.

Updating the jQuery usage in core is something we should get in early, and will also require some upstream patches to libraries used by WordPress (or, in some cases, updates may exist, the initial focus of this PR is the direct use in core).

A fair amount of deprecations that are not addressed in this PR are being thrown by jQuery UI.

The process taken to identify deprecation issues in core has been a manual one, the initial plan here was to merely do some fancy regex search replace commands, but a lot of components built by core are using similar function names and argument parameters, so instead this has been a manual endeavor, and probably should remain so to keep accidentally breaking changes to a minimum during the alpha stage.

In testing, each admin page was visited, and deprecation warnings addressed individually until there were external libraries throwing warnings. Individual interactions on the page are performed (for example on the classic post editor screen, change the publication time, change the post status), although I make no guarantee there's not scenarios where an interaction may have been missed, so would be good to get more eyes involved.

One exception that is in core, but has not been patched up
{{{js
Accessibility mode.
$( window ).on( 'load', function() {

component.setupAccessibleMode();

});
}}}

This is used in the different components under `src/js/_enqueues/wp/widets` which are triggering "too late" in some scenarios, and jQuery throws a warning that the load event has already fired by the time the component is initialized.

#5 @Clorith
6 weeks ago

#52056 was marked as a duplicate.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


5 weeks ago

#7 @hellofromTonya
5 weeks ago

  • Keywords early needs-testing added

Marking as needs-testing and early, just as Step 2 in 5.6 was marked.

#8 @Clorith
5 weeks ago

#52091 was marked as a duplicate.

This ticket was mentioned in Slack in #core by lukecarbis. View the logs.


4 weeks ago

#10 @lukecarbis
4 weeks ago

@Clorith mentions in the Pull Request:

A fair amount of deprecations that are not addressed in this PR are being thrown by jQuery UI.

We discussed this in today's bug scrub, and think it could potentially be a good idea to split the jQuery UI piece into a separate issue.

#11 @prbot
4 weeks ago

azaozz commented on PR #812:

Great job!

Just a small suggestion: seems it may be better to replace $.trim( var ) with ''.trim( var ) instad of var.toString().trim(). The last would throw an error when var is undefined, both $.trim() and ''.trim() would return an empty string. All browsers since IE10 have that, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trim. For multiple uses can even set local version: var trim = ''.trim; at the top, etc. Alternatively can do var = var || '' before using var.toString().

For the way $( window ).on( 'load', is used in the widgets js perhaps may need to look at Document.readyState, see: https://developer.mozilla.org/en-US/docs/Web/API/Document/readyState. Seems it was incorrect before the jQuery update but now that throws a warning.

#12 @prbot
4 weeks ago

Clorith commented on PR #812:

Good catch on the .trim() bit, I half expected it to be validated before it reached the point of trying to use it.

The .on( 'load' bit has likely been wrong for some time as you say, yeah, isn't the readyState technically the same here though, and would still be "too late"? Or am I misreading you perhaps?

#13 @prbot
4 weeks ago

azaozz commented on PR #812:

...isn't the readyState technically the same

Yeah, document.onreadystatechange can be used instead of "DOM ready", etc. Thinking it can look at the "property" document.readyState. Perhaps something like:

if ( document.readyState === 'complete' ) {
    // Page is fully loaded.
    component.setupAccessibleMode();
} else {
    // Page is still loading.
    $( window ).on( 'load', function() {
        component.setupAccessibleMode();
    });
}
Last edited 4 weeks ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 weeks ago

#15 @audrasjb
2 weeks ago

  • Keywords needs-dev-note added

#16 @prbot
2 weeks ago

Clorith commented on PR #812:

Ahh, I see what you mean then, that does make sense then.

I've patched the widgets to use your suggestion above (it made the most sense right off the bat).

For the toString().trim() section, I chose the option of var = var || '';, as a quick check showed it being the way core has previously done it in another location as well, so sticking to the same made sense.

#17 @hellofromTonya
2 days ago

@Clorith @azaozz What's left for this ticket to get it merged for pre-Beta 1 testing?

#18 follow-up: @Clorith
2 days ago

We'll want to get in the core fixes for deprecations sooner rather than later (did the timeline for 5.7 change, or am I just terrible at keeping track here?)

As for removing jQuery Migrate being enabled by default, I think we will want to move that back one release (to 5.8). I would ideally like to have more stewing time to capture any missed deprecations in core, and give the jQuery UI team a chance to get their deprecation fixes done as well, so that we do not need to build that our selves.

#19 @hellofromTonya
2 days ago

did the timeline for 5.7 change, or am I just terrible at keeping track here?

Nope, 5.7 timeline hasn't changed. Beta 1 is 2 Feb, ie 2 weeks away.

I would ideally like to have more stewing time to capture any missed deprecations in core, and give the jQuery UI team a chance to get their deprecation fixes done as well, so that we do not need to build that our selves.

Makes sense for the jQuery deprecations.

For those following, the jQuery UI ticket is #52163.

Last edited 2 days ago by hellofromTonya (previous) (diff)

#20 in reply to: ↑ 18 @hellofromTonya
2 days ago

Replying to Clorith:

We'll want to get in the core fixes for deprecations sooner rather than later

Are these captured in PR 812?

#21 follow-up: @Clorith
2 days ago

Yes, PR 812 is good to go as a first push. There will very likely be more minor patches needing to go in for this once we get some real testing going and places that were missed are discovered. But we really just need to get it in to start that process (I had hoped for early to be... much earlier than this :/ but it is what it is)

#22 @hellofromTonya
2 days ago

Agreed. Get it merged. Then monitor for other minor patch needs.

Is anything blocking PR812 from being merged?

#23 @Clorith
2 days ago

I believe it's on the radar for @SergeyBiryukov (just needs a committer really to give it the once-over and get it in)

This ticket was mentioned in Slack in #core-committers by hellofromtonya. View the logs.


2 days ago

#25 @SergeyBiryukov
44 hours ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#26 in reply to: ↑ 21 ; follow-up: @SergeyBiryukov
38 hours ago

Replying to Clorith:

Yes, PR 812 is good to go as a first push.

Thanks for the PR! Just noting it needs a refresh after [49944] / #46872 and [49973] / #52073.

The first three conflicts are fairly easy to resolve (just remove #doaction2), but I'm less sure about the change in js/_enqueues/wp/widgets/text.js. It looks like the change from this PR takes a different approach at fixing the same issue, so we need to decide on the preferred approach. Since [49973] appears to be tested and confirmed, do we still need $( window ).on( 'load', ... ) there? /cc @azaozz

Last edited 38 hours ago by SergeyBiryukov (previous) (diff)

#27 @hellofromTonya
8 hours ago

  • Keywords needs-refresh added

Adding needs-refresh for resolving the merge conflicts Sergey noted above.

#28 in reply to: ↑ 26 @azaozz
5 hours ago

Replying to SergeyBiryukov:

Since [49973] appears to be tested and confirmed, do we still need $( window ).on( 'load', ... ) there?

Thinking best would be to keep the changes from [49973] as they make sense. Right, they are tested, also looking at #40986 and [40941] it's not clear why this was originally running on $( window ).on( 'load' instead of DOM ready.

#29 @Clorith
4 hours ago

I suspect the reason was to account for different timings of when they were enqueued, if called by plugins or themes in other locations as well?

If that were the case, the readyState check would help account for this, and not cause unexpected behaviors in anything calling them in a strange location, I'll let someone make that judgement call on which is the preferred approach here though.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 hours ago

#31 @SergeyBiryukov
2 hours ago

  • Keywords needs-refresh removed

After some testing, [49973] seems to work as expected for me too, so I guess we can skip that part of the patch for now and get back to it if any follow-up issues are reported.

I have already resolved the other merge conflicts locally and will commit shortly.

Note: See TracTickets for help on using tickets.