Closed
Bug 1513600
Opened 6 years ago
Closed 6 years ago
textTrackList disappear when hovering the video, making it impossible to select a track
Categories
(Toolkit :: Video/Audio Controls, defect, P1)
Toolkit
Video/Audio Controls
Tracking
()
VERIFIED
FIXED
mozilla66
People
(Reporter: nchevobbe, Assigned: timdream)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
**Steps to reproduce**
1. Open http://html5doctor.com/demos/webvtt/
2. Play the video (important, the bug only appears when the video is playing)
3. Click the closedCaptionButton to make the list of track appears (there should be 2 items: Off and an empty one
4. Try to click on "Off" (or the other one, it doesn't matter)
**Expected results**
I can click the button
**Actual results**
As soon as I move the mouse, the popup disappear, making it impossible to to select a track.
---
Ran mozregression against this: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4ff0fcd61c7a1f791414516a6e22efca94530f19&tochange=d8dca67e244037cefcee992c48b1ef1f0fc7293c
Seems that Bug 1493525 introduced the bug.
Reporter | ||
Comment 1•6 years ago
|
||
Tim, looks like you worked on Big 1493525, could you have a look at this? Thanks!
Flags: needinfo?(timdream)
Assignee | ||
Comment 2•6 years ago
|
||
Thanks for filing.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
status-firefox64:
--- → disabled
status-firefox65:
--- → affected
status-firefox66:
--- → affected
tracking-firefox65:
--- → ?
Flags: needinfo?(timdream)
Assignee | ||
Comment 3•6 years ago
|
||
I am not entirely sure the regression range is correct -- removing the relevant call path doesn't fix this bug.
Actually -- revert videocontrols.js along all the way to when it is first checked in still won't fix this bug.
I think the UA Widget is receiving different event properties or whatnot. Will continue working on this but it will not be related to bug 1493525.
Assignee | ||
Comment 4•6 years ago
|
||
There is more than 1 bug that causes this regression. The first one is indeed bug 1493525 -- where checkEventWithin() tried to check the DOM references passed from the mouseover/mouseout event with this.videocontrols, which is replaced by the JS Proxy instance in bug 1493525.
The second bug is yet to be identified. It looks like within UA Widget, the |relatedTarget| of the mouse event is always <video> even if the mouse is moved from one Shadow DOM element to another.
That doesn't happen on a vanilla Shadow DOM, as this won't reproduce:
data:text/html,<script>onload = () => { document.body.attachShadow({mode: 'open'}).innerHTML = '<style>span {border: 1px solid}</style><span>sibling</span><span>hover me</span>'; document.body.shadowRoot.lastElementChild.addEventListener("mouseover", evt => console.log(evt.relatedTarget, evt.relatedTarget.textContent))}</script>
I am not sure if it is worthy to identify the second bug, because even if I did, we might not want to correct the behavior.
Also, mouseenter/mouseleave never fires on UA Widget Shadow DOM elements, so that won't work.
Perhaps I can simplify everything instead...
Assignee | ||
Comment 5•6 years ago
|
||
The checkEventWithin method is broken by two bugs:
The first one is bug 1493525 because we ended up pass the proxy instance, instead of the element reference, as the parent node to compare.
The second one is unknown and happened sometime after that bug. The |relatedTarget| of the mouse event is always <video>, instead of the element within Shadow DOM that the cursor is moving out to.
Instead of identify the second bug in the DOM, this patch employs a simpler fix by using elementFromPoint() to identify the cursor position.
Updated•6 years ago
|
Keywords: regression
Priority: -- → P1
Assignee | ||
Comment 6•6 years ago
|
||
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10aa1d024484
Use elementFromPoint() to measure isMouseOverVideo r=jaws
Comment 8•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment 9•6 years ago
|
||
Is there any way to write an automated test for this regression? Also, please nominate this for Beta approval when you get a chance.
status-firefox-esr60:
--- → unaffected
tracking-firefox66:
--- → +
Flags: qe-verify+
Flags: needinfo?(timdream)
Flags: in-testsuite?
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 9030924 [details]
Bug 1513600 - Use elementFromPoint() to measure isMouseOverVideo r=jaws
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: Bug 1493525
User impact if declined: The text track list will not be usable.
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: No
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): The function changed is only used in this one place alone so the patch should not cause other regressions.
String changes made/needed:
Flags: needinfo?(timdream)
Attachment #9030924 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Is there any way to write an automated test for this regression?
It does possible to write an automated test for this, let me file a follow-up.
Comment 12•6 years ago
|
||
I managed to reproduce the issue using an older version of Nightly (2018-12-12) on Windows 10 x64.
I verified the fix using latest Nightly 66.0a1 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is not reproducing anymore.
Flags: qe-verify+
Comment 13•6 years ago
|
||
Let's leave qe-verify+ until it will be verified on 65 beta as well.
Flags: qe-verify+
Comment 14•6 years ago
|
||
Comment on attachment 9030924 [details]
Bug 1513600 - Use elementFromPoint() to measure isMouseOverVideo r=jaws
[Triage Comment]
Fixes an unusable text track list. Approved for 65.0b6.
Attachment #9030924 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•6 years ago
|
||
bugherder uplift |
Comment 16•6 years ago
|
||
I verified the fix on beta 65.ob6 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64 and the issue is not reproducing anymore.
You need to log in
before you can comment on or make changes to this bug.
Description
•