WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#38550 closed defect (bug) (fixed)

Remove jQuery dependency from wp-custom-header.js

Reported by: joemcgill Owned by: joemcgill
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: javascript Cc:

Description

In [38985], we used jQuery to trigger a custom wp-custom-header-video-loaded event once a video handler has run. We should use native JS events here instead so we can remove the jQuery dependency.

Attachments (4)

38550.diff (2.0 KB) - added by adamsilverstein 5 years ago.
38550.2.diff (2.1 KB) - added by joemcgill 5 years ago.
38550.3.diff (2.2 KB) - added by adamsilverstein 5 years ago.
38550.4.diff (2.1 KB) - added by joemcgill 5 years ago.

Download all attachments as: .zip

Change History (17)

This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.


5 years ago

#2 @adamsilverstein
5 years ago

Native events would be nice here... unfortunately ie8 requires some kludgy workarounds.... Do we need to support ie8?http://caniuse.com/#feat=addeventlistener

As far as I can see the listener for wp-custom-header-video-loaded is only added in twentyfourteen, am I missing something? I was expecting it in twentyseventeen

Do we really need to remove this dependency? twentyfourteen already loads jQuery and uses it extensively in functions.js

I am attaching a patch that takes an alternate approach, exposing the wp global to both closures and using a callback function. That's still a bit clunky though and I am not convinced its a good idea.

When creating the patch I noticed that jQuery was not listed as a dependency where wp-custom-headers.js is enqueued, to jQuery was getting enqueued elsewhere. If jQuery is already a dependency for the context, maybe we don't need to worry about removing it?

#3 @peterwilsoncc
5 years ago

Related #38585.

wp-custom-header.js doesn't support browsers without addeventlistener support. The dependency can be removed and custom events used wihout reducing browser support. http://caniuse.com/#feat=customevent

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


5 years ago

@joemcgill
5 years ago

#5 @joemcgill
5 years ago

  • Keywords 2nd-opinion has-patch added; needs-patch removed
  • Owner set to joemcgill
  • Status changed from new to accepted

38550.2.diff Is a basic implementation using native events instead of jQuery's trigger() method that fires after a custom video is loaded so themes can bind any additional JS that needs to fire once the video is loaded. I'm unsure about dispatching these events from the global document root or if these should be dispatched from elsewhere.

#6 @celloexpressions
5 years ago

38550.2.diff seems like a good solution to me.

#7 @adamsilverstein
5 years ago

Testing 38550.2.diff in ie8, i get an error message/notice when viewing the front page in twentyfourteen. In 38550.3.diff I added capability checks before using the native events to avoid this error.

https://cl.ly/3B0m1Q0r0346/Dashboard_2016-11-02_10-26-20.jpg

#8 @joemcgill
5 years ago

  • Keywords 2nd-opinion removed

Thanks @adamsilverstein, I was assuming the browser check wasn't necessary on the wp-custom-header.js side since we're bailing early if there is no support for addEventListener, but didn't think to do the same on the theme side. 38550.3.diff looks good to me, assuming the overall strategy here makes sense.

#9 @bradyvercher
5 years ago

I like the idea of removing the jQuery dependency, but after tinkering a bit, it might be more helpful if we go all in with jQuery instead.

If I recall correctly, play and pause buttons are required for accessibility, so those would likely need to be inserted dynamically. It would be less verbose with jQuery, while some of the existing code could also be simplified a bit.

Most themes are going to be using jQuery, so they'll understand how to listen for events using it. With this approach, they'll have to learn an alternative approach. Granted it is a native API, so that learning experience could be worthwhile, depending on your vantage point.

It's not a major deal either way, I just thought I'd throw that out there.

Based on some testing, there are some additional changes that need to happen here, though. The supportsVideo() method needs to be overridable from a theme or plugin, but it's currently called privately, so it can't be replaced. Should I upload a patch to show what that might look like?

#10 follow-up: @joemcgill
5 years ago

Thanks @bradyvercher,

Most themes are going to be using jQuery, so they'll understand how to listen for events using it. With this approach, they'll have to learn an alternative approach. Granted it is a native API, so that learning experience could be worthwhile, depending on your vantage point.

I don't think there is any problem with themes using jQuery to listen for this event. In fact, we could do something like this in Twenty Fourteen if we want:

// Update masthead offset once a custom header video loads.
$( document ).on( 'wp-custom-header-video-loaded', function() {
	mastheadOffset = $( '#site-header' ).height();
} );

The other two issues—play/pause button and allowing themes to replace the supportsVideo() test—should be separate tickets, IMO.

@joemcgill
5 years ago

#11 @joemcgill
5 years ago

For testing purposes, I've added 38550.4.diff which uses jQuery to listen for the wp-custom-header-video-loaded event in Twenty Fourteen.

#12 in reply to: ↑ 10 @bradyvercher
5 years ago

Replying to joemcgill:

I don't think there is any problem with themes using jQuery to listen for this event.

I think you're right about that. I was thinking about the reverse scenario where I don't think you can use addEventListener() to catch an event triggered by jQuery. The approach in 38550.4.diff does make more sense to me.

#13 @joemcgill
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 39102:

Themes: Remove jQuery dependency from wp-custom-header.js.

In [38985], we used jQuery to trigger a custom event once a video
handler has completed so themes, like Twenty Fourteen, can execute
their own adjustments after the header video has loaded.

This replaces the jQuery trigger() method with a native event and
updates Twenty Fourteen accordingly.

Props adamsilverstein, joemcgill.
Fixes #38550.

Note: See TracTickets for help on using tickets.