Fix jQuery warnings as WordPress upgraded it from 1.12.4-wp to 3.5.1 by htdat · Pull Request #649 · Automattic/edit-flow

htdat · 2021-05-18T09:22:09Z

Description

This PR has done these changes to fix all warnings:

Old	New	Ref
jQuery.expr[':']	jQuery.expr.pseudos
.click(fn)	.on("click", fn)	Reference
.click()	.trigger("click")	Reference
.hover(fn1, fn2)	.on("mouseenter", fn1).on("mouseleave", fn2)	Reference
removeAttr('selected');	prop( 'selected', false );	Reference
.attr('selected');	.prop( 'selected', true );	Reference
.bind	.on	Reference
$('.myButton').live('click', function)	$( document ).on('click', '.myButton', function)	Reference

The following events are changed similarly to click above: blur, focus, focusin, focusout, resize, scroll, dblclick, mousedown, mouseup, mousemove, mouseover, mouseout, mouseenter, mouseleave, change, select, submit, keydown, keypress, keyup, and contextmenu. - ref

Steps to Test

Do not apply this PR yet.
Install https://wordpress.org/plugins/enable-jquery-migrate-helper/ and activate it.
Visit http://site.test/wp-admin/tools.php?page=jqmh and set it up like the following:

Visit the following pages to see the issue:

http://site.test/wp-admin/index.php?page=calendar
http://site.test/wp-admin/index.php?page=story-budget
All pages under Edit Flow menu in wp-admin http://site.test/wp-admin/admin.php?page=ef-settings
Page listing all posts http://site.test/wp-admin/edit.php
Block and Classic editor pages for a post

Apply this PR.
Open tab Console in your browser.
Vist all pages above
Expect that there is no more warning regarding Edit Flow in wp-admin, and no errors in Console tab.

Note

You may apply this diff so that we can avoid the cache busting issue in browsers.

Index: edit_flow.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- edit_flow.php	(revision 883e80484e860bbd12aced41c4d4eca733344027)
+++ edit_flow.php	(date 1621319608357)
@@ -46,7 +46,7 @@
 }
 
 // Define contants
-define( 'EDIT_FLOW_VERSION' , '0.9.6' );
+define( 'EDIT_FLOW_VERSION' , '0.9.7-dev-' . time() );
 define( 'EDIT_FLOW_ROOT' , dirname(__FILE__) );
 define( 'EDIT_FLOW_FILE_PATH' , EDIT_FLOW_ROOT . '/' . basename(__FILE__) );
 define( 'EDIT_FLOW_URL' , plugins_url( '/', __FILE__ ) );

…4-wp to 3.5.1 Ref https://make.wordpress.org/support/2020/12/handling-potential-jquery-issues-in-wordpress-5-6/

Ref https://github.com/jquery/jquery-migrate/blob/main/warnings.md#jqmigrate-jqueryfnhover-is-deprecated

Ref https://stackoverflow.com/a/14354091

mikeyarce · 2021-05-31T17:27:57Z

@htdat I did test and I can see that the warnings no longer show up, that's great!

I'm wondering though, how can we make sure that all the functionality is still the same? Do you have any steps to test for each change, or do we have any automated tests for those?

Do we need to test across different browsers, operating systems, versions of WP?

nielslange

I still see the warnings on two pages.

http://editflow.local/wp-admin/index.php?page=calendar:

⚠️ JQMIGRATE: jQuery.expr[':'] is deprecated; use jQuery.expr.pseudos
⚠️ JQMIGRATE: jQuery.isArray is deprecated; use Array.isArray
⚠️ JQMIGRATE: jQuery.isFunction() is deprecated
⚠️ JQMIGRATE: jQuery.unique is deprecated; use jQuery.uniqueSort

http://editflow.local/wp-admin/index.php?page=story-budget:

⚠️ JQMIGRATE: jQuery.expr[':'] is deprecated; use jQuery.expr.pseudos

Do these warning not appear on your end? I also added define( 'EDIT_FLOW_VERSION' , '0.9.7-dev-' . time() ); to ensure that I'm not facing a caching problem while testing. Yet, on these two pages, the warnings remain.

htdat · 2021-06-03T06:53:37Z

Thanks for your review, @nielslange!

