WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#34204 closed defect (bug) (fixed)

Add fallbacks for WP oEmbed JavaScript & CSS

Reported by: peterwilsoncc Owned by: johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch
Focuses: javascript Cc:

Description (last modified by peterwilsoncc)

Improve JavaScript and CSS support for IE8+, fallbacks include:

  • those listed below
  • PNG fallbacks for icons (IE9-)
  • feature detect select and focus

Original description follows:


Add the following to each of the WP oEmbed JavaScript to avoid potential for errors:

  • window.load fallback for document.DOMContentLoaded event
  • feature detects presence of addEventListener and document.querySelector before attempting to run

I'm in two minds about each of these to be honest (more so the event fallback), will leave to another to decide.

Attachments (13)

34204.1.diff (2.2 KB) - added by peterwilsoncc 5 years ago.
embed-icons.zip (316.4 KB) - added by melchoyce 5 years ago.
x.png (14.3 KB) - added by melchoyce 5 years ago.
34204.2.diff (20.0 KB) - added by peterwilsoncc 5 years ago.
embed.zip (1.2 KB) - added by peterwilsoncc 5 years ago.
34204.3.diff (9.7 KB) - added by peterwilsoncc 5 years ago.
34204.4.diff (12.0 KB) - added by peterwilsoncc 5 years ago.
34204.5.diff (13.0 KB) - added by peterwilsoncc 5 years ago.
34204.diff (12.3 KB) - added by swissspidy 5 years ago.
34204.6.diff (12.4 KB) - added by pento 5 years ago.
34204.7.diff (12.6 KB) - added by pento 5 years ago.
34204.8.diff (35.4 KB) - added by peterwilsoncc 5 years ago.
34204.9.diff (11.1 KB) - added by peterwilsoncc 5 years ago.

Download all attachments as: .zip

Change History (49)

#1 @peterwilsoncc
5 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.4

#3 follow-up: @pento
5 years ago

For DOMContentLoaded and addEventListener fallbacks, lets do the same as in wp-emoji-loader.js.

querySelector is only a problem for IE < 8. I'd like to at least have some sort of vaguely working fallback - the share dialog working, at a minimum.

#4 in reply to: ↑ 3 ; follow-up: @peterwilsoncc
5 years ago

  • Keywords needs-refresh added

Replying to pento:

querySelector is only a problem for IE < 8. I'd like to at least have some sort of vaguely working fallback - the share dialog working, at a minimum.

Coolio, can do. A few questions:

  • SVG share icons will need a fallback for IE < 9, text, external PNG or inline PNG?
  • Any objection to adding in some IDs for the share dialog fallback? I'd like to avoid selecting a tonne of elements and checking className.

Note to self: feature check focus() in refresh.

#5 in reply to: ↑ 4 @pento
5 years ago

Replying to peterwilsoncc:

  • SVG share icons will need a fallback for IE < 9, text, external PNG or inline PNG?

I'm inclined to make them external. No point in sending them down the wire for sites that won't use them.

  • Any objection to adding in some IDs for the share dialog fallback? I'd like to avoid selecting a tonne of elements and checking className.

Go for it.

#6 @peterwilsoncc
5 years ago

  • Description modified (diff)
  • Summary changed from Ensure WP oEmbed JavaScript runs error free to Add fallabacks for WP oEmbed JavaScript & CSS

There is some additional feature detection & error checking I need to do to expand browser support. I will upload refresh Sat or Sunday evening USA time.

@melchoyce are you able to upload some PNG fallbacks for the icons?

I've modified the the description and title to reflect browser support rather than specific JS support.

@melchoyce
5 years ago

#7 @melchoyce
5 years ago

Added comment + share icons in embed-icons.zip. Forgot about the X, so I'll upload that next.

@melchoyce
5 years ago

#9 @SergeyBiryukov
5 years ago

  • Summary changed from Add fallabacks for WP oEmbed JavaScript & CSS to Add fallbacks for WP oEmbed JavaScript & CSS

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


5 years ago

@peterwilsoncc
5 years ago

#11 @peterwilsoncc
5 years ago

  • Keywords needs-refresh removed
  • Type changed from enhancement to defect (bug)

Switching the type to bug as the JS was throwing errors in supported browsers.

