1068867 - [Camera] Bottom buttons are unlabeled for screen reader

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

The issues:
 - Preview button (graphic) on left is unlabeled.
 - Camera shutter button is unlabeled.
 - Camera/video switch is unlabeled and inoperable.
 - Video record button is unlabeled and inoperable.

Comment 1

[Eitan Isaacson [:eeejay]]

Also, the center shutter/record button accessible bounds should be bigger.

Comment 2

[Eitan Isaacson [:eeejay]]

Moved camera/video switch operability to bug 1068868.

Comment 3

[Alexey Novak]

I would love to work on this bug if it is still available.

Comment 4

[Eitan Isaacson [:eeejay]]

Indeed! I'm assigning it to you.

Comment 5

[Alexey Novak]

Could you quickly mention the strategy to resolve this bug?

In my previous resolved issues I was using combination of data-l10n-id and aria-label to properly annotate interactive elements. Was wondering if I need to do the same here?

Maybe except of preview button since I saw that there is a dynamically added test-thumbnail img into the HTML element. I will try to see if alt attribute would be enough for this one.

Comment 6

[Alexey Novak]

Attachment: First attempt to resolve the bug

Hi Eitan,

Have to apologize for such a weak pull request. I was trying to find any good attributes for the screen reader to read those controls. But it seems that those buttons are DIVs and also dynamically generated through control.js file.

Looks like screen reader reads out data-icon attributes of those divs. There are really 2 solutions for this problem (as I can see so far).
1) rename all data-icons to something more readable and expressive
2) add ariaLabel attribute for those divs

Merge request is 2nd solution. The only problem that data-icon are still read by a screen reader so that you have something similar to "Camera Camera Button" (for the camera button example).

Also looks like both |mode-switch_bg-icon| and |mode-switch_current-icon| both got a selection by a screen reader. Currently I added ariaLabel for both... not sure what exactly has to be focused here by the screen reader. On one hand you want to read out possible options, on another you want to read out what is currently selected as well.

I also added |alt| attribute to the thumbnail picture so it is read to the user properly. But screen reader adds word "graphic" to it as well. So it reads "Photo Thumbnail graphic" to the user.

Any suggestions or ideas on this pull request?

Also I'm not entirely sure how I can resolve this task. Any hints or suggestions?
> Also, the center shutter/record button accessible bounds should be bigger.

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Alexey Novak from comment #6)
> Created attachment 8508315 [details] [review]
> First attempt to resolve the bug
> 
> Hi Eitan,
> 
> Have to apologize for such a weak pull request. I was trying to find any
> good attributes for the screen reader to read those controls. But it seems
> that those buttons are DIVs and also dynamically generated through
> control.js file.
> 
> Looks like screen reader reads out data-icon attributes of those divs. There
> are really 2 solutions for this problem (as I can see so far).
> 1) rename all data-icons to something more readable and expressive

That would not be a good path forward.

> 2) add ariaLabel attribute for those divs

This is right. In addition to aria labels, you need to add roles.

> 
> Merge request is 2nd solution. The only problem that data-icon are still
> read by a screen reader so that you have something similar to "Camera Camera
> Button" (for the camera button example).
> 
> Also looks like both |mode-switch_bg-icon| and |mode-switch_current-icon|
> both got a selection by a screen reader. Currently I added ariaLabel for
> both... not sure what exactly has to be focused here by the screen reader.
> On one hand you want to read out possible options, on another you want to
> read out what is currently selected as well.

This is the more challenging control to work with. It requires some creative thinking. There are a few options:
1. Make the whole switch one button, and simply change the label, depending on which mode is selected.
2. Make the switch a radiogroup with a label of "mode", and the child camera/video items get a role of radio. This is tough, because the markup and styling here looks difficult to work with.

I think option two is preferable, but it won't be easy. You could find examples how to implement radiogroup/radio here:
http://oaa-accessibility.org/example/31/

