Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#57946 closed defect (bug) (fixed)

Deprecated jQuery in Farbtastic

Reported by: malae's profile Malae Owned by: audrasjb's profile audrasjb
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.1.1
Component: External Libraries Keywords: has-patch commit
Focuses: javascript Cc:

Description (last modified by sabernhardt)

jQuery.fn.mousedown() event shorthand is deprecated in /wp-admin/js/farbtastic.js
jQuery.fn.bind() is deprecated in /wp-admin/js/farbtastic.js

Attachments (2)

57946.patch (16.4 KB) - added by mrinal013 2 years ago.
Replacement of the old fabrasic.js
57946.1.patch (2.0 KB) - added by sabernhardt 2 years ago.
replace old jQuery methods and add documentation notes

Download all attachments as: .zip

Change History (25)

#1 @sabernhardt
2 years ago

  • Component changed from Shortcodes to External Libraries
  • Description modified (diff)
  • Focuses javascript added
  • Summary changed from Deprecated jQuery to Deprecated jQuery in Farbtastic

Thanks for the report!

Usually, external library issues should be reported upstream, but the Farbtastic GitHub repo does not have recent activity.

This script was moved to the js folder in [43309], though the most recent content change was [16305].

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.3

The upstream repository has been archived by the owner on Feb 27, 2019, and is now read-only.

Looks like this will now be an "adopted" library, and can be patched with necessary changes.

#3 @Presskopp
2 years ago

Farbtastic itself is long dead. But it still is to find in the Twenty Eleven Theme and some remnants are still to find in Core. Instead of patching it, we should completely get rid of it, I'd say.

This ticket was mentioned in PR #4626 on WordPress/wordpress-develop by @mrinal013.


2 years ago
#4

  • Keywords has-patch added

Trac ticket: 57946

#5 @peterwilsoncc
2 years ago

I don't think it's possible to remove the library as it's used by many plugins (a few false positives in there) and quite a few themes.

One of the true positives is CMB2 which is used by many more sites than the install count suggests as it's often included as a package within client project themes and/or plugins.

As the library is already considered adopted by core, it's fine to modify the files in the pull request.

#6 @oglekler
2 years ago

  • Milestone changed from 6.3 to 6.4

If we cannot remove this library, we need to adopt it and this ticket needs a new patch. Due to this, I am moving this into the 6.4 milestone.

#7 @mrinal013
2 years ago

  • Keywords needs-testing added

@peterwilsoncc
Just change some code of the core Farbtastic.js library. So further testing is required.

Last edited 2 years ago by mrinal013 (previous) (diff)

#8 @oglekler
2 years ago

  • Milestone changed from 6.4 to 6.3

Because patch is now ready for testing and this ticket was in 6.3 milestone, I am returning it to this milestone.

@mrinal013 commented on PR #4626:


2 years ago
#9

Reopen PR

@mrinal013 commented on PR #4626:


2 years ago
#10

Reopen PR

@mrinal013 commented on PR #4626:


2 years ago
#11

Reopen PR

#13 @mrinal013
2 years ago

  • Keywords reporter-feedback needs-dev-note added

This ticket was mentioned in Slack in #core-test by mrinal. View the logs.


2 years ago

#15 @mrinal013
2 years ago

  • Keywords reporter-feedback needs-dev-note removed

Removed need-dev-note and reporter-feedback. Because I think they are not appropriate now.

@mrinal013
2 years ago

Replacement of the old fabrasic.js

@sabernhardt
2 years ago

replace old jQuery methods and add documentation notes

#16 follow-up: @sabernhardt
2 years ago

Note: I did not test the patch. I simply reduced the changes, mainly so it's easier to review.

#17 in reply to: ↑ 16 @mrinal013
2 years ago

Replying to sabernhardt:

Note: I did not test the patch. I simply reduced the changes, mainly so it's easier to review.

Thanks.

In https://core.trac.wordpress.org/attachment/ticket/57946/57946.patch, I did somehow similar changes as https://core.trac.wordpress.org/attachment/ticket/57946/57946.1.patch. Additionally, in https://core.trac.wordpress.org/attachment/ticket/57946/57946.patch,

  1. Following coding standard, added with wp-prettier.
  2. Use navigator.userAgent instead of navigator.appVersion, since navigator.appVersion is not recommended https://developer.mozilla.org/en-US/docs/Web/API/Navigator/appVersion.

#18 follow-up: @jorbin
2 years ago

Following coding standard, added with wp-prettier.

This adds alot of unnecessary changes which are harder to review and also mean that if fabrasic ever comes back from the dead, it would really hard to upstream these changes.

#19 in reply to: ↑ 18 @mrinal013
2 years ago

Replying to jorbin:

Following coding standard, added with wp-prettier.

This adds alot of unnecessary changes which are harder to review and also mean that if fabrasic ever comes back from the dead, it would really hard to upstream these changes.

Like this idea.

Also, tested the patch: https://core.trac.wordpress.org/attachment/ticket/57946/57946.1.patch

It works ok on my end.

Last edited 2 years ago by mrinal013 (previous) (diff)

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


2 years ago

#21 @audrasjb
2 years ago

  • Keywords commit added; needs-testing removed

self assigning for commit

#22 @sabernhardt
2 years ago

If navigator.appVersion should (ever) be removed, too, I think it could be better to remove the full IE6 PNG background section.

#23 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Resolution set to fixed
  • Status changed from new to closed

In 56252:

External Libraries: Update deprecated jQuery code in Farbtastic lib.

This changeset updates some deprecated jQuery code in the Farbtastic external library. As this vendor script is not maintained anymore, this changeset
also adds a docblock to specify that the library has been "adopted" by WP Core.

Props Malae, sabernhardt, SergeyBiryukov, Presskopp, mrinal013, peterwilsoncc, oglekler, jorbin.
Fixes #57946.

Note: See TracTickets for help on using tickets.