1069614 - Albums are not accessible in main view.

Status: RESOLVED FIXED

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

Description

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

Expected:
Albums grid/table should:
* have proper grid or table semantics
* should be activatable
* traverse-able in the correct order.

Observed:
Only album/artist text nodes are accessible by the screen reader and nothing can be activated.

Comment 1

[aurelien levy]

I can try something and make a PR but I think using a list will be better than a grid here

Comment 2

[Hubert Figuiere [:hub]]

Let me know if you need help for the music side of things.

Also please be aware that we have some refactoring in progress so I recommend working off the master branch (2.2) as this is where things are happening.

Comment 3

[aurelien levy]

maybe it's better if I wait after the end of refactoring i'm not very expert in git manipulation yet.

Comment 4

[aurelien levy]

Attachment: Bug-1069614 - Albums are not accessible in main view.

fix with aria, not sure where the click event is applied on the cover to check it. I add a tabindex"0" so if it's one the same element the click event must fire too. It look like the 2.1 branch is different so not sure if I need to do the same fix here too

Comment 5

[Hubert Figuiere [:hub]]

I'm not exactly sure how the accessibility side works. :yzen will probably have more insight.

As for the changes, keep it to 2.2 (git master).

I have put some comments in the PR regarding coding style (spacing). Please follow these.

Thank you.

Comment 6

[aurelien levy]

done

Comment 7

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

I left some comments in Github, overall looks pretty good. Feel free to mark me for a11y-review on your patch.

Comment 8

[Jim Porter (:squib)]

Comment on attachment 8530494 [details] [review]
Bug-1069614 - Albums are not accessible in main view.

Clearing out review for now until the PR is updated to account for :yzen's comments.

Comment 9

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

Comment on attachment 8530494 [details] [review]
Bug-1069614 - Albums are not accessible in main view.

Works well from a11y standpoint, thanks!

Comment 10

[Jim Porter (:squib)]

Comment on attachment 8530494 [details] [review]
Bug-1069614 - Albums are not accessible in main view.

Thanks, this looks good. I have a couple of minor nits on GitHub, though.

Comment 11

[aurelien levy]

I made the changes but for now I can't find how to rebase the two commits in one because of some troubles with my local branch. I have uncommited and unwanted commits stopping me to do the rebase and I don't know how to remove them. I get this message :

The following untracked working tree files would be overwritten by checkout:
apps/keyboard/js/layouts/te.js
apps/music/js/metadata/flac.js
+ many other files
Please move or remove them before you can switch branches.
Aborting
could not detach HEAD

Sorry for that again

Comment 12

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

Ok Aurelien, from what I can tell your Github branch only has 2 commits, which is good, we can fix that:

* Lets remove your local Bug-1069614---Albums-are-not-accessible- branch (make sure you are in another branch, like master): 
git branch -D Bug-1069614---Albums-are-not-accessible-

* Check out your public Bug-1069614---Albums-are-not-accessible- branch:
git checkout bug-1109353

* So now, hopefully we can do rebase:

git rebase -i upstream/master

* The familiar dialog will ask what to do with only 2 commits: squash the nit one and pick the original one and make sure the message looks good.

* Once rebase is done push it to the branch:
git push -f origin Bug-1069614---Albums-are-not-accessible-

Comment 13

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

Also next time if you want to keep things simple and not add new commits, you can update your commit (as long as it's the last one in the log) but doing:

git reset --soft 'HEAD^'

making and staging all your changes and then running:

git commit -c ORIG_HEAD

Comment 14

[aurelien levy]

Attachment: Bug 1069614 - making albums accessible in the main view

Ok I failed to clean the first PR so I do a clean new one. Hope this will be good this time

Comment 15

[Hubert Figuiere [:hub]]

You don't need to recreate the PR. Just pushing to the existing branch is enough to get it updated.

Comment 16

[aurelien levy]

I know but I have some issues with my local branch so I have to remove it entirely know it was clean

Comment 17

[Jim Porter (:squib)]

Comment on attachment 8535773 [details] [review]
Bug 1069614 - making albums accessible in the main view

r=me with on minor comment on Github. Thanks!

Comment 18

[aurelien levy]

sorry for this late fix, I think it's good now

Comment 19

[Jim Porter (:squib)]

(In reply to aurelien levy from comment #18)
> sorry for this late fix, I think it's good now

As :hub has mentioned on GitHub, you've left a merge conflict in your patch. You'll need to fix that before this can land.

Comment 20

[Eitan Isaacson [:eeejay]]

Please see Jim's comment about the merge conflict in your patch so we could land this. Thanks!

Comment 21

[aurelien levy]

done sorry for the delay

Comment 22

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/5553a118471e041be28ad6406f148a2c1a10841f

Thanks, Aurelien!

Comment 23

[Eitan Isaacson [:eeejay]]

I'm re-opening this since, while we got nice roles, the album buttons are not operable.

Basically, the click listener waits for transitionend to happen after the :active state ends, and only then plays the album. I guess we never get an :active state when we simulate a click from the accessibility API.

Comment 24

[Eitan Isaacson [:eeejay]]

Made a blocker instead of reopening this.

Comment 25

[Eitan Isaacson [:eeejay]]

Comment on attachment 8535773 [details] [review]
Bug 1069614 - making albums accessible in the main view

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Albums in music grid view will not be perceivable to screen reader users.
[Testing completed]: Yes.
[Risk to taking this patch] (and alternatives if risky): Low.
[String changes made]: No.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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