> 
> I also added |alt| attribute to the thumbnail picture so it is read to the
> user properly. But screen reader adds word "graphic" to it as well. So it
> reads "Photo Thumbnail graphic" to the user.

Yes, the problem here is that you need to assign the thumbnail a role of "button". I think both the role, and the aria-label, should be on the parent element in this case. The general rule should be that the role goes on the element with the actual click listener.

> 
> Any suggestions or ideas on this pull request?

The center button needs the following:
1. A label and role=button on '.capture-button' element because:
 (a) it has the click listener.
 (b) it is geometrically larger and better for explore by touch.
2. The label should not be "Camera button", the button role will take care of describing the type of control, so you only need to label the button.
3. The label should not be "Camera" because that does not explain to the user what the button *does*, it should probably be "shutter", or "take picture".
4. When in video mode, the button changes to a record button, and when you press it it turns into a stop button. The label needs to be updated for each case.

> 
> Also I'm not entirely sure how I can resolve this task. Any hints or
> suggestions?

I hope that is helpful :)

Comment 8

[Alexey Novak]

This is VERY helpful. Thanks so much Eitan. I will start looking into this immediately and will let you know if I have any extra questions.

Comment 9

[Alexey Novak]

I'm sorry for such a late reply. I was away for the last 3 days and now coming back on this bug.

Managed to make work properly a thumbnail button and a camera button.
Although not entirely sure how to change the aria-label on the camera button: (Camera, Start Video, Stop Video)
I found that there is |setMode()| function in controllers/camera.js and views/controls.js I left a comment in my pull request. It kinda makes sense to set the Camera/Start Video labels using |app.l10nGet| inside of views/controls.js whenever user switch Camera to Video. However I'm not sure when the label should be changed for a Stop Video button when the video started recording. My Nexus S errors out on me when I start recording so I had to search everywhere in the code to find what triggers UI change but with no results.

The closest I could get was 
- |onRecordingChange()| in controllers/controls.js
- |startRecording()| in lib/camera/camera.js and there is an event fired |self.emit('willrecord')|

Still trying to search for the proper place but so far with no results. I would appreciate any hints.

p.s.
Radio button I have not started yet since wanted to figure out the rest of the controls.

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Alexey Novak from comment #9)
> I'm sorry for such a late reply. I was away for the last 3 days and now
> coming back on this bug.
> 
> Managed to make work properly a thumbnail button and a camera button.
> Although not entirely sure how to change the aria-label on the camera
> button: (Camera, Start Video, Stop Video)

I would name it Shutter, Start recording, Stop recording.

> I found that there is |setMode()| function in controllers/camera.js and
> views/controls.js I left a comment in my pull request. It kinda makes sense
> to set the Camera/Start Video labels using |app.l10nGet| inside of
> views/controls.js whenever user switch Camera to Video. >

This is the correct approach. The label should be set in setMode().

> However I'm not sure
> when the label should be changed for a Stop Video button when the video
> started recording. My Nexus S errors out on me when I start recording so I
> had to search everywhere in the code to find what triggers UI change but
> with no results.
> 
> The closest I could get was 
> - |onRecordingChange()| in controllers/controls.js

This is the right codepath, I would introduce a method in the view similar to setMode(), where it would call set('recording',..) on itself, and assign a new aria-label.

Comment 11

[Alexey Novak]

I made changes to use proper aria-labels as well as took over recording state.
Although I wonder if I should use |l10nGet| which reads labels from a property file. Most of the controllers have access to this from |app.l10nGet| but in view there is no |app|.
Is it possible and should I use it?

I also started inserting a radiogroup for a switch. I'm not entirely convinced by it.
- For one user can only focus on one radio button at a time
- Imagine default state is Camera Mode radio button. If user press action, phone will focus on Video Mode radio button. If user press action, phone will focus on toggle-camera-rear (maybe it should go back on Camera Mode radio button?)

