Opened 8 years ago
Closed 8 years ago
#38550 closed defect (bug) (fixed)
Remove jQuery dependency from wp-custom-header.js
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.
8 years ago
#3
@
8 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.
8 years ago
#5
@
8 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
@
8 years ago
38550.2.diff seems like a good solution to me.
#7
@
8 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.
#8
@
8 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
@
8 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:
↓ 12
@
8 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.
#11
@
8 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
@
8 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.
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 intwentyfourteen
, am I missing something? I was expecting it intwentyseventeen
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?