Refreshed patch added in 34204.2.diff:

  • embed.zip new images to be extracted to the directory src/wp-includes/images/embed/
  • in the iframe:
    • cross browser events - dialog and tabs work back to IE7
    • removed use strict, this in the event methods generates warnings in jshint.
    • feature detection for focus & select
    • the dialog uses IDs and tags for getting all elements required for opening/closing the dialog
    • inputs use querySelectorAll and does not add the focus/select events in browsers without support
    • styles work back to IE7 but IE7 is ugly. IE8 is surprisingly good.
  • in the embedding JavaScript (window.top):
    • JS support only available in browsers supporting querySelectorAll and modern events.

#12 follow-up: @adamsilverstein
5 years ago

@peterwilsoncc I tested this with Browserstack, IE8:

Did I miss anything? Share should work in ie8, right?

#13 @ocean90
5 years ago

Can't we just show text ("x Comments", "Share", "Close") if the browser doesn't support SVGs?

#14 @wonderboymusic
5 years ago

In 35083:

oEmbed: add fallback icons (not used yet) for older browsers.

Props melchoyce.
See #34204.

#15 in reply to: ↑ 12 @peterwilsoncc
5 years ago

Replying to adamsilverstein:

Did I miss anything? Share should work in ie8, right?

I'll do some further testing to see what I missed. Cross-browser events and a generic selector might need to be added to the front end too.

If this increases the JS too much then we can feature detect and hide the share icons by expanding on 34204.1.diff.

#16 @peterwilsoncc
5 years ago

Ok, so hands up if your forgot the entire point of the security attribute was to prevent JS running in older browsers :)

Revised approach:

  • share icon only displays if JavaScript is running in the iframe
  • security policy locks out browsers without querySelector/modern events so reverted to feature test in patch one
  • css fallbacks still in play for the browser impaired

@ocean90

There's not a reliable feature detect available, see Modernizr discussion , but it an approach worth considering. Inline SVG with text fallbacks may be the ticket.

#17 @swissspidy
5 years ago

  • Keywords needs-refresh added

Thanks for digging into all this!

+1 for only showing the share icon if JavaScript is working.

