Closed Bug 1068912 Opened 10 years ago Closed 9 years ago

[Camera] Play button in video preview is not labeled

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master verified)

VERIFIED FIXED
Tracking Status
b2g-master --- verified

People

(Reporter: eeejay, Assigned: yzen)

References

Details

(Keywords: access, Whiteboard: [b2ga11y p=1])

Attachments

(2 files)

The center button is not labeled.
Summary: Play button in video preview is not labeled → [Camera] Play button in video preview is not labeled
Depends on: 1068998
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Attached file Github pull request.
This Pull request depends on the one from bug 1068909
Attachment #8575480 - Flags: review?(jdarcangelo)
Comment on attachment 8575480 [details] [review]
Github pull request.

Camera-related code in this patch looks good. Adding David to review the video_player.js code since I believe he is the owner.

David: Only the 2nd commit in this PR is related to this bug. Thanks!
Attachment #8575480 - Flags: review?(jdarcangelo)
Attachment #8575480 - Flags: review?(dflanagan)
Attachment #8575480 - Flags: review+
I don't understand this patch well enough to review, and need to ask questions first

1) What does the CSS transition stuff have to do with accessibility?

2) I thought Eitan recently patched video_player.js so that the control buttons did have aria labels. Now this patch is marking the whole thing as aria-hidden, which seems like it would undo Eitan's work.

Eitan or Yura: could one of you explain these things before I review?
Flags: needinfo?(yzenevich)
Flags: needinfo?(eitan)
Hi David, some answers inline:

(In reply to David Flanagan [:djf] from comment #3)
> I don't understand this patch well enough to review, and need to ask
> questions first
> 
> 1) What does the CSS transition stuff have to do with accessibility?

So the controls for the video player, when off screen, should also be hidden from the screen reader. With the original styling, that was not the case, because they were hidden with opacity: 0. The screen reader would still be able to step into them which is not desirable. So our approach was to also add visibility: hidden/visible in places where we have opacity 0/1 respectively. 

We would be done there, but in cases where opacity is handled with transitions it is not enough. We do not want to immediately apply visibility because then we will loose the transition. Thus we have a visibility transition handled with delay, where it's set to hidden only after opacity transition is done, and, in the other case, set to visible when opacity transition starts from 0 to 1. Hopefully this explanation helps.

> 
> 2) I thought Eitan recently patched video_player.js so that the control
> buttons did have aria labels. Now this patch is marking the whole thing as
> aria-hidden, which seems like it would undo Eitan's work.

The change in the video player is complementary to the work Eitan has done. With his changes we have the right roles and labels present and that's great. The change I'm introducing just hides the video element from the screen reader. The reason why is because it is only useful if the app/website uses browser default video controls. If that's the case, our accessibility module can parse the build controls and add support for things like keyboard navigation. However, in cases where the controls are reimplemented (such as this implementation of the VideoPlayer) it is only presented as an unlabelled graphic element. This is fairly confusing, since it's not operable nor meaningful for the screen reader user so I opted for hiding it all together.

> 
> Eitan or Yura: could one of you explain these things before I review?
Flags: needinfo?(yzenevich)
Flags: needinfo?(eitan)
Comment on attachment 8575480 [details] [review]
Github pull request.

r- because I think the change that removes the styles for .videoPlayerControls is likely to break something. Either leave that style in, or convince me that it is harmless to remove it.

Also, consider making the other VideoPlayer.css changes to the gallery app as well. It has its own copy of this same file.
Attachment #8575480 - Flags: review?(dflanagan) → review-
(In reply to David Flanagan [:djf] from comment #5)
> Comment on attachment 8575480 [details] [review]
> Github pull request.
> 
> r- because I think the change that removes the styles for
> .videoPlayerControls is likely to break something. Either leave that style
> in, or convince me that it is harmless to remove it.
> 
> Also, consider making the other VideoPlayer.css changes to the gallery app
> as well. It has its own copy of this same file.

Ahh, good catch about the toggle control, sorry I missed that. The reason I wanted to remove the styling that stretches the video controls container across the whole player is because in case, for example, where MediaFrame is used, screen reader user will never be able to interact with the MediaFrame.

The difference with the screen reader is that there's no concept of empty space that the toggle controls functionality utilizes. Screen reader always needs to focus on something to be able to activate it. I played around with implementation a little bit and if I were to enable tapping on the video element itself, the effect would be the same. In addition to that, screen reader will be able to access it. Would that work for you?
Flags: needinfo?(dflanagan)
Comment on attachment 8575480 [details] [review]
Github pull request.

Fixed the issue around controls toggling, also updated gallery app video player styling.
Attachment #8575480 - Flags: review- → review?(dflanagan)
Hi David, I was wondering if you would have a moment to take a look at this one. I was hoping to land this before the March 19th cutoff? Thanks!
Flags: needinfo?(dflanagan)
Flags: needinfo?(dflanagan)
I remember having a lot of trouble with the video player getting the events just right for hiding and showing controls. I don't remember what the specific issues were. So theoretically your change seems fine, but based on my previous experience with this module, I'm very worried about introducing regressions.

So even if I could make time to complete a review today, there is no way I would support the uplift of this patch to 2.2 at this late stage. 

Sorry I didn't write the code to be screen-reader friendly in the first place!
Flags: needinfo?(dflanagan)
Comment on attachment 8575480 [details] [review]
Github pull request.

Finally I've been able to make the time to really understand this patch, and I'm satisfied that it works as it should. If we were not planning to replace this code with a web component, I'd ask you to remove the controls element completely from the JS code, but for now I think it is okay like this.

Note that I did have a couple of nits about comments in the camera code.
Attachment #8575480 - Flags: review?(dflanagan) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This bug has been verified as "pass" on latest Nightly build of Flame v3.0.

STR:
1.Enable Screen Reader in Settings app.
2.Go to Camera app and record a video, then tap the "Preview" icon to preview the video.
3.Tap the play/pause button.
**Both the "Play" and "Pause" button are labeled by the voice "play button"/"pause button".

See attachment: verified_v3.0.3gp
Reproduce rate: 0/10


Device: Flame v3.0 build(Verified)
Build ID               20150622160206
Gaia Revision          311c4e59936a407e64509f54fecb440d8a78e3c8
Gaia Date              2015-06-20 20:21:42
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/be81b8d6fae9
Gecko Version          41.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150622.193834
Firmware Date          Mon Jun 22 19:38:45 EDT 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
Attached video verified_v3.0.3gp
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: