Closed Bug 1162096 Opened 9 years ago Closed 8 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: access)

Attachments

(3 files)

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.
Attached file Github pull request.
Hey Wilson, what do you think about this approach (in regards to l10n for a11y)?
Attachment #8688662 - Flags: review?(wilsonpage)
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Hi Wilson, updated the PR, left a reply for one of your comments. Let me know what you think. Thanks
Flags: needinfo?(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.
Flags: needinfo?(wilsonpage)
Attachment #8688662 - Flags: review?(wilsonpage)
Attached file Gaia-component l10n
Attachment #8691424 - Flags: review?(wilsonpage)
Hi Wilson, the l10n PR should be updated and ready for another look. thanks!
Flags: needinfo?(wilsonpage)
Comment on attachment 8691424 [details] [review]
Gaia-component l10n

r+ with NITs addressed :)
Flags: needinfo?(wilsonpage)
Attachment #8691424 - Flags: review?(wilsonpage) → review+
Comment on attachment 8688662 [details] [review]
Github pull request.

Ok, I think this is ready for another view. Thanks!
Attachment #8688662 - Flags: review?(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.
Flags: needinfo?(stas)
Attachment #8688662 - Flags: review?(wilsonpage)
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.
Flags: needinfo?(stas)
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?
Flags: needinfo?(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?
Flags: needinfo?(wilsonpage)
(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.
Attached file Github pull request.
Simplifying the way l10n is done in fxos-component.
Attachment #8701521 - Flags: review?(wilsonpage)
Comment on attachment 8688662 [details] [review]
Github pull request.

Updated l10n for toolbar, moved dependencies from bower to npm.
Attachment #8688662 - Flags: review?(wilsonpage)
Comment on attachment 8688662 [details] [review]
Github pull request.

Some comments inline
Attachment #8688662 - Flags: review?(wilsonpage)
Hi Wilson, would you mind publishing v1.0.3 fxos-component to npm?
Flags: needinfo?(wilsonpage)
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.
Attachment #8688662 - Flags: review?(wilsonpage)
(In reply to Yura Zenevich [:yzen] from comment #18)
> Hi Wilson, would you mind publishing v1.0.3 fxos-component to npm?

Done :)
Flags: needinfo?(wilsonpage)
OK, should be finished with the comments, thanks!
Flags: needinfo?(wilsonpage)
Comment on attachment 8688662 [details] [review]
Github pull request.

conditional r+

- Can we rename 'gaia-toolbar' -> 'fxos-toolbar' as part of this change? :)
Flags: needinfo?(wilsonpage)
Attachment #8688662 - Flags: review?(wilsonpage) → review+
Done, also bumped version to 1.0.0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: