Closed Bug 1069223 Opened 5 years ago Closed 5 years ago

Controls are not labelled for the screen reader

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
2.2 S6 (20feb)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: MarcoZ, Assigned: bugzilla)

References

Details

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

Attachments

(1 file)

They all only say "link" or something like that. This also goes for the controls at the top, not just the bottom ones.
Awesome, let me know if you have questions about the controls that are not labelled. You should be able to play around with the app without the screen reader and then see what best makes sense.
Assignee: nobody → ross.ziegler
Status: NEW → ASSIGNED
Sorry, I accidentally logged in with my Persona account.  Could you reassign to this one (bugzilla@imross.com)?

I'll take a look!
np
Assignee: ross.ziegler → bugzilla
Attachment #8556740 - Flags: feedback?(yzenevich)
Okay, so the pull request is attached above.  

About what I did:

I ended up adding "button" roles and l10n id's to the "favorite", "speaker", "seek up", "seek down", and "on/off" controls, and I used the "slider" role on the container listening for swipes on the frequency dialer, making sure to update its "aria-valuenow" value every time it's affected.  My understanding is that the "slider" role isn't normative, so I'm unsure if that was the right move or not, but please take a look and let me know.
Comment on attachment 8556740 [details] [review]
Proposed accessibility changes to FM Radio app

Looks really good, Ross! I added comments in Github to make it even more polished. One thing that's missing (in addition to comments in Github) is the mode when we added some stations to favourites. '.fav-list' will need to be defined as a listbox and its entries should be options. We should also track aria-selected when the station is on and it's on the list. Overall this is looking really nicely. Please flip the feedback? flag back to me when you want me to take a look again!
Attachment #8556740 - Flags: feedback?(yzenevich)
Hey Yura,

I went in and addressed most everything you mentioned with the exception of tracking the 'wheel' events while in accessibility mode.  I'm not sure what your test environment looks like, but for whatever reason on my Flame phone, I'm having the toughest time getting the wheel events to consistently fire while swiping on the screen.  Debugging this behavior is proving to be quite difficult at the moment because I'm not even really sure what to expect from the event.  A few questions about that...

- I'm noticing that when I swipe with one finger, the event never fires at all.  I can only seem to get it to fire with 2 fingers.  Is that the expected behavior?

- My understanding for wheel events is that they can return 3 types of position data: 'line', 'pixel', and 'page'.  I'm not sure how the accessibility wheel events are geared up, but the event only ever seems to return values of '-1' or '+1' (swiping down vs. swiping up).  So with that in mind, is there a particular strategy for calculating 'momentum' to determine how many 'ticks' to advance the frequency dialer on a swipe?  I feel like I might be missing something here.

- More generally, is there any good documentation for the Screen Reader as to how it functions, what events are exposed, etc. that I can read?  I think a technical overview might be helpful. 

Let me know if there's anything else on the PR that needs a second look!
Flags: needinfo?(yzenevich)
Will look at your pull request, but for now some quick comments inline:

(In reply to Ross from comment #7)
> Hey Yura,
> 
> I went in and addressed most everything you mentioned with the exception of
> tracking the 'wheel' events while in accessibility mode.  I'm not sure what
> your test environment looks like, but for whatever reason on my Flame phone,
> I'm having the toughest time getting the wheel events to consistently fire
> while swiping on the screen.  Debugging this behavior is proving to be quite
> difficult at the moment because I'm not even really sure what to expect from
> the event.  A few questions about that...

Capturing wheel events is affected by the element that you attach the event to as well as the currently focused (by screen reader) element. That element's event handler will only get the event if the currently focused element is within its subtree. (hope that makes sense :)


> 
> - I'm noticing that when I swipe with one finger, the event never fires at
> all.  I can only seem to get it to fire with 2 fingers.  Is that the
> expected behavior?

Yep, they only fire with 2 finger swiptes. Single finger swipes are used for quick navigation if it's setup.

> 
> - My understanding for wheel events is that they can return 3 types of
> position data: 'line', 'pixel', and 'page'.  

Yep we use 'page' for screen reader exclusively.

> I'm not sure how the
> accessibility wheel events are geared up, but the event only ever seems to
> return values of '-1' or '+1' (swiping down vs. swiping up).  So with that
> in mind, is there a particular strategy for calculating 'momentum' to
> determine how many 'ticks' to advance the frequency dialer on a swipe?  I
> feel like I might be missing something here.

Yes this is intentional, our swiping effect is constant with no relation to the speed of the swipe.

> 
> - More generally, is there any good documentation for the Screen Reader as
> to how it functions, what events are exposed, etc. that I can read?  I think
> a technical overview might be helpful. 

I'm afraid there's no technical overview at the moment, but I will take care of that soon. There's however user overview:
https://wiki.mozilla.org/Accessibility/Mobile/ScreenReader

In case you feel adventurous and want to see the actual implementation, you can find it here:
https://github.com/mozilla/gecko-dev/tree/master/accessible/jsat

> 
> Let me know if there's anything else on the PR that needs a second look!
Hi Ross, so I looked the pull request and it is pretty great! I tested on device and as you might know everything works but the slider.
I looked a little more into how we handle those cases and it looks like we fire keypress up and down events on the element when we swipe with a single finger. Which means you can try adding listeners for the dialer container. You should be able to capture keypress events on it when swiping up or down with different keycodes: KeyEvent.DOM_VK_DOWN or KeyEvent.DOM_VK_UP. Some examples:
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/pagbar.js#L62-L75 <- is a good one
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/value_selector/value_picker.js#L269-L275

Flag me to take a look again whenever you need me to take a look, thanks!
Flags: needinfo?(yzenevich)
Hey Yura,

I updated the PR again with fixes for the comments you left on GitHub, as well as adding an implementation for the slider using the wheel event.  It seems to work "okay" on my device, but you need to have some pretty spectacular precision with your fingers to catch that dialer every time, and I'm seemingly only able to get the event to fire using a pretty rapid swipe speed.  I'm not sure if this is related to the touch/wheel events conflicting somehow or if there's something else that I can do to improve that but please take a look and let me know what you think.  My device seems to be intermittently unresponsive in general use, so it could just be my hardware.

Otherwise, I didn't mess around with the keyboard events because I was able to get the wheel event working.  Is a single-finger swipe something you'd like to see implemented here as well?

And finally, side note:  I feel like I'm getting inconsistent output in the console from WebIDE and I'm not sure if it's related to any settings on my end or if it's just how things are working right now.  To give example, I spent a pretty good chunk of time trying to figure out why a certain object property was giving me a NaN value at runtime, and it turned out to be because the property didn't exist (due to a typo).  I'm used to seeing non-existent object properties show up as errors in the console in typical settings, but in this case, there was no error at all.  Is there any way to enable that output aside from the "Console Enabled" setting from the Developer Menu on the phone (I already have that enabled)?
Flags: needinfo?(yzenevich)
I looked at your pull request and it looks good, see some comments inline. But we are really close!

(In reply to Ross from comment #10)
> Hey Yura,
> 
> I updated the PR again with fixes for the comments you left on GitHub, as
> well as adding an implementation for the slider using the wheel event.  It
> seems to work "okay" on my device, but you need to have some pretty
> spectacular precision with your fingers to catch that dialer every time, and
> I'm seemingly only able to get the event to fire using a pretty rapid swipe
> speed.  I'm not sure if this is related to the touch/wheel events
> conflicting somehow or if there's something else that I can do to improve
> that but please take a look and let me know what you think.  My device seems
> to be intermittently unresponsive in general use, so it could just be my
> hardware.

Yeah I think it's probably a combination of hardware and our gesture detection. It might have to be polished a little bit more down the road.

> 
> Otherwise, I didn't mess around with the keyboard events because I was able
> to get the wheel event working.  Is a single-finger swipe something you'd
> like to see implemented here as well?

I think, yeah.. My initial suggestion to use wheel events was incorrect. I forgot about the one finger mode of interaction with sliders (that translates into the keyboard up and down events). That would be the right and consistent approach with the rest of the UI. The handler you have is very similar to what the keyboard one would be so. Sorry to make you do the extra bit. In terms of wheel events we use them mostly for interfaces where the role is not indicative of the interaction. So for example a utility tray that you can swipe down from the status bar on top is triggered by the wheel event, etc.

> 
> And finally, side note:  I feel like I'm getting inconsistent output in the
> console from WebIDE and I'm not sure if it's related to any settings on my
> end or if it's just how things are working right now.  To give example, I
> spent a pretty good chunk of time trying to figure out why a certain object
> property was giving me a NaN value at runtime, and it turned out to be
> because the property didn't exist (due to a typo).  I'm used to seeing
> non-existent object properties show up as errors in the console in typical
> settings, but in this case, there was no error at all.  Is there any way to
> enable that output aside from the "Console Enabled" setting from the
> Developer Menu on the phone (I already have that enabled)?

I'm afraid this one is an implementation detail and some of the inconsistencies you are seeing are part of the ongoing active development process of the WebIDE. I am not sure if there are any other settings that you might find useful, but I'd played around with some of them in the about:config when you search for webide...
Flags: needinfo?(yzenevich)
Okay, sounds good.  Thanks for the info! :)

Updated the PR with the keypress event taking the place of the wheel event.
Flags: needinfo?(yzenevich)
Comment on attachment 8556740 [details] [review]
Proposed accessibility changes to FM Radio app

This looks really good, it works really well too. Just a couple of nits in the Pull request. 

Also start considering adding the unit tests similar to the other bug you worked on. I think you can just add them into already existing https://github.com/mozilla-b2g/gaia/blob/master/apps/fm/test/unit/fm_test.js

Mark me for the final a11y-review then but otherwise great job!
Flags: needinfo?(yzenevich)
Attachment #8556740 - Flags: feedback+
Hey Yura,

Went through and addressed the nits and additionally added all but one of the unit tests, but I'm again a little unsure if I did things correctly and I was hoping you could take a look and give me some feedback.  A few of the points I was a little unsure of...

- I ended up just fitting in the tests where they seemed MOST appropriate given the file structure, but in doing so, I also had to ammend some of the tempNodes to include more markup.  I know we avoided that route altogether in the last Bug by just including the body, but I tried to work with what I had here and keep code additions to a minimum.  Is that the way to go for this test file or should I be grouping all of the Accessibility changes into their own suite and/or doing something else?

- For the screen-reader keypress events, I went in and actually created new Events and then artificially fired them on the dialer-container to simulate that functionality.  Is that the proper way to go about that or are there better event simulation techniques that I should be implementing?

- Finally, I was unable to test the speaker-switch "aria-pressed" attribute maintenance because the SpeakerManager object is called inside an init() function that itself is called by what looks to be the AirplaneModeHelper, and I'm not totally positive how that's all supposed to come together. Should I be loading in the AirplaneModeHelper here or calling init() directly in a new suite?  Advice would be much appreciated.

Thanks!
Flags: needinfo?(yzenevich)
Also, on the nit regarding the change from the Switch Statement to the If/Else, I was just curious why If/Else is the better choice.
(In reply to Ross from comment #15)
> Also, on the nit regarding the change from the Switch Statement to the
> If/Else, I was just curious why If/Else is the better choice.

I find switch more bug prone since one can fall through the condition if break is forgotten for example. Personally, I try to stick to crockford's "javascript good parts", however, switch use is fairly common in Gaia, primarily, when there are a million cases within event handling and all that.
Flags: needinfo?(yzenevich)
(In reply to Ross from comment #14)
> Hey Yura,
> 
> - I ended up just fitting in the tests where they seemed MOST appropriate
> given the file structure, but in doing so, I also had to ammend some of the
> tempNodes to include more markup.  I know we avoided that route altogether
> in the last Bug by just including the body, but I tried to work with what I
> had here and keep code additions to a minimum.  Is that the way to go for
> this test file or should I be grouping all of the Accessibility changes into
> their own suite and/or doing something else?

What you did looks right. Since the test original authors went with mock markup, it generally is fine to extend it for your tests.
Also if you'd like you can group all accessibility tests under accessibility suite:
something like this: 
suite('accessibility', function() {/* your tests go here */});
(In reply to Ross from comment #14)
> - For the screen-reader keypress events, I went in and actually created new
> Events and then artificially fired them on the dialer-container to simulate
> that functionality.  Is that the proper way to go about that or are there
> better event simulation techniques that I should be implementing?

This is a right approach, thanks!
(In reply to Ross from comment #14)

> - Finally, I was unable to test the speaker-switch "aria-pressed" attribute
> maintenance because the SpeakerManager object is called inside an init()
> function that itself is called by what looks to be the AirplaneModeHelper,
> and I'm not totally positive how that's all supposed to come together.
> Should I be loading in the AirplaneModeHelper here or calling init()
> directly in a new suite?  Advice would be much appreciated.

So it looks like the last test suite is actually calling init(), and it looks like it all gets set up with the mock speaker manager. A test case like this should work there:

test('test speaker switch accessibility', function() {
  var speakerSwitch = $('speaker-switch');
  assert.equal(speakerSwitch.getAttribute('aria-pressed'), 'false');
  speakerSwitch.click();
  assert.equal(speakerSwitch.getAttribute('aria-pressed'), 'true');
});
Comment on attachment 8556740 [details] [review]
Proposed accessibility changes to FM Radio app

Great work! I added some comments on Github. Cheers.
Okay, looks like I got everything taken care of in the new PR.  One thing I did want to ask was about Sinon stubs.  It seems that once you stub a method using Sinon, it stays stubbed for the entirety of the test file (it's not restored after the suite ends).  Is that okay or should I be trying to restore those stubs in teardown somehow?
Flags: needinfo?(yzenevich)
(In reply to Ross from comment #22)
> Okay, looks like I got everything taken care of in the new PR.  One thing I
> did want to ask was about Sinon stubs.  It seems that once you stub a method
> using Sinon, it stays stubbed for the entirety of the test file (it's not
> restored after the suite ends).  Is that okay or should I be trying to
> restore those stubs in teardown somehow?

From what I can tell from the pull request you use stub in suiteSetup for a specific test suite, so outside of it, the method will be be stubbed. That looks correct.
Flags: needinfo?(yzenevich)
Comment on attachment 8556740 [details] [review]
Proposed accessibility changes to FM Radio app

Great. a11y-review+ for me with nits addressed. Could you then mark Tim or Pin from https://wiki.mozilla.org/Modules/FirefoxOS#FM_Radio for review?

Thanks again!
Attachment #8556740 - Flags: feedback+ → a11y-review+
Attachment #8556740 - Flags: review?(timdream)
Okay, nits addressed!  If you have another bug in mind for me to work on, let me know! :)
Attachment #8556740 - Flags: review?(timdream) → review+
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/27794

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
Flags: needinfo?(bugzilla)
Hi Ross, while we wait for Gaia to be reopened, I was wondering if you have a profile on https://mozillians.org/ yet. If not let me know if [want to] make one and I could vouch for you.
In fact you should get an invite :)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8556740 [details] [review]
Proposed accessibility changes to FM Radio app

[Approval Request Comment] This PR makes most of FM Radio app accessible.
[Bug caused by] (feature/regressing bug #): improvement, not abug
[User impact] if declined: Screen reader users will not have an accessible FM app.
[Testing completed]: on device + unit tests
[Risk to taking this patch] (and alternatives if risky): fairly small, mostly accessibility related DOM
[String changes made]: https://github.com/mozilla-b2g/gaia/pull/27794/files#diff-8dcb961a453e52175a94d80926347945
Attachment #8556740 - Flags: approval-gaia-v2.2?
Attachment #8556740 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.