Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Description • 10 years ago

Both rating and share bars need to have a better screen reader ux. 

The hide on timeout needs to be handled more gracefully.
All controls should have proper labels and be operable.

	Ross Assignee
	Comment 1 • 10 years ago

Hi Yura,

I've taken a look at the HTML markup for both the "Caption/Share bar" and the "Star Rating bar" in the Music app, and came up with some preliminary revisions that I'm thinking will improve the accessibility UX for each.  Please take a look at the attachment and let me know what you think.

In response to gracefully hiding both bars on timeout, would it be permissible to set aria-hidden="true" on the parent element of each bar and then reset aria-hidden="false" once the bars are brought back into view?

Also, fyi, this is my first Mozilla bugfix attempt so please let me know if I'm doing anything wrong and/or there are better ways to go about communicating.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 2 • 10 years ago

Hi Ross, assigning this to you. This is a good start!

(In reply to Ross from comment #1)
> Created attachment 8525795 [details]
> Initial HTML Markup Suggestion
> 
> Hi Yura,
> 
> I've taken a look at the HTML markup for both the "Caption/Share bar" and
> the "Star Rating bar" in the Music app, and came up with some preliminary
> revisions that I'm thinking will improve the accessibility UX for each. 
> Please take a look at the attachment and let me know what you think.

I ll reply in the next comment.

> 
> In response to gracefully hiding both bars on timeout, would it be
> permissible to set aria-hidden="true" on the parent element of each bar and
> then reset aria-hidden="false" once the bars are brought back into view?

I think the best way is to avoid using aria-hidden (especially for elements that are completely hidden to the sighted user as well). Here I would check what happens on timeout. If the visibility is handled with CSS visibility or display properties, it should be all that we need. What we actually need here is to tell the user that something is shown or hidden on the screen, especially if the screen reader focus is somewhere else in the app. I would suggest considering using aria-live [1] for the containers that are shown/hidden dynamically (alternatively you can focus on those panels if that makes sense from the UX point of view).

> 
> Also, fyi, this is my first Mozilla bugfix attempt so please let me know if
> I'm doing anything wrong and/or there are better ways to go about
> communicating.

In case you are comfortable working with git fell free to work off the branch and then you can post a diff or better a pull request so I could leave comments inline. Alternatively, similar to what you did with this patch, I would suggest attaching it as a file because then I can leave the inline comments in Bugzilla.

[1] https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 3 • 10 years ago

Comment on attachment 8525795 [details]
Initial HTML Markup Suggestion

Overall good use of roles. One thing to be aware of is - aria-checked attribute would need to be handled programmatically. In other words, when the user clicks on one of the star ratings, we need to update the attribute to 'true' ourselves. That will also require a unit test later on.

A couple of comments about aria-labels:
* They need to be less verbose (as short as possible). The prefixes that you have (for example Rating:) could be labels for the container (e.g. radiogroup) so you would not have to repeat them every time.
* All of the aria-labels must be localized. What that means is that instead of actually specifying aria-label attributes you just need to add the appropriate data-l10n-id attributes for those elements. Once you've done that, you would need to add the matching entry in the localization (l10n) .properties file. The l10n framework then will render the attributes from those files automatically. See links to examples from different apps:
** .properties file - https://github.com/yzen/gaia/blob/master/apps/calendar/locales/calendar.en-US.properties#L250-L281
** Matching attributes in HTML - https://github.com/yzen/gaia/blob/master/apps/calendar/elements/settings.html#L11 and so on.

Overall great effort and do not hesitate to ping me. Also if you are adding attachment you can select a feedback ? flag and put my name there. This way it will not escape off my radar.

	Ross Assignee
	Comment 4 • 9 years ago

Hey Yura,

I was away visiting family for awhile so apologies for the delay, but I've since returned home and have everything at my fingertips.  Happy Holidays!

After reading through your comments, I went ahead and forked your branch of Gaia on Git, and generated a Pull Request with a few of the proposed changes.  I'm admittedly not very familiar with Git, but my understanding is that you should be able to see this Pull Request from your account.  Is that right?  If not, the public URL is https://github.com/FunkTron/gaia/compare/yzen:master...master

A few notes/questions:

- In regard to hiding both of the bars on timeout, they're actually not hidden at all using visibility settings in CSS -- they're merely being moved off screen by using position settings (top, bottom).  So in this case, is using "aria-hidden" admissible or should we be going after CSS visibility settings after the bars have been removed from view?

- Is there any documentation for the l10n framework?  I see how labels are defined in the various localization files, but I also see what looks like variable resolution for dynamic content (Ex: in the Calendar app, the dynamic "Start Date" seems to be accessed with {{startDate}} ).  If these are indeed variables, where are they defined?

- I briefly took a look at the javascript files in the Music App to try and take care of any programmatic changes that would need to be made, but stopped short of writing anything because I'm unsure of the structure.  Are there any javascript frameworks or helpful dependencies that Gaia relies on?  Where (which file) should I be looking to include a function that checks for the user's star-rating and updates its "aria-checked" attribute?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 5 • 9 years ago

Hi Ross, just back from the Holidays myself, Happy New Year!
Here are some comments:
Generally labeling sounds pretty good with the screen reader.

(In reply to Ross from comment #4)
> Hey Yura,
> 
> I was away visiting family for awhile so apologies for the delay, but I've
> since returned home and have everything at my fingertips.  Happy Holidays!
> 
> After reading through your comments, I went ahead and forked your branch of
> Gaia on Git, and generated a Pull Request with a few of the proposed
> changes.  I'm admittedly not very familiar with Git, but my understanding is
> that you should be able to see this Pull Request from your account.  Is that
> right?  If not, the public URL is
> https://github.com/FunkTron/gaia/compare/yzen:master...master

Yep I could see it, thanks. Generally it would probably be best to file a pull request against an official Gaia repository (no need to worry that it's just work in progress), I could then leave inline comments and they would stick until the very end. So feel free to do that.

> 
> A few notes/questions:
> 
> - In regard to hiding both of the bars on timeout, they're actually not
> hidden at all using visibility settings in CSS -- they're merely being moved
> off screen by using position settings (top, bottom).  So in this case, is
> using "aria-hidden" admissible or should we be going after CSS visibility
> settings after the bars have been removed from view?

Yes we generally prefer not to use aria-hidden unless absolutely necessary (this creates a special a11y solution where, ideally, we would like to keep it unified with the rest of the UI). So since there's a delay here, we would apply a correct visibility property value using transition (similar and in addition to how it's moved off screen now).

> 
> - Is there any documentation for the l10n framework?  

Yes, this is what our l10n guys suggest looking at: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices

> I see how labels are
> defined in the various localization files, but I also see what looks like
> variable resolution for dynamic content (Ex: in the Calendar app, the
> dynamic "Start Date" seems to be accessed with {{startDate}} ).  If these
> are indeed variables, where are they defined?

Yes so, in addition to data-l10n-id there's also data-l10n-args attribute that contains a stringified JSON of data that will be distributed into a string, for example:

data-l10n-id could be = "test-string" and the data-l10n-args could be "{'test': 'test'}" that will work well with an entry in properties file like this: test-string=this is a {{test}} and will be resolved to 'this is a test'

> 
> - I briefly took a look at the javascript files in the Music App to try and
> take care of any programmatic changes that would need to be made, but
> stopped short of writing anything because I'm unsure of the structure.  Are
> there any javascript frameworks or helpful dependencies that Gaia relies on?

AFAIK, none of the apps use any frameworks so it is plain JavaScript. We do have some shared javascript but it's only mostly used in the system app. All includes are generally listed here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/music/index.html unless module loader is used (which I don't think it is in music app).

All JS lives in https://github.com/mozilla-b2g/gaia/tree/master/apps/music/js
and UI specific stuff is in: https://github.com/mozilla-b2g/gaia/tree/master/apps/music/js/ui

> Where (which file) should I be looking to include a function that checks for
> the user's star-rating and updates its "aria-checked" attribute?

I think setRatings (https://github.com/mozilla-b2g/gaia/blob/master/apps/music/js/ui/views/player_view.js#L305) is called when the rating is being set. I think this is where I'd look.

Please let me know if that makes sense (hopefully it helps) :)

	Ross Assignee
	Comment 7 • 9 years ago

Hey Again!

Your comments were once again very helpful, thanks.

So I followed your advice and initiated the Pull Request against the official Gaia Repo instead of running it against your fork.  I've attached it to this thread above, following the guidelines from here: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch (let me know if I did something wrong).

I think this patch should cover most of what we already talked about, but the remaining considerations I have are:

1) As far as the roles on the rating star labels are concerned, I'm undecided whether we should be going with "radio" or "checkbox".  On the one hand, an album can only logically be rated 1-star OR 2-stars OR 3-stars, etc.  It can't be 1-star AND 2-stars AND 3-stars at the same time.  On the other hand however, the UI doesn't reflect this behavior.  The UI shows both the actual star-rating AND all preceding stars as being highlighted.  It's a nice effect visually, and I think it's pretty understandable, but does that idea apply for Aria?  I currently have the roles declared as "checkbox" but please let me know if that should be changed to "radio"

