WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#31058 closed defect (bug) (fixed)

audio video previews JS errors in IE 8

Reported by: afercia Owned by: wonderboymusic
Milestone: 4.1.1 Priority: normal
Severity: normal Version: 4.1
Component: Media Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

Noticed a couple of JS errors when browsing prev/next in the attachment details modal using Internet Explorer 8: images are fine but as soon as you next (or previous) items is a video or audio, the preview is not rendered and the prev/next navigation fails.

It looks like the first error is caused by this line in media-audiovideo.js

t.media.remove();

where t.media seems to me it's the DOM object and maybe it should be the t.$media jQuery object instead? See proposed patch.

About the second JS error, to be honest I gave up but seems it's from mediaelement-and-player.min.js.

See errors details below.

Message: Object doesn't support this property or method
Line: 56
Char: 5
Code: 0
/wp-includes/js/media-audiovideo.js?ver=4.2-alpha-31007-src

Message: 'parentNode.tagName' is null or not an object
Line: 15
Char: 17054
Code: 0
/wp-includes/js/mediaelement/mediaelement-and-player.min.js?ver=2.16.2

https://cldup.com/5KiNOyCd7r.png

Attachments (5)

31058.patch (401 bytes) - added by afercia 5 years ago.
mediaelement-and-player.js (151.2 KB) - added by afercia 5 years ago.
mediaelement-and-player.js.diff (911 bytes) - added by dd32 5 years ago.
Previous js file in diff form.
31058.monkey-patch.diff (1.2 KB) - added by dd32 5 years ago.
31058.document-fragment.patch (1.2 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (22)

@afercia
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 4.1.1

#2 @dd32
5 years ago

  • Keywords has-patch added

Duplicated in IE8, as expected, replacing with t.$media.remove() seems appropriate to me, jQuery uses t.media.parentNode.removeChild( t.media ); which is the correct cross-browser remove method.

For reference for those confused, ChildNode.remove() browser compabitility is relatively recent, and not actually supported by IE.

#3 @wonderboymusic
5 years ago

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

In 31386:

Avoid an IE8 JS error when removing a MediaElement player.

Props afercia.
Fixes #31058.

#4 @wonderboymusic
5 years ago

In 31387:

After [31386], this needs to happen in audio-video.manifest.js.

See #31058.

#5 @dd32
5 years ago

In 31395:

Avoid an IE8 JS error when removing a MediaElement player.

Props afercia.
Merges [31386] to the 4.1 branch.
Fixes #31058.

#6 @dd32
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Turns out this was still broken in IE8 when Flash was also installed, re-opening for fixing.

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


5 years ago

#8 @afercia
5 years ago

Hacked the mediaelement-and-player.js non-minified version downloaded from GitHub, you can compare the original:
https://github.com/johndyer/mediaelement/blob/master/build/mediaelement-and-player.js
with the one attached, see mejs.HtmlMediaElementShim.createPlugin
Not 100% sure, of course it should be reviewed by someone more familiar with mediaelementjs than me, but seems it works.

@dd32
5 years ago

Previous js file in diff form.

#9 @dd32
5 years ago

mediaelement-and-player.js.diff is attachment:mediaelement-and-player.js in diff form against the github original (You can't apply this patch directly to WordPress's minified files)

Although this fixes it, I question why the MediaElement site itself works without this change, which suggests to me this could be caused somehow by how we're using it.

#10 follow-up: @dd32
5 years ago

Seems like the fix (mediaelement-and-player.js.diff) is correct, the problem is due to us instantiating mediaelement on <audio|video> tags that aren't yet part of the main DOM, this is something that we should open a PR upstream for.
In Chrome (if I force it down the flash route) it hits this case but node.parentNode is null (instead of IE, where it's a node with no real context).

The Mediaelementjs.com and front-end views aren't affected, as the elements are within the page already, so it can easily find the <body> element and break out of the loop.

Since I didn't really like the idea of shipping a new/patched mediaelement mediaelement-and-player.min.js file in 4.1.1, and we've already modified media-audiovideo.js, i've monkey patched it in 31058.monkey-patch.diff so that it thinks that the root-level element in the virtual dom is a <body> tag (I couldn't simply unset/delete/set to null, as IE didn't seem to want to let me).
This tests fine for me in IE8, and although I can't really see this causing any issues, I've not thoroughly tested it against other browsers/versions.
I'd appreciate it if others could weigh in on this, and if these fixes hit the right spots or not.

#11 in reply to: ↑ 10 @afercia
5 years ago

Replying to dd32:

the problem is due to us instantiating mediaelement on <audio|video> tags that aren't yet part of the main DOM, this is something that we should open a PR upstream for.
In Chrome (if I force it down the flash route) it hits this case but node.parentNode is null (instead of IE, where it's a node with no real context).

@dd32 thanks very much for looking into this. I think you're right, noticed that Internet Explorer 8 while looking for parent nodes, at a certain point returns a DocumentFragment, see screenshot (note: parentNode.tagName returns undefined but parentNode.nodeName returns #document-fragment)

https://cldup.com/qrzzEfBOIc.png

https://developer.mozilla.org/en/docs/Web/API/Node.parentNode
Document and DocumentFragment nodes can never have a parent, so parentNode will always return null.

Also Chrome doesn't reach the body: after 6 parent DIVs (same as IE) it stops but returns null.

To my understanding, this means MediaElementPlayer is instantiated too early, before browsers have the time to insert the new elements into the DOM tree. Tried to delay a few milliseconds the instantiation of MediaElementPlayer and now both IE and Chrome can reach the body, see screenshot:

https://cldup.com/agtF_hoJf3.png

Attached experimental patch, not sure this is the way to go would greatly appreciate if others more expert than me could have a look here. Please notice in the patch I've set the timeout to "1" for testing purposes but maybe should be 10 ?

#12 @dd32
5 years ago

I think the reason the delay works, because by that time the render() has bubbled up the chain, and it's then been inserted into the DOM.
I had a half-written comment along the lines of "we could delay creating the elements until after it's inserted into the DOM", but the fact that the MediaElement loop is written to expect either a root <body> or a null parentNode signals that it's been patched against this issue, just not in a way that works with older IE, which is why I didn't consider attempting to delay the creation.

#13 @dd32
5 years ago

Upstream PR: https://github.com/johndyer/mediaelement/pull/1423

For 4.1.1 it seems the setTimeout() method is preferred, and should work reliably from what I can tell from talking to others.
For 4.2 we should be able to update to the latest MediaElement.js (assuming that is merged), but in the meantime I guess we can use the same workaround.

#14 @dd32
5 years ago

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

In 31444:

Fix the display of Audio and Video in the Media Library when using IE8 and below.

This delays the execution of instantiating of the MediaElement.js player until the render is complete, and the node is in the DOM.
Although it's a bug that this is needed in the first place, this will cover us until a new MediaElement.ks release is made.

Props afercia.
Fixes #31058 for 4.1.

#15 @dd32
5 years ago

  • Keywords fixed-minor added
  • Resolution fixed deleted
  • Status changed from closed to reopened

re-opening for 4.2 fixes.

#17 @dd32
5 years ago

  • Keywords fixed-minor removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Marking as fixed for 4.1.1 & 4.2, as per @wonderboymusic, Thanks!

Note: See TracTickets for help on using tickets.