Some minor notes:

  • wp-oembed-embed-ie.css is missing from your latest patch
  • Can you move wp_enqueue_style( 'open-sans' ); to wp_oembed_enqueue_scripts too?
  • no-js class should always be removed (or better: renamed to js, regardless of featuredBrowser
  • Is featuredBrowser even needed? It's always true since the JS isn't run in IE8 and lower.

#18 @peterwilsoncc
5 years ago

  • Keywords needs-refresh removed

Apologies for the missed files, refreshed in 34204.4.diff.

The no-js/js classes indicates whether the browser passes the feature tests & can add events for the popup. It should only change if the features pass.

Perhaps the answer is better class names:

  • queryselector-(un)supported
  • w3cevents-(un)supported

Please make suggestions, naming things is hard.

~

featuredBrowser is required in the event a visitor hits the embed HTML directly, eg http://wordpress-develop.dev/?p=1&embed=true

~

Enqueueing open-sans moved.

~

If this is committed prior to #34272, #34272 will require a refresh.

Last edited 5 years ago by peterwilsoncc (previous) (diff)

#19 @swissspidy
5 years ago

Ah, I didn't see you were already replacing no-js with js. I think that should stay, adding the sharing icon when the class is js. So that's fine.

What I'd change now:

  • Move the no-js / js class to the body class, like in the rest of core
  • Rename featuredBrowser to something like supportedBrowser.
  • Hook print_emoji_detection_script, print_oembed_embed_scripts et al to the new wp_oembed_enqueue_scripts hook.

I can do the refresh once these tickets get in.

#20 @peterwilsoncc
5 years ago

Refresh following [35235] and the following changes in 34204.5.diff.

  • featuredBrowser => supportedBrowser
  • enqueue scripts hook renamed enqueue_embed_scripts to match new convention
  • Moved the print_emoji_styles hook to print_embed_styles
  • print_emoji_detection_script kept on embed_head to follow convention from wp_head
  • in both twentyfifteen and -sixteen the (no-)js classes are on the HTML element so I have left it there. I can't see any other instances of use on the front end.

Note: there was a bug created following #34272 so the 34272.2.diff:ticket:34272 will need to be applied/committed for this patch to work.

@swissspidy
5 years ago

#21 follow-up: @swissspidy
5 years ago

There's no print_embed_styles hook.

print_emoji_styles is already hooked to wp_print_styles so the styles are already loaded.
34204.diff correctly hooks enqueue_embed_scripts to embed_head instead of oembed_head.
Also, now that I'm working on it, we should leave the other hooks as-is.

Thanks for the info re: the no-js class. In the admin it's added to the body, but I think here it's fine on the html embed.

#22 in reply to: ↑ 21 @peterwilsoncc
5 years ago

Replying to swissspidy:

There's no print_embed_styles hook.

print_emoji_styles is already hooked to wp_print_styles so the styles are already loaded.
34204.diff correctly hooks enqueue_embed_scripts to embed_head instead of oembed_head.

Thanks, missed both of these.

Looks like there is both print_embed_styles L451 and wp_print_styles L454 on the embed_head hook. Should this be the case or should one be removed?

Also, now that I'm working on it, we should leave the other hooks as-is.

Thanks for the info re: the no-js class. In the admin it's added to the body, but I think here it's fine on the html embed.

Cool, now I see where you were coming from. Ta.

#23 @swissspidy
5 years ago

print_embed_styles is more like the equivalent to print_emoji_styles, not wp_print_styles. Hooking print_embed_styles to wp_print_styles would be possible, but change the order in which the CSS is loaded.

I prefer the default CSS to be loaded before any other CSS, as it currently is the case.

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


5 years ago

#25 @swissspidy
5 years ago

  • Keywords commit added
  • Owner set to johnbillion
  • Status changed from new to assigned

If nothing's holding us back here, I'd like to get this in.

#26 @peterwilsoncc
5 years ago

Note: #34451 will require a refresh if this is committed first.

Is there anything else this ticket needs to cover before being committed?

@pento
5 years ago

#27 follow-up: @pento
5 years ago

  • Keywords 2nd-opinion added; commit removed

34204.6.diff updates the patch to apply cleanly against trunk.

I'm not particularly excited about the * IE7 hack being in wp-embed-template.css, I think browser hacks should really be kept to conditionally loaded files.

@helen: Do you have opinions on this?

#28 in reply to: ↑ 27 @swissspidy
5 years ago

Replying to pento:

34204.6.diff updates the patch to apply cleanly against trunk.

I'm not particularly excited about the * IE7 hack being in wp-embed-template.css, I think browser hacks should really be kept to conditionally loaded files.

Note: In that patch the * hack is now in both CSS files. Moving it to wp-embed-template-ie.css makes sense, if that hack is really needed. wp-includes/dashicons.css doesn't have these hacks in it.

This ticket was mentioned in Slack in #design by swissspidy. View the logs.


5 years ago

#30 follow-up: @helen
5 years ago

The short answer is no, don't keep them anywhere at all, but I'd like to know what those hacks are there for, exactly, please.

#31 in reply to: ↑ 30 @peterwilsoncc
5 years ago

Replying to helen:

The short answer is no, don't keep them anywhere at all, but I'd like to know what those hacks are there for, exactly, please.

The CSS fallbacks are less of an issue now the embed template in themes is blessed, refer #34278.

The lte ie8 CSS is there to provide a fallback for the SVG elements

The * hacks are there to cover a number of issues:

  • IE7 does not support data-uris, using the * hack avoids the extra HTTP requests for IE8.
  • display:table, etc is not supported in IE7- so the site icon, site name and icons were stacked
  • without the * hacks, IE7 does not display the reference to comments in the inline-block

I'm happy to discard the * hacks or to move them all to the IE file.

@pento
5 years ago

#32 @pento
5 years ago

  • Keywords needs-refresh added; 2nd-opinion removed

34204.7.diff refreshes the patch against trunk.

@peterwilsoncc - could you try moving all of the * hacks to the IE file? it's easier to judge which are worth keeping if they're all in one place.

#33 @peterwilsoncc
5 years ago

  • Keywords needs-refresh removed

@pento moved to the IE file in 34204.8.diff, other notes:

  • the cssmin upgrade deletes * hacks, added an option to prevent.
  • At some point .js .dashicons-share was set to block, it should be inline-block so this has changed.
  • Most of the * hacks are to emulate inline-block in IE7
  • .wp-embed { *width: auto; } is to account for the lack of border-box support in IE7
  • and the png fallbacks this ticket started with

#34 @peterwilsoncc
5 years ago

As discussed, 34204.9.diff is the * hacks be gone edition.

Width definition is removed from wp-embed as it was benign without an explicit width set on the body.

#35 @pento
5 years ago

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

In 35466:

Embeds: Add fallbacks for IE7-9.

Older IE versions need just that little bit of extra tender care to keep them going.

Props peterwilsoncc, swissspidy, pento.

Fixes #34204.

#36 @pento
5 years ago

In 35467:

Embeds: Revert [35083], as the PNG files ended up not being used in [35466].

See #34204.

Note: See TracTickets for help on using tickets.