1513600 - textTrackList disappear when hovering the video, making it impossible to select a track

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

**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.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Tim, looks like you worked on Big 1493525, could you have a look at this? Thanks!

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks for filing.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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...

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1513600 - Use elementFromPoint() to measure isMouseOverVideo r=jaws

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.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cfae697ea6a09d1bb2f3acd4d36cc2416ca17cb

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10aa1d024484
Use elementFromPoint() to measure isMouseOverVideo r=jaws

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/10aa1d024484

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is there any way to write an automated test for this regression? Also, please nominate this for Beta approval when you get a chance.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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:

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

[Oana Botisan, Desktop QA]

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.

Comment 13

[Bogdan Maris, Desktop QA]

Let's leave qe-verify+ until it will be verified on 65 beta as well.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd09f113cb8a

Comment 16

[Oana Botisan, Desktop QA]

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.