2) I know you had briefly mentioned unit testing for the javascript code additions.  What's the procedure for that?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 8 • 9 years ago

Comment on attachment 8546340 [details] [review]
Updated Pull Request on Official Gaia repo to include javascript and CSS edits in addition to the previous HTML & localization files

Hi Ross, the PR looks really good. I left several comments, let me know if they make sense.

In terms of the tests, you are correct, we need to be able to test the JS changes - which is going to be the radio button selection/deselection tests. This fits a unit test, I belive, but because there aren't too many, you would need to add a new unit test file in apps/music/test/unit/player_view_test.js (the tests are written with mocha).

A similar example is something you can find in apps/calendar/test/unit/views tests. Where a single view is tested. 

You would need to load the view js file: require('/js/ui/views/player_view.js'); and if necessary add some mock markup.

Among the tests that you should try writing are:
test the setRating itself (https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/js/ui/views/player_view.js#L305), test click handler on the element that has rating in the data set (https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/js/ui/views/player_view.js#L960) and test https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/js/ui/views/player_view.js#L552 with different song data metadata. I realize this is probably a task bigger than the changes themselves but I think it's really worthwhile. So let me know if you get stuck and we can go through it in more detail.

Thanks!

	Ross Assignee
	Comment 10 • 9 years ago

Okay, I've implemented the changes to the PR as noted in your comments on Git.  One thing I'm unsure of is if I've applied the data-l10n-id + data-l10n-args correctly... The changes don't seem to have had an effect on the star elements' aria-labeling.  I tried to append the .ariaLabel notation in the localization file (as we did for 'Rating' and 'Share') on each of the star-rating declarations but that didn't seem to do the trick either.  Let me know if there's something I'm missing with how that's supposed to come together.

As for the unit testing, I think I might need a little more guidance.  I understand that we're using Mocha here as a test framework, but do you know which assertion library we're using?  A few of the test files I've read seem to suggest that Chai is in use, so I tried to write a sample test using the "expect/should" style and I've attached it above.  Does that look okay? I'm admittedly pretty new to unit-testing as a whole, but hopefully I'm not too far off here.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 11 • 9 years ago

Hi Ross,

I looked at your stuff and it works pretty well. aria-label gets populated correctly (I tested it and all). Just add .ariaLabel to the star rating strings, without it l10n framework populates inner text/html.

(In reply to Ross from comment #10)
> Okay, I've implemented the changes to the PR as noted in your comments on
> Git.  One thing I'm unsure of is if I've applied the data-l10n-id +
> data-l10n-args correctly... The changes don't seem to have had an effect on
> the star elements' aria-labeling.  I tried to append the .ariaLabel notation
> in the localization file (as we did for 'Rating' and 'Share') on each of the
> star-rating declarations but that didn't seem to do the trick either.  Let
> me know if there's something I'm missing with how that's supposed to come
> together.
> 
> As for the unit testing, I think I might need a little more guidance.  I
> understand that we're using Mocha here as a test framework, but do you know
> which assertion library we're using? 

Yes assertion library we use, afaik, is chai - http://chaijs.com/api/assert/.

Here is an example of a unit test from the music app: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/test/unit/m3u_test.js

Generally in your case it would be sufficient to load a player view with require, similar to how it's done here: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/test/unit/m3u_test.js#L5

After that similar to the suite setup in the above unit test (https://github.com/mozilla-b2g/gaia/blob/master/apps/music/test/unit/m3u_test.js#L34-L38) you would need to bootstrap the component you test. Since it is a view component, you would need to add some mock markup into the test document (but only what you need really - the rating radio related markup). So you would literally write to document.body's inner html a block of HTML string that corresponds to https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/index.html#L104-L110 . After that, since you loaded the PlayerView object with require, you just need to call its init function to bind the selectors. That's all you need to do in suiteSetup. After that you just need to add a test that calls setRatings with various values and verifying the aria-checked is assigned correctly. That should be all you need to test, since that's the only JS you added.

>                                      A few of the test files I've read seem
> to suggest that Chai is in use, so I tried to write a sample test using the
> "expect/should" style and I've attached it above.  Does that look okay? I'm
> admittedly pretty new to unit-testing as a whole, but hopefully I'm not too
> far off here.

Hope that helps.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 12 • 9 years ago

Comment on attachment 8549127 [details]
Sample unit test for 'setRating' function

See comment above. But the idea is right. No need to check star-on stuff, just aria specific things.

	Ross Assignee
	Comment 13 • 9 years ago

> Hi Ross,
> 
> I looked at your stuff and it works pretty well. aria-label gets populated
> correctly (I tested it and all). Just add .ariaLabel to the star rating
> strings, without it l10n framework populates inner text/html.
> 

Okay, cool.  I updated the localization file with the proper lines for star-rating.

I've also written up a unit test file and added it to the PR (https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/test/unit/player_view_test.js).  Could you take a look at it?  Also, I'm wondering how best I should configure my setup to run the tests myself.  Are there any recommended settings for Mocha and/or Chai and/or Sinon that I should go with to perform these tests locally?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 14 • 9 years ago

(In reply to Ross from comment #13)
> > Hi Ross,
> > 
> > I looked at your stuff and it works pretty well. aria-label gets populated
> > correctly (I tested it and all). Just add .ariaLabel to the star rating
> > strings, without it l10n framework populates inner text/html.
> > 
> 
> Okay, cool.  I updated the localization file with the proper lines for
> star-rating.
> 
> I've also written up a unit test file and added it to the PR
> (https://github.com/FunkTron/gaia/blob/Bug1069630/apps/music/test/unit/
> player_view_test.js).  Could you take a look at it?  Also, I'm wondering how
> best I should configure my setup to run the tests myself.  Are there any
> recommended settings for Mocha and/or Chai and/or Sinon that I should go
> with to perform these tests locally?

Thanks, I'll take a look. In terms of tests, I should've given you this link earlier :) - https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests. You can basically run it locally with your gaia profile and a Firefox nightly instance. So for example, I run unit tests with this command on my mac:
bin/gaia-test /Users/yzenevich/Github/yzen_gaia /Applications/FirefoxNightly.app/Contents/MacOS/firefox

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 15 • 9 years ago

I should've mentioned that we use TDD style with mocha that provides suite(), test(), setup(), and teardown() instead of BDD that has describe(), it(), before(), after(), beforeEach(), and afterEach() (just like the tests in the example).

	Ross Assignee
	Comment 16 • 9 years ago

Hey Yura,

Okay, I think I've got the unit test (sorta) figured out.  I updated the PR again, and the unit tests are actually working and written in the TDD style, but I did some things that might be questionable and I was hoping to get a little more feedback.

1) I ended up having to require '/shared/js/lion.js' to avoid a "navigator.mozL10n undefined" error -- was that the best way to do that?  I had contemplated just overriding the navigator variable (similar to what I did in item #2 below) but was unsure which way to go.

2) On Line 61, I overrode the #setSeekBar() method to avoid having to configure the seek controls in my bootstrapped code.  I wouldn't have ordinarily done this but that particular method is called by #init() and its inclusion adds unnecessary complexity to the test suite setup.  Again, was this okay to do here?  Is there a better way to achieve code isolation in instances like this?

3) I didn't include a suiteTeardown() function because I think I'm still a little unclear as to its purpose in a view context.  Could you clarify how I would use that here?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 17 • 9 years ago

Hi Ross, sorry for the delay.

I looked at the tests and they look great! They test exactly what we need. I left detailed comments regarding you questions in Github so hopefully they help. Once they are addressed I think you can either ping me to take the final look or you can ask for a final review from the module owner (or I can do that on your behalf).

To quickly sum up:
1) You would need to use the mock of l10n instead of the real thing (details on Github).
2) You can use sinon.stub (test support utils) to stub the member with a dummy one (details on Github).
3) You did not really need it for the version you had, however since you would need to put back the real l10n into the navigator object for (1) after using the mock one, that would be done in the suiteTeardown that you'd have to add.

