1478484 - Click and drag no longer works with media controls for audio files

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

STR:
- Navigate to a audio file that will be played in the browser. For example: http://media.ca7.uscourts.gov/sound/external/me.17-3300.17-3300_07_06_2018.mp3
- Attempt to click and drag the volume or playback position
- The position will be updated upon clicking, but cannot be dragged.

Expected result:
- Both volume and playback position should be draggable.

This appears to be a regression caused by bug 1474574.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

:Gijs, could you please recommend someone to take a look at this, as I see :timdream, who worked on bug 1474574, is on PTO?

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1478484 - avoid focusing the video element if we get focus events for the controls,

Review commit: https://reviewboard.mozilla.org/r/259686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259686/

Comment 3

[:Gijs (he/him)]

The issue is that the focus capturing code ends up capturing and redirecting focus when the scrubber gets focus on click, breaking the click-and-drag motion. Fairly simple patch, though I don't see how we could easily automated test this given it involves dragging the scrubber and we don't have good test framework infra for doing that, to my knowledge.

Comment 4

[:Gijs (he/him)]

(Thanks for filing this, btw - I also happened to notice bug 1478599 when investigating this. :-) )

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8995194 [details]
Bug 1478484 - avoid focusing the video element if we get focus events for the controls,

https://reviewboard.mozilla.org/r/259686/#review266690

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/642f7c9b2c48
avoid focusing the video element if we get focus events for the controls, r=jaws

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/642f7c9b2c48

Comment 8

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

I didn’t check the DOM spec but I would imagine calling focus() on focused element should be a no-op. If so we are leaking implementation detail here. I’ll investigate when I am back.

Comment 9

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) (PTO Jul 21-) from comment #8)
> I didn’t check the DOM spec but I would imagine calling focus() on focused
> element should be a no-op. If so we are leaking implementation detail here.
> I’ll investigate when I am back.

It's an interesting question, esp. with Shadow DOM - but it seems somewhat intuitive to me that if I have:

A (focusable)
|- B (focusable, anon)

and the user focuses B, that fires a focus event, and because we don't expose the anon content, the exposed target on the event is (A).

It also seems reasonable to me that under those circumstances, calling A.focus() after B takes focus will take focus away from B again - otherwise, how is a page supposed to do that?

Of course, the combination of these two effects is confusing and I have no idea what the spec says about this.

While I would agree that we're leaking the fact that the user focuses controls, I'm not convinced that's something we should worry about, as long as the event doesn't expose the control nodes to web content in any way.

Brian, do you have any further thoughts about this, and/or how this might impact our plans to switch to web components?

Comment 10

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

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #9)

The specs do not care about anonymous content in any way, given that they are Gecko-specific implementation details.

videocontrols is expected to be migrated to Shadow DOM-based UA Widget in bug 1431255. Shadow DOM somehow has its own class of different behavior. Here is a little test case:

data:text/html,<div tabindex="-1" id="outer"></div><div tabindex="-1" id="another">another focusable</div><script>onload = () => { d = document.body.firstChild; d.attachShadow({ mode: 'closed' }).innerHTML = '<input id="input">'; document.addEventListener('focus', e => { console.log(e.target.id); d.focus()}, true); }</script>

( The DOM looks like this:

<body> (focus-able as document)
  <div tabindex="-1"> (focus-able)
    shadow root
      <input> (focus-able)
  <div tabindex="-1" id="another">
    #text another focusable
)

What I found was:

1) If the focused element is *not* the <div> and the user clicks on the <input>, the a focus event is dispatched on the document with evt.target == <div>, and calling div.focus() will prevent the user from focus on the input. This is similar to our bug.
2) If the focused element is the <div> and the user clicks on the <input>, no focus event is dispatched on the document.

The patch in bug 1431255 will behave correctly even without the patch here because of (2), I think. This is good because this means we leak impl detail less, not more.

I would worry that the effect of (1) will be visible in Shadow DOM-based <input type="date">, but it's not worse than the current XBL impl so perhaps it is not actionable.

Comment 11

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

This needs to be in beta since bug 1474574 was uplifted.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Jared, can you please nominate this for approval since Gijs is on PTO?

Comment 13

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

Comment on attachment 8995194 [details]
Bug 1478484 - avoid focusing the video element if we get focus events for the controls,

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1474574
[User impact if declined]: As described in the summary
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No
[Why is the change risky/not risky?]: The change is local and should not break other things.
[String changes made/needed]: None.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8995194 [details]
Bug 1478484 - avoid focusing the video element if we get focus events for the controls,

Fix for regression in 62, let's uplift for beta 16.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/87a0f14ada20

Comment 16

[Gyula Palko[:gpalko]]

Verified, that the issue is no longer reproducible, media controls are draggable on Nightly 63.0a1(20180809220138) and Beta 62.0b16(20180809104529).
Environments covered: Win10 x64, Ubuntu 16.04 and MacOS 10.13.