969553 - Update shared tabs styling and markup to be screen reader accessible.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Currently actual tab buttons are wrapped within the container [role="tab"] element which results in an additional stop when traversing the screen with the screen reader. This needs to change.

Comment 1

[Marco Zehe (:MarcoZ)]

Need help with the markup, or do you know where you have to go with these? My blog has an Advanced ARIA Tip #1 on this subject, too.

Comment 2

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

Thanks Marco, pretty much what I'm doing but was missing aria-controls and aria-labelledby!

Comment 3

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

Attachment: Github pull request.

I removed role selectors ([role="tab"], [role="tablist"], [role="tabpanel"]) from css and replaced them with css classes (.bb-tabpanel, .bb-tablist). 

It is better to avoid styling based on roles in order to not break the semantics. For example: email app uses tablist/tab/tabpanel roles for all of its toolbars only for the styling purposes. The toolbars that the style is applied to are not actually tabs but collections of buttons that trigger dialogs, add flags to the email etc. This breaks the semantics for the screen reader user.

Also, I updated the elements that the role="tab" is attached to. Preferably it should be an actual focusable element that triggers the tab selection (e.g. <a>) and not its parent. 

I added role="presentation" to the parent <li> elements to make sure that the screen reader only focuses on the actual tab.

aria-selected is not handled by the shared AccessibilityHelper and is used across all apps that need to update it.

Comment 4

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

(In reply to Yura Zenevich [:yzen] from comment #3)
> aria-selected is not handled by the shared AccessibilityHelper and is used
> across all apps that need to update it.
s/not/now

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 8375796 [details] [review]
Github pull request.

Flagging one peer per touched app just so they know this is coming and to spread the knowledge around good ARIA and CSS practices.

I don't think we should wait on everyone's feedback to land this though.

Comment 6

[Anthony Ricaud (:rik)]

Marco: Could you actually link to your blog post? It will be helpful knowledge in a different form than a pull request :)

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Anthony Ricaud (:rik) from comment #6)
> Marco: Could you actually link to your blog post? It will be helpful
> knowledge in a different form than a pull request :)

http://www.marcozehe.de/2013/02/02/advanced-aria-tip-1-tabs-in-web-apps/

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8375796 [details] [review]
Github pull request.

I'm not sure if it's going to be easy to land this in a massive PR, but F+. Left one comment on github.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8375796 [details] [review]
Github pull request.

In terms of engineering, it's great to have this kind of clean-up. (That's Tim the developer talking)

However judging by the experience of bug 914892, we need to be more careful on this kind of change; it pretty hard to identify if we have changed all instances in every app that use that CSS block.

There were some discussion around migrating building blocks (and shared script in general) with version. I wonder if we need to make that work before working on bug like this?

That said, since it's early in the release cycle, if you are confident of your fix, and you are available for resolving regression bugs daily-ish for the next 6 weeks, we can try to do it.

(... and that, is Tim the manager talking)

Comment 10

[Marco Zehe (:MarcoZ)]

Eitan beat me to it. :) Thanks!

Comment 11

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Comment on attachment 8375796 [details] [review]
Github pull request.

Hey nice one :) I like it :) I work on the refresh of [BB]Tabs and I tried to keep the markup like was before the refresh but I also realize that we have a lot of problems with the aria-roles at all.

I can help you make this changes (two hands more :) on the apps who use the [BB]Tabs I already unify the markup on them and I'm familiar with the [BB] so feel free to assign me some bugs or ask me for r+/f+

Comment 12

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> Comment on attachment 8375796 [details] [review]
> Github pull request.
> 
> In terms of engineering, it's great to have this kind of clean-up. (That's
> Tim the developer talking)
> 
> However judging by the experience of bug 914892, we need to be more careful
> on this kind of change; it pretty hard to identify if we have changed all
> instances in every app that use that CSS block.
> 
> There were some discussion around migrating building blocks (and shared
> script in general) with version. I wonder if we need to make that work
> before working on bug like this?
> 
> That said, since it's early in the release cycle, if you are confident of
> your fix, and you are available for resolving regression bugs daily-ish for
> the next 6 weeks, we can try to do it.
> 
> (... and that, is Tim the manager talking)

Hi Tim, I'll be available for that :)!

Comment 13

[Salvador de la Puente González [:salva]]

Comment on attachment 8375796 [details] [review]
Github pull request.

