1162096 - [accessibility][gaia-toolbar] Make sure gaia-toolbar is accessible to the screen reader.

Status: RESOLVED FIXED

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

Description

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

Several issues found:

* Semantics should be of a toolbar as well as its child elements.
* Consider specifying a landmark location for the toolbar (e.g. if it's in the footer)
* Icons need to be correctly localized for the screen reader.

Comment 1

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

Attachment: Github pull request.

Hey Wilson, what do you think about this approach (in regards to l10n for a11y)?

Comment 2

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

Hi Wilson, updated the PR, left a reply for one of your comments. Let me know what you think. Thanks

Comment 3

[Wilson Page [:wilsonpage]]

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

Load of valuable work in here! Comments:

- I'm not sure the dynamic `data-l10n-id` attributes inside shadow-root will be found by l10n.js (but I may be wrong).

- There seems like a lot of code here to just proxy values from the light-dom to the shadow-dom. It's perhaps OK if we only have one node to localize, but this approach might not scale that well. I feel like we could be smarter about this, but not yet sure how :)

Perhaps we just let the user optionally provide a more-button that we can 'project' into the correct place in the shadow-dom.

usage:

<gaia-toolbar>
  <button more-button aria-label="More"></button>
  <button>Tab1</button>
  <button>Tab2</button>
  <button>Tab3</button>
</gaia-toolbar>

shadow:

<div class="inner">
  <div class="more-button">
    <content select="[more-button]"></content>
  </div>
  <content></content>
</div>

We can put any 'click' handlers on `div.more-button` so that the component works without the projected button, but is enhanced when it's used.

We may have to do some detection for this projected button and apply different a11y logic it it's not present.

I favour this approach as it means we don't have to do all the work and requires less documentation/understanding IMO. You may have reasons why this is not appropriate though :)

- I remember seeing gaia-toolbar show up on some profiles I was doing for Music app. This was due to the logic run within `toolbar.reflow()`. As this functionality is expensive and rarely used, if would be nice to change it to opt-in. Or optimize it to not require reading measurements from the DOM.

Comment 4

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

Attachment: Gaia-component l10n

Comment 5

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

Hi Wilson, the l10n PR should be updated and ready for another look. thanks!

Comment 6

[Wilson Page [:wilsonpage]]

Comment on attachment 8691424 [details] [review]
Gaia-component l10n

r+ with NITs addressed :)

Comment 7

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

l10n: https://github.com/gaia-components/gaia-component/commit/670779452e032fb12d04e12419d5975c6dd6ed7d

Comment 8

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

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

Ok, I think this is ready for another view. Thanks!

Comment 9

[Wilson Page [:wilsonpage]]

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

Some questions around l10n. I just need Stas to clarify that the localization team can support locale files in separate repos.

Comment 10

[Staś Małolepszy :stas]

I'm afraid this isn't supported right now.  New strings are only discovered in the the main Gaia repository right now.  We'd also need to figure out where to store translation files, how to track progress of translations, how to handle branching and versioning, as well as how to keep the list of languages available in the component in sync with the list of languages available in Gaia, and what to do when they get out of sync.

I'm not saying this isn't possible to figure out, and we're working on answering these questions, but we need more time to think this through.  I'd much rather see us continue with the current approach of storing the translation files in the main Gaia repository.  We could say that the gaia-toolbar components declares a "localization interface" in that it expects the app to provide the "gaia-toolbar-more.ariaLabel" translation.

Also, please prefer camel case for string ids for forward compatiblity with the l20n syntax.

Comment 11

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

So Stas and I talked about meeting or having a conversation about gaia-components l10n in Orlando so this can hold off for the time being.

On the side note, Wilson, there's an issue with marionette-client being broken in npm and it doesn't look like there's a solution unless we stop using relative paths in test packages in gaia. I have something working for it but it's not very elegant. Perhaps you have some thoughts?

Comment 12

[Wilson Page [:wilsonpage]]

(In reply to Yura Zenevich [:yzen] from comment #11)
> So Stas and I talked about meeting or having a conversation about
> gaia-components l10n in Orlando so this can hold off for the time being.
> 
> On the side note, Wilson, there's an issue with marionette-client being
> broken in npm and it doesn't look like there's a solution unless we stop
> using relative paths in test packages in gaia. I have something working for
> it but it's not very elegant. Perhaps you have some thoughts?

This wasn't previously an issue was it? Can we roll back to an earlier version, or is this an issue with NPM3?

Comment 13

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

(In reply to Wilson Page [:wilsonpage] from comment #12)
> (In reply to Yura Zenevich [:yzen] from comment #11)
> > So Stas and I talked about meeting or having a conversation about
> > gaia-components l10n in Orlando so this can hold off for the time being.
> > 
> > On the side note, Wilson, there's an issue with marionette-client being
> > broken in npm and it doesn't look like there's a solution unless we stop
> > using relative paths in test packages in gaia. I have something working for
> > it but it's not very elegant. Perhaps you have some thoughts?
> 
> This wasn't previously an issue was it? Can we roll back to an earlier
> version, or is this an issue with NPM3?

OK the problem has been resolved, Gareth uses npmr to publish to npm which fixes dependencies before publishing. 1.9.4 works just fine now.

Comment 14

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

Attachment: Github pull request.

Simplifying the way l10n is done in fxos-component.

Comment 15

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

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

Updated l10n for toolbar, moved dependencies from bower to npm.

Comment 16

[Wilson Page [:wilsonpage]]

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

https://github.com/fxos-components/fxos-component/commit/3908528e7444470a6251dbcbfa1a5aa53231aea8

Comment 17

[Wilson Page [:wilsonpage]]

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

Some comments inline

Comment 18

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

Hi Wilson, would you mind publishing v1.0.3 fxos-component to npm?

Comment 19

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

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

Ok hopefully all comments addressed. Note this depends on fxos-component@1.0.3 published to npm so travis will fail atm.

Comment 20

[Wilson Page [:wilsonpage]]

(In reply to Yura Zenevich [:yzen] from comment #18)
> Hi Wilson, would you mind publishing v1.0.3 fxos-component to npm?

Done :)

Comment 21

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

OK, should be finished with the comments, thanks!

Comment 22

[Wilson Page [:wilsonpage]]

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

conditional r+

- Can we rename 'gaia-toolbar' -> 'fxos-toolbar' as part of this change? :)

Comment 23

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

Done, also bumped version to 1.0.0