Hope that helps, and thanks again!

	Ross Assignee
	Comment 18 • 9 years ago

Okay great, thanks for clearing all that up.  I've updated the PR, would you like to give it a final review?

I left a note in Github under your comment regarding empty lines/proper formatting but that should do it for this one!

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 19 • 9 years ago

Comment on attachment 8546340 [details] [review]
Updated Pull Request on Official Gaia repo to include javascript and CSS edits in addition to the previous HTML & localization files

Looks great. Marking for review, but please update the trailing spaces and that one nit I mentioned! This is really great!

	Ross Assignee
	Comment 20 • 9 years ago

Okay, sounds good, and I made the nit changes as requested.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 21 • 9 years ago

Hi Ross, once the pull request is reviewed and before it is merged into master we would need to squash all your commits into 1 (with the correct format: 'Bug 1069630 - ...'). Do you know how to squash your commits in the branch? In any case this is what I normally do:
* Use rebase -i for this. So, in the your gaia directory, while you are in your working branch, you can run:
    git rebase -i upstream/master
* At this point, git gives you a way to edit all the commits. I normally 'pick' the first one, then choose 's' for squash for the rest, so the rest of the commits are squashed to the first picked commit.
* You can then modify the commit message as well.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 22 • 9 years ago

^ Make sure if that your working branch is up to date too (e.g. rebased with master) before trying that.

	Ross Assignee
	Comment 23 • 9 years ago

