Closed Bug 1069614 Opened 6 years ago Closed 5 years ago

Albums are not accessible in main view.


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

Gonk (Firefox OS)
Not set


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

2.2 S5 (6feb)
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: yzen, Assigned: aurelien.levy)



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


(1 file, 1 obsolete file)

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

Only album/artist text nodes are accessible by the screen reader and nothing can be activated.
I can try something and make a PR but I think using a list will be better than a grid here
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.
maybe it's better if I wait after the end of refactoring i'm not very expert in git manipulation yet.
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
Flags: needinfo?(hub)
Attachment #8530494 - Flags: review?(squibblyflabbetydoo)
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.
Assignee: nobody → aurelien.levy
Flags: needinfo?(hub) → needinfo?(yzenevich)
I left some comments in Github, overall looks pretty good. Feel free to mark me for a11y-review on your patch.
Flags: needinfo?(yzenevich)
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.
Attachment #8530494 - Flags: review?(squibblyflabbetydoo)
Attachment #8530494 - Flags: review?(squibblyflabbetydoo)
Attachment #8530494 - Flags: a11y-review?(yzenevich)
Comment on attachment 8530494 [details] [review]
Bug-1069614 - Albums are not accessible in main view.

Works well from a11y standpoint, thanks!
Attachment #8530494 - Flags: a11y-review?(yzenevich) → a11y-review+
Blocks: 1108662
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.
Attachment #8530494 - Flags: review?(squibblyflabbetydoo) → review-
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:
+ many other files
Please move or remove them before you can switch branches.
could not detach HEAD

Sorry for that again
Flags: needinfo?(yzenevich)
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-
Flags: needinfo?(yzenevich)
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
Attachment #8530494 - Attachment is obsolete: true
Ok I failed to clean the first PR so I do a clean new one. Hope this will be good this time
Attachment #8535773 - Flags: review?(squibblyflabbetydoo)
You don't need to recreate the PR. Just pushing to the existing branch is enough to get it updated.
I know but I have some issues with my local branch so I have to remove it entirely know it was clean
Flags: needinfo?(hub)
Flags: needinfo?(hub)
Comment on attachment 8535773 [details] [review]
Bug 1069614 - making albums accessible in the main view

r=me with on minor comment on Github. Thanks!
Attachment #8535773 - Flags: review?(squibblyflabbetydoo) → review+
sorry for this late fix, I think it's good now
Flags: needinfo?(squibblyflabbetydoo)
(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.
Flags: needinfo?(squibblyflabbetydoo)
Please see Jim's comment about the merge conflict in your patch so we could land this. Thanks!
Flags: needinfo?(aurelien.levy)
done sorry for the delay
Flags: needinfo?(aurelien.levy)

Thanks, Aurelien!
Closed: 5 years ago
Resolution: --- → FIXED
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.
Resolution: FIXED → ---
Depends on: 1137053
Made a blocker instead of reopening this.
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Attachment #8535773 - Flags: approval-gaia-v2.2?
Attachment #8535773 - 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.