1132191 - Improve accessibility of the video app.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

There are several controls that are inaccessible or not labelled for the screen reader.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1132191 > mozilla-b2g:master

Comment 2

[Russ Nicoletti [:russn]]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

Hi Yura,

Thanks for the very thorough PR.

I'm giving r-, mostly because I have some questions about the use of the existing video-container element and the new hide-controls element. I'd like to understand the need for those changes before approving.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

Hi Russ,
I think I got all your comments/concerns addressed. I ended up with the original markup (e.g. wihtout introducing the hide-button element). The only thing I had to do is to switch the order video container, header and footers, so the markup is consistent with presentation (Similar to the previous PR). Hopefully that works for you, thanks!

Comment 4

[Russ Nicoletti [:russn]]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

Hi Yura, the changes look good. I have put some questions in the PR. 

There is one issue that I see that needs to be resolved. When toggling the video controls when the video does not fill the entire screen, the background -- not covered by the video -- flashes blue. This can be seen by changing the orientation of the device to landscape when viewing a video taking in portrait orientation or vice-versa.

I'm going to leave the review r? until this is resolved.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

Ok, I updated the pull request and it fixed the issue when video container was getting :active background change.

Comment 6

[Russ Nicoletti [:russn]]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

Looks good, thanks for the changes. I have one last comment in the PR regarding 'video-controls-bottom'.

Comment 7

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/2d3efff1544e49f05521495cebc3b8bcc0066380

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8567264 [details] [review]
[gaia] yzen:bug-1132191 > mozilla-b2g:master

[Approval Request Comment] This PR makes video app awesome for the screen reader
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: if declined the video app will stay inaccessible to the screen reader
[Testing completed]: unit + on device
[Risk to taking this patch] (and alternatives if risky): some markup and css changes across the app
[String changes made]: https://github.com/mozilla-b2g/gaia/pull/28342/files#diff-55698edbdb20279da9494aafdf4c3d48

Comment 9

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/2ae86d204d68872b31492d5084c37ba2f0606dc0