1608923 - PIP feature in Firefox: The PIP video buttons don't announce to NVDA/screen reader

Status: VERIFIED FIXED

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

Description

[Dwellaz]

Attachment: NVDA_FF_PIP_DD.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.88 Safari/537.36

Steps to reproduce:

I have a custom html5 video player that has audio, therefore, the new PIP button appears on hover.

In Firefox + Windows 10, turn on NVDA screen reader. I also use highlighter plugin on NVDA to see where focus moves, but you do you.

Hover the video and invoke the PIP.
PIP launches the video in focusTrap popup.
Tab onto this video.
Notice that NVDA focuses each video player button, but only announces "Button". Each time you tab to another button, NVDA says "Button".
The user doesn't know if they are on play, close, etc.

Actual results:

Using NVDA on a PIP video, the video player buttons do not announce. They only say "button"

Expected results:

Each button on the video player should announce its name to the screen reader. For example, if play button has NVDA focus, it should announce "Play"

A screen reader user would not know which button they need to use to control video playback or close the PIP video unless the buttons announce their proper names.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[:Gijs (out for now; he/him)]

All of these buttons:

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/toolkit/components/pictureinpicture/content/player.xhtml#27,30,32,34

probably want an aria-label attribute with a text description of what the button does. For the play/pause button, we'll want to update that when the video plays/pauses.

If we want to use FTL here I don't think we can fix this for 73. If we're using DTD we may be able to reuse the videocontrols strings, though that's messy for the play/pause button and live updates.

Comment 3

[:Gijs (out for now; he/him)]

Attachment: Bug 1608923 - reuse some DTD strings so at least play/pause and the mute/unmute button are labeled, r?mconley

Comment 4

[:Gijs (out for now; he/him)]

Attachment: Bug 1608923 - use fluent for PiP button accessible labels, r?mconley

Depends on D60199

Comment 5

[:Gijs (out for now; he/him)]

Hello mheubusch; we need a text label/descritpion (to be used for accessibility software to label the button) for the [x] and the PiP button that both close the PiP window and "pop the video back in" to the page where it came from. Could you suggest what to use? Something like "Close and re-select tab", or "Move video back into webpage" or something along those lines perhaps...

Comment 6

[:Gijs (out for now; he/him)]

Hm, in fact, let's split this bug.

Comment 7

[:Gijs (out for now; he/him)]

Let's move the close button stuff to bug 1610334.

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/cf65ab95f911
https://hg.mozilla.org/mozilla-central/rev/c714bda799cf

Comment 9

[:Gijs (out for now; he/him)]

Comment on attachment 9121432 [details]
Bug 1608923 - reuse some DTD strings so at least play/pause and the mute/unmute button are labeled, r?mconley

Beta/Release Uplift Approval Request

• User impact if declined: No AT/screenreader support on a new feature
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This reuses existing strings to add some labels to some of the buttons in the PiP window where possible. Given anything else requires new strings, this is our best bet for beta.

Note: please do NOT uplift the second commit/patch.

• String changes made/needed: Not in this patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9121432 [details]
Bug 1608923 - reuse some DTD strings so at least play/pause and the mute/unmute button are labeled, r?mconley

Improves screen reader support for the PiP feature. Approved for 73.0b8. Note that only this patch should be uplifted to Beta.

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c95299ef20e

Comment 12

[Hani Yacoub, Desktop QA]

I tried to verify this bug on Windows 10 x64 using Firefox Nightly 74.0a1 (2020-01-22) and only "Play" and "Mute" buttons are announced while using screen reader, "X" and "PIP" buttons are not.
I'm not sure if this is the expected behaviour or not, could you please help us out?

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Hani Yacoub from comment #12)

I tried to verify this bug on Windows 10 x64 using Firefox Nightly 74.0a1 (2020-01-22) and only "Play" and "Mute" buttons are announced while using screen reader, "X" and "PIP" buttons are not.
I'm not sure if this is the expected behaviour or not, could you please help us out?

Yes, it's expected, I filed bug 1610334 for the [X] and PIP buttons, see comment #7.

Comment 14

[Hani Yacoub, Desktop QA]

My bad, thanks for the clarification.
Marking this as verified asa fixed on Firefox Nightly 74.0a1 (2020-01-22)

Comment 15

[Hani Yacoub, Desktop QA]

One more question, is it expected that on Beta while PIP mode is activated the "Volume" button is not displayed?

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Hani Yacoub from comment #15)

One more question, is it expected that on Beta while PIP mode is activated the "Volume" button is not displayed?

Yes, https://searchfox.org/mozilla-central/rev/cfd1cc461f1efe0d66c2fdc17c024a203d5a2fd8/browser/app/profile/firefox.js#1498-1503 .

Comment 17

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 73.0b8.