^^  Ha, you read my mind :).  I had started to read about that process (https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch) just a few minutes ago, but thanks for the steps.

I think I've got it under control but I'll be sure to ping you if I have any questions before the merge.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 24 • 9 years ago

Hi Ross, while it's being reviewed let me know if you want another bug, and also feel free to join #accessibility channel on IRC!

	Ross Assignee
	Comment 25 • 9 years ago

Hey Yura,

Yeah, I'd love to work on another bug.  If you have one in mind, let me know and I'll get started!  I also joined the #accessibility chan as Funktr0n, so I should be around.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 26 • 9 years ago

Hey Ross, how about bug 1069223. I wonder though what you use for testing purposes as most of the media apps (which FM Radio is one) might require an actual device or at least a B2G desktop emulator.

	R
	Comment 27 • 9 years ago

I've actually got a Flame reference phone so I do all my testing on the real device.  The bug looks good to me -- do you want to assign it to me?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 28 • 9 years ago

Hi Dominic, I replied to your comment in the PR, let me or Ross know if anything else needs to be done, Thanks.

	Dominic Kuo [:dkuo]
	Comment 29 • 9 years ago

Comment on attachment 8546340 [details] [review]
Updated Pull Request on Official Gaia repo to include javascript and CSS edits in addition to the previous HTML & localization files