and I'm still looking into how to refactor HTML. It works with radiogroup now which is great. But screen reader still reads out divs with class |mode-switch_bg-icon|.
Was thinking that maybe to go the least amount of changes (which is not always the best way to do things) and just make divs with class |mode-switch_bg-icon| not accessible by a screen reader. Basically treat them as background images for the radiogroup switch. Tried to use |tabindex="-1"| but that did not work for me. Is it the right direction to look or it should be a complete redesign of HTML and CSS for the switch?

Comment 12

[Alexey Novak]

Another question. I do push partial changes into that pull request which is great because my changes are visible for everyone but at the same time I do not think it is good to spawn Gaia Try Hook for such pushes. Do I need to stop those whenever I have such "non-complete solution" push ?

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to Alexey Novak from comment #11)
> I made changes to use proper aria-labels as well as took over recording
> state.
> Although I wonder if I should use |l10nGet| which reads labels from a
> property file. Most of the controllers have access to this from
> |app.l10nGet| but in view there is no |app|.
> Is it possible and should I use it?
> 

Yes, these labels need to be localized. You could do it in 2 ways:
var l10n = navigator.mozL10n;
this.els.capture.setAttribute('aria-label', l10n.get(recording ? 'capture-button-start-video' : 'capture-button-start-video'));

Or:
 this.els.capture.dataset.l10nId = recording ? 'capture-button-start-video' : 'capture-button-start-video';

> I also started inserting a radiogroup for a switch. I'm not entirely
> convinced by it.
> - For one user can only focus on one radio button at a time
> - Imagine default state is Camera Mode radio button. If user press action,
> phone will focus on Video Mode radio button. If user press action, phone
> will focus on toggle-camera-rear (maybe it should go back on Camera Mode
> radio button?)

I'm not entirely sure what you mean here? The cursor jumps?

> 
> and I'm still looking into how to refactor HTML. It works with radiogroup
> now which is great. But screen reader still reads out divs with class
> |mode-switch_bg-icon|.
> Was thinking that maybe to go the least amount of changes (which is not
> always the best way to do things) and just make divs with class
> |mode-switch_bg-icon| not accessible by a screen reader. Basically treat
> them as background images for the radiogroup switch. Tried to use
> |tabindex="-1"| but that did not work for me. Is it the right direction to
> look or it should be a complete redesign of HTML and CSS for the switch?

tabindex will have no effect here, try using role=presentation, it may be more helpful.

Comment 14

[Eitan Isaacson [:eeejay]]

I just realized that we already have a bug open for the mode switch, bug 1068868. So I would separate that into a pull request over there.

Comment 15

[Alexey Novak]

So I removed code related to the switch and will be putting it into another branch and another pull request.

Please let me know if I need to change anything. The only problem I found that calling l10n.get() will break Camera App UI in control.js 
I commented out the line. It is a bit weird because the video ariaLabel works like a charm. My suspicion that |setMode| function is called during some early stages when the OS starts and that causes some issues.
I'm not entirely sure how to go around it so I hardcoded "Shutter" label for now.

Also I assume that I will need to write some tests for the pull request. What tests do we expect?
- by default Camera button should have a proper label "Shutter"
- when shooting mode changes then Camera label should change
- when camera enters and exits recording state then label should change
- when photo is made then thumbnail button should have a proper label

Not entirely sure what test file I should modify. So far I found the following ones:
- apps/camera/test/marionette/lib/camera.js
- apps/camera/test/unit/controllers/camera_test.js
- apps/camera/test/unit/controllers/camera_controls.js
- apps/camera/test/capture_test.js

Comment 16

[Eitan Isaacson [:eeejay]]

(In reply to Alexey Novak from comment #15)
> So I removed code related to the switch and will be putting it into another
> branch and another pull request.
> 
> Please let me know if I need to change anything. The only problem I found
> that calling l10n.get() will break Camera App UI in control.js 
> I commented out the line. It is a bit weird because the video ariaLabel
> works like a charm. My suspicion that |setMode| function is called during
> some early stages when the OS starts and that causes some issues.
> I'm not entirely sure how to go around it so I hardcoded "Shutter" label for
> now.

You could simply assign dataset.l10nId directly and not have a dependency on l10n.js.

> 
> Also I assume that I will need to write some tests for the pull request.
> What tests do we expect?
> - by default Camera button should have a proper label "Shutter"
> - when shooting mode changes then Camera label should change
> - when camera enters and exits recording state then label should change
> - when photo is made then thumbnail button should have a proper label

Yup, all those cases look right.

> 
> Not entirely sure what test file I should modify. So far I found the
> following ones:
> - apps/camera/test/marionette/lib/camera.js
> - apps/camera/test/unit/controllers/camera_test.js
> - apps/camera/test/unit/controllers/camera_controls.js
> - apps/camera/test/capture_test.js

Yeah, I would modify current tests to check for the right label. All this should be reviewed and verified with the module peer. For larger cases, you could make a python gaia-ui test, but if there are already tests that bring the camera into all the needed states, I would just add assertions there.

Comment 17

[Alexey Novak]

I'm sorry but I am only aware of how to run python tests.
How would I run those .js tests? There is one marionette and other 3 are "non-marionette" ?

I'm a bit confused here since new to such test files in Gaia.

Comment 18

[Alexey Novak]

So after reading on JS Unit tests I was trying to run them but it was not working at all. So then I had to update Gaia and Nightly and tests started to work. But now the switch Camera->Video does not work for a screen reader (since I wanted to test if my code still works after updating the code). Ugh... probably this need to be addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=1068868 and then I need to come back to this bug.

Comment 19

[Eitan Isaacson [:eeejay]]

Please rebase this, and ask for review (in the attachment details page set 'review' to '?', and select a person from the 'suggested reviewers' drop-down to do the review).

Comment 20

[Eitan Isaacson [:eeejay]]

Comment on attachment 8508315 [details] [review]
First attempt to resolve the bug

I am not a camera module owner, so I am forwarding the review to somebody who is.

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8508315 [details] [review]
First attempt to resolve the bug

Over to the apps team.

Comment 22

[Wilson Page [:wilsonpage]]

Comment on attachment 8508315 [details] [review]
First attempt to resolve the bug

Some nits and questions.

Comment 23

[Alexey Novak]

I tried to merge the branch with 1068868 and see how it works and it looked positive.
But the problem with mozL10n persists, it is always undefined. But at the same time if I hardcode a11ylabel to something like "Shutter" and then |setMode| is called later, |navigator.mozL10n| is not undefined anymore.

I think that it is async load. Tried to look everywhere else for examples but could not find anything suitable or similar to my problem. Any advices on how to overcome this problem ?

Comment 24

[Eitan Isaacson [:eeejay]]

(In reply to Alexey Novak from comment #23)
> I tried to merge the branch with 1068868 and see how it works and it looked
> positive.
> But the problem with mozL10n persists, it is always undefined. But at the
> same time if I hardcode a11ylabel to something like "Shutter" and then
> |setMode| is called later, |navigator.mozL10n| is not undefined anymore.
> 
> I think that it is async load. Tried to look everywhere else for examples
> but could not find anything suitable or similar to my problem. Any advices
> on how to overcome this problem ?

Indeed. There is a race condition with l10n loading.
I think the trick is to not set the label directly, but to set dataset.l10nId instead.
then the strings in the locale file should have the ariaLabel attribute. Example:

capture-button-stop-video.ariaLabel = Stop Recording

Comment 25

[Alexey Novak]

I'm still waiting for 1068868 to be merged in. But I made a separate branch which has a fix for this bug which is on top of it.
You can see a change here https://github.com/anvk/gaia/compare/1068868_7?expand=1

Comment 26

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

After chatting with Alex, I'll take this bug to wrap it up by 19th.

Comment 27

[GitHub Autolander Bot]

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

Comment 28

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

Hi Justin, 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!

Comment 29

[Justin D'Arcangelo [:justindarc]]

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

LGTM. Sorry for the delay in this review. Thanks for the patch!

Comment 30

[GitHub Autolander Bot]

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