We are loosing functionality. I think it is something related with the CSS. Moving to :mai for further details.

Comment 14

[Marina Rodríguez [:mai]]

Comment on attachment 8375796 [details] [review]
Github pull request.

Please, review my comments on GitHub. Once addressed, ask for the feedback again.
Thank you

Comment 15

[Anthony Ricaud (:rik)]

Comment on attachment 8375796 [details] [review]
Github pull request.

I've left two comments in the PR for bad CSS selectors.

Also, I'm seeing a flash of "Select All" "Deselect All" buttons when opening the Dialer. I'm not sure why but this needs to be fix.

Comment 16

[Marina Rodríguez [:mai]]

Comment on attachment 8375796 [details] [review]
Github pull request.

Looks good to me. Thanks!

Comment 17

[Dominic Kuo [:dkuo]]

Comment on attachment 8375796 [details] [review]
Github pull request.

Anthony,

overall the patch looks good, but since highlighting the selected tab does not depend on the hash change, you should modify the related code, like: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/js/music.js#L177 which forcing the hash to be the first page(mix) after music is reset by mount/unmount the storages as well, also in the manifest.webapp, the launch_path contains the hash because we want to highlight the tab, probably we can remove it if the approach of how we highlight the tabs is changed. And if you are going to land this and not confident of breaking nothing, please set the review(music part) to me and I will check for you, thanks.

Comment 18

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

All comments addressed in the PR and this bug. Anthony, Ben, would you have time for another f? or should I ask for r?

Comment 19

[Anthony Ricaud (:rik)]

Comment on attachment 8375796 [details] [review]
Github pull request.

Looks good overall. A few nits and some concerns around unit tests.

Comment 20

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

Comment on attachment 8375796 [details] [review]
Github pull request.

Added unit tests for accessibility_helper. Using mock accessibility helper in other apps' unit tests. addressed the nits.

Comment 21

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

Anthony, the PR should be ready for, hopefully, final round of feedback :).

Comment 22

[Anthony Ricaud (:rik)]

I'll try to take a look at this later this week but it's not high on my priority list.

In any case, even with a positive review, we should not land this on master anymore because it is a too risky change. Sorry :(
http://www.quickmeme.com/img/54/5433867a3c819788cd0651b4c35613c6be6072c3fb4cbd25d102d502452c1f02.jpg

Comment 23

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

I updated the PR to the latest master. As in previous version of the PR tabs are made accessible across all apps that have them and the tests you requested are added. Hopefully the timing to get the f? is better this time around :)

Comment 24

[Anthony Ricaud (:rik)]

Comment on attachment 8375796 [details] [review]
Github pull request.

I'm still seeing the flash that I mentioned in comment 15.

I've left some comments on Github about the tests.

Comment 25

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

Comment on attachment 8375796 [details] [review]
Github pull request.

Fixed the issues with flickering buttons, all comments addressed.

Comment 26

[Anthony Ricaud (:rik)]

Yura: If the fix from bug 986047 lands before you (very likely), can you rebase and make sure the fix still works? And that we don't have any extra CSS.

Comment 27

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8375796 [details] [review]
Github pull request.

LGTM, but please rebase before merging as you have conflicts with dialer app.

Comment 28

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

Rebased

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/e1c443d9ff62a21920d389be097d0e666f38e70b

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures on TBPL.
https://github.com/mozilla-b2g/gaia/commit/ff5d799612ae831fcb97f3a645408c7b57d58743

https://tbpl.mozilla.org/php/getParsedLog.php?id=36752591&tree=B2g-Inbound

Comment 31

[Anthony Ricaud (:rik)]

Yura: This failure is because unit tests are run in isolation on TBPL. I'm not sure we can easily do that locally but maybe https://developer.mozilla.org/en-US/Firefox_OS/TBPL#Running_TBPL_Try_jobs_against_Gaia_builds can help you.

Comment 32

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

Attachment: Github pull request.

Updated the pull request to fix the test failure. Added mocks for AccessibilityHelper in the necessary clock tests. If all is good I will squash the 2 commits before marking for checkin.

Comment 33

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

The failing test now passes.
https://tbpl.mozilla.org/?tree=Try&rev=0562128877c9

Comment 34

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8397427 [details] [review]
Github pull request.

LGTM, squash your commits and that's all ;)

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/6a3bdfc1ee9acb908ba54dcd0f631d11aeb0a0f1