Yura, thanks for patching this and the code looks good to me, it looks no harm to the player view and you have tests for it! there is one minor issue about the tests and please read my github comments.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 30 • 9 years ago

Ross, I left the comment about what Dominic mentioned in Github. Also a couple of overlooked linting things. Thanks and sorry for another round :)

	Ross Assignee
	Comment 31 • 9 years ago

Hi Yura,

No probs, I updated the PR with the proposed changes.  Would have been faster on this one but my Internet connection's been down the past few days.  Should be back up tomorrow (hopefully!).

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 33 • 9 years ago

Hi Ross, thanks, there's just one linting issue in the file (see github).

	Ross Assignee
	Comment 34 • 9 years ago

Okay, fixed the line too long error.  Also installed gjslint to help catch these on future commits.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 35 • 9 years ago

Hi Ross, would you also squash all your commits into one? We would need to do that before merging. You can do it from your branch by running: git rebase -i, see comment 21

	Ross Assignee
	Comment 36 • 9 years ago

All set (hope I did that right!)

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 37 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/d0837b7a1191faeeae66bd0b786d44426fed947b

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 38 • 9 years ago

Ross, thanks a lot for the pull request, I merged it in and cograts :)

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 39 • 9 years ago

s/cograts/congrats

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 40 • 9 years ago

Comment on attachment 8546340 [details] [review]
Updated Pull Request on Official Gaia repo to include javascript and CSS edits in addition to the previous HTML & localization files

[Approval Request Comment] This pull request fixes accessibility for rating and share functionality in the music app.
[Bug caused by] (feature/regressing bug #): improvement, not a bug
[User impact] if declined: Screen reader users would not be able to share or rate the music as the controls would be inaccessible.
[Testing completed]: Unit tests and manual on device
[Risk to taking this patch] (and alternatives if risky): Low, added a11y specific attributes mostly.
[String changes made]: https://github.com/mozilla-b2g/gaia/pull/27263/files#diff-68d1545207ad905d29bc337911e04066

	Ross Assignee
	Comment 41 • 9 years ago

(In reply to Yura Zenevich [:yzen] from comment #38)
> Ross, thanks a lot for the pull request, I merged it in and cograts :)

Awesome! Thanks! :)

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 9 years ago

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