I still see the warnings on two pages.

When looking at console trace, you can see that these warnings are not from Edit Flow. Therefore, we're not concerned about them:

htdat · 2021-06-03T09:58:12Z

@mikeyarce

I'm wondering though, how can we make sure that all the functionality is still the same? Do you have any steps to test for each change, or do we have any automated tests for those?

Well, that's tricky! We do not have automated tests for all of these jQuery changes. I am going to go through each of the changes and write up steps for testing. It looks like that for now https://d.pr/i/x8b4pQ

I have reviewed 5/12 files today. I am changing this PR to Draft. After finishing the reviews for all 12 files, I will move it back.

Do we need to test across different browsers, operating systems, versions of WP?

Basically, this PR follows up all official guidelines from jQuery, I think testing in one environment (one browser, OS, and WP) is enough. Otherwise, if an issue happens, its cause is likey from jQuery, browser, OS, or WP.

htdat

Write down all possible test cases.

htdat · 2021-06-03T07:09:58Z

 		t.what = '#usergroup-';

-		$('.editinline').live('click', function(){
+		$( document ).on( 'click', '.editinline', function(){


Visit http://valet-vipgo-3279.test/wp-admin/admin.php?page=ef-user-groups-settings

Hover on a group name, then click Quick Edit.

✅ if we're able to see relevant fields to update this group.

This looks good but I found a bug related to this: #654

✅

htdat · 2021-06-03T07:44:05Z


 		// prepare the edit row
-		row.keyup(function(e) { if(e.which == 27) return inlineEditUsergroup.revert(); });
+		row.on( 'keyup', function(e) { if(e.which == 27) return inlineEditUsergroup.revert(); });


Visit http://valet-vipgo-3279.test/wp-admin/admin.php?page=ef-user-groups-settings

Hover on a group name, then click Quick Edit.

See relevant fields to update this group, and try to edit some texts.

✅ if we press Escape key and the Quick Edit fields are closed and their values are reset.

Note: e.which == 27 here is equal to pressing "Escapce" key.

htdat · 2021-06-03T07:50:04Z

+		$('a.cancel', row).on( 'click', function() { return inlineEditUsergroup.revert(); });
+		$('a.save', row).on( 'click', function() { return inlineEditUsergroup.save(this); });
+		$('input, select', row).on( 'keydown', function(e) { if(e.which == 13) return inlineEditUsergroup.save(this); });


Visit http://valet-vipgo-3279.test/wp-admin/admin.php?page=ef-user-groups-settings

Hover on a group name, then click Quick Edit.

See relevant fields to update this group, and try to edit some texts.

✅ if we click Cancel button and the Quick Edit fields are closed and their values are reset.
✅ if we click Update User Group button and the Quick Edit fields are saved then closed.
✅ if we press Enter key and the Quick Edit fields are saved then closed.

Note: e.which == 13 is for the Enter key.

htdat · 2021-06-03T07:56:36Z

+		$('input, select', row).on( 'keydown', function(e) { if(e.which == 13) return inlineEditUsergroup.save(this); });

-		$('#posts-filter input[type="submit"]').mousedown(function(e){
+		$('#posts-filter input[type="submit"]').on( 'mousedown', function(e){


This seems redundant as there is no element with this ID posts-filter in this page http://valet-vipgo-3279.test/wp-admin/admin.php?page=ef-user-groups-settings

Then this event is not triggered.

Yeah this seems redundant. It looks like other pages like wp-admin/admin.php?page=ef-editorial-metadata-settings use this but not User Groups

Created #664

htdat · 2021-06-03T08:13:49Z

+	$("#toggle_details").on( 'click', function() {
+		$(".post-title > p").toggle('hidden');


TODO

I am going to remove this function as I am not able to find any element for $("#toggle_details"):

Only one result (exactly this line) with this search https://github.com/Automattic/Edit-Flow/search?q=toggle_details&type=

Check on the HTML as well as browser console with this code:

jQuery("#toggle_details")

Looks like toggle_details was removed here:
95c6874

Safe to remove from this! ✅

Created #663

htdat · 2021-06-04T08:42:01Z

 							.text(filter.label)
 							.attr(options._filterIdAttr, i)
-							.bind('click', _filterTabEvent)
+							.on('click', _filterTabEvent)


Try editing a post.

Find the Notifications section at the bottom.

✅ if we can click All and Selected and they act properly.

htdat · 2021-06-04T08:46:56Z

+					.on('search', _filterInputSearch)
+					.on('keydown', _filterInputKeydown)
+					.on('keyup', _filterInputKeyup)


Try editing a post.

Find the Notifications section at the bottom.

✅ if we type some letters in the Search field and the list below will be filtered in accordance with.

htdat · 2021-06-04T08:55:06Z

 						.addClass(options.inputPlaceholderClass)
 						.val(options.inputPlaceholder)
-						.focus(function() {
+						.on( 'focus', function() {


See the comment in the L74 above:

// Fallback for browsers that don't support placeholders

I think this is a very low risk and we do not need a test case for it. Because it's difficult to find a browser not supporting placeholder nowadays: https://caniuse.com/input-placeholder

htdat · 2021-06-04T09:12:39Z

 	// We need :contains to be case insensitive so lets redefine it as containsi (i for case insensitive)
 	// http://stackoverflow.com/questions/187537/is-there-a-case-insensitive-jquery-contains-selector
-	jQuery.expr[':'].containsi = function(a, i, m){
+	jQuery.expr.pseudos.containsi = function(a, i, m){


Try editing a post.

Find the Notifications section at the bottom.

Search for both upper and lower cases for a specific user.

✅ if the filtered list always returns a result no matter the search input is upper or lower case.

htdat · 2021-06-04T10:11:45Z

 				tp_inst.$altInput = $(o.altField).css({
 					cursor: 'pointer'
-				}).focus(function() {
+				}).on ( 'focus', function() {


This is a library and it's being used for the Date field in Editorial Metadata module - related PR #169
We can see it in action when editing a post.

I see that the author has not changed this yet. I tried finding a way to test this file but it's not clear how I can replicate these events.

https://github.com/trentrichardson/jQuery-Timepicker-Addon/blob/7160ce59c660462f4d3379951f1e141131bcb02c/src/jquery-ui-timepicker-addon.js#L287

Hmmm yeah this is a tough one....

I think as long as the date picker works we're good. I tested it on the post editing screen and it worked for me.
✅

The author no longer maintains this library so I've created a new GH issue on our end to replace it.

#660

htdat · 2021-06-04T10:21:16Z

@mikeyarce and @nielslange I've found out all the test cases for most changes here. Otherwise, I wrote down explanations and my researches for that.

For each of these test cases, I also commented on the code change to verify my test cases correct. I also look at browser console when testing and writing these tests.

To review those tests, it would be better to start here https://github.com/Automattic/Edit-Flow/pull/649/files
Finally, this takes time for manual testing so we can review this together in a call if possible.

mikeyarce · 2021-06-07T21:13:14Z

@htdat excellent work with all these test cases and examples! I went through each one and tested them myself and I think this is good to go. I left a few comments about other things that I found but nothing to do with these changes.

I think after merging this what would be nice would be:

Create an issue to track all the code we need to remove (all the unused JS for example)
Create an issue about the datepicker as we might need to find a substitute eventually since that library seems to have not been updated since 2016

What do you think?

nielslange · 2021-06-08T11:26:26Z

Thanks for your review, @nielslange!

I still see the warnings on two pages.

When looking at console trace, you can see that these warnings are not from Edit Flow. Therefore, we're not concerned about them:

Oh, I must have missed that, @htdat. In that case, the PR looks good to me.

htdat

I've created 4 issues: 661 -> 644 to report issues raised from this PR.

htdat · 2021-06-09T04:42:43Z

 				tp_inst.$altInput = $(o.altField).css({
 					cursor: 'pointer'
-				}).focus(function() {
+				}).on ( 'focus', function() {


The author no longer maintains this library so I've created a new GH issue on our end to replace it.

#660

htdat · 2021-06-09T04:48:41Z

 		function focus_on_load()
 		{
-			$('input[rel="' + options.randomElement + '"]').get(0).focus();
+			$('input[rel="' + options.randomElement + '"]').get(0).trigger( 'focus' );


Created this issue #661

htdat · 2021-06-09T05:22:24Z

 		ef_append_to_dropdown('select[name="_status"]');
 		// Refresh the custom status dropdowns everytime Quick Edit is loaded
-		jQuery('#the-list a.editinline').bind( 'click', function() {
+		jQuery('#the-list a.editinline').on( 'click', function() {


Created #662

htdat · 2021-06-09T05:26:39Z

+	$("#toggle_details").on( 'click', function() {
+		$(".post-title > p").toggle('hidden');


Created #663

htdat · 2021-06-09T07:06:54Z

+		$('input, select', row).on( 'keydown', function(e) { if(e.which == 13) return inlineEditMetadataTerm.save(this); });

-		$('#posts-filter input[type="submit"]').mousedown(function(e){
+		$('#posts-filter input[type="submit"]').on( 'mousedown', function(e){


Created #664

htdat · 2021-06-09T07:07:07Z

+		$('input, select', row).on( 'keydown', function(e) { if(e.which == 13) return inlineEditUsergroup.save(this); });

-		$('#posts-filter input[type="submit"]').mousedown(function(e){
+		$('#posts-filter input[type="submit"]').on( 'mousedown', function(e){


Created #664

htdat · 2021-06-09T07:07:56Z

+		$('input, select', row).on( 'keydown', function(e) { if(e.which == 13) return inlineEditCustomStatus.save(this); });

-		$('#posts-filter input[type="submit"]').mousedown(function(e){
+		$('#posts-filter input[type="submit"]').on( 'mousedown', function(e){


Created #664

htdat · 2021-06-09T07:11:42Z

@mikeyarce and @nielslange - thanks for your reviews!

Create an issue to track all the code we need to remove (all the unused JS for example)

Create an issue about the datepicker as we might need to find a substitute eventually since that library seems to have not been updated since 2016

I've created issues #660 -> #664 to track them. I am going to merge this PR now. 🙌

htdat added 3 commits May 18, 2021 14:40

Fix various issues for jQuery when WP core upgraded jQuery from 1.12.…

cbd2fbc

…4-wp to 3.5.1 Ref https://make.wordpress.org/support/2020/12/handling-potential-jquery-issues-in-wordpress-5-6/

Correct the change of hover event

825510a

Ref https://github.com/jquery/jquery-migrate/blob/main/warnings.md#jqmigrate-jqueryfnhover-is-deprecated

Correct .live() function

883e804

Ref https://stackoverflow.com/a/14354091

htdat added type: bug status: needs review labels May 18, 2021

htdat requested review from mikeyarce, natebot and nielslange May 18, 2021 09:22

htdat self-assigned this May 18, 2021

Change .live to .on in file editorial-metadata-configure.js

8a52203

nielslange reviewed Jun 2, 2021

View reviewed changes

htdat marked this pull request as draft June 3, 2021 10:00

htdat commented Jun 4, 2021

View reviewed changes

htdat marked this pull request as ready for review June 4, 2021 10:19

mikeyarce approved these changes Jun 7, 2021

View reviewed changes

htdat commented Jun 9, 2021

View reviewed changes

htdat merged commit 6ade734 into master Jun 9, 2021

htdat deleted the fix/jquery-issues branch June 9, 2021 07:12

This was referenced Jun 11, 2021

Remove redundant jQuery selector #the-list a.editinline #668

Merged

Remove redundant jQuery selector #toggle_details #669

Merged

Remove jquery.quicksearch.js file #670

Closed

Remove redundant jQuery selector #posts-filter input[type="submit"] #672

Closed

		$("#toggle_details").on( 'click', function() {
		$(".post-title > p").toggle('hidden');

Conversation

htdat commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to Test

Note

Uh oh!

mikeyarce commented May 31, 2021

Uh oh!

nielslange left a comment

Choose a reason for hiding this comment

Uh oh!

htdat commented Jun 3, 2021

Uh oh!

htdat commented Jun 3, 2021

Uh oh!

htdat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htdat commented Jun 4, 2021

Uh oh!

mikeyarce commented Jun 7, 2021

Uh oh!

nielslange commented Jun 8, 2021

Uh oh!

htdat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htdat commented May 18, 2021 •

edited

Loading

htdat commented Jun 9, 2021 •

edited by nielslange

Loading