Opened 10 years ago
Closed 10 years ago
#31058 closed defect (bug) (fixed)
audio video previews JS errors in IE 8
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 4.1.1 | Priority: | normal |
| Severity: | normal | Version: | 4.1 |
| Component: | Media | Keywords: | has-patch |
| Focuses: | javascript | Cc: |
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
Attachments (5)
Change History (22)
#3
@
10 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from new to closed
In 31386:
#6
@
10 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.
10 years ago
#8
@
10 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.
#9
@
10 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:
↓ 11
@
10 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
@
10 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 butnode.parentNodeis 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://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:
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
@
10 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
@
10 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.
Duplicated in IE8, as expected, replacing with
t.$media.remove()seems appropriate to me, jQuery usest.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.