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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

54 bytes, text/x-github-pull-request
wilsonpage
: review+
Details | Review | Splinter Review
57 bytes, text/x-github-pull-request
wilsonpage
: review+
Details | Review | Splinter Review
57 bytes, text/x-github-pull-request
wilsonpage
: review+
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8688662 [details] [review]
Github pull request.

Hey Wilson, what do you think about this approach (in regards to l10n for a11y)?
Attachment #8688662 - Flags: review?(wilsonpage)
(Assignee)

Updated

3 years ago
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 4

3 years ago
Created attachment 8691424 [details] [review]
Gaia-component l10n
Attachment #8691424 - Flags: review?(wilsonpage)
(Assignee)

Comment 5

3 years ago
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+
(Assignee)

Comment 8

3 years ago
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)
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
Created attachment 8701521 [details] [review]
Github pull request.

Simplifying the way l10n is done in fxos-component.
Attachment #8701521 - Flags: review?(wilsonpage)
(Assignee)

Comment 15

3 years ago
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)
(Assignee)

Comment 18

3 years ago
Hi Wilson, would you mind publishing v1.0.3 fxos-component to npm?
Flags: needinfo?(wilsonpage)
(Assignee)

Comment 19

3 years ago
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)
(Assignee)

Comment 21

3 years ago
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+
(Assignee)

Comment 23

3 years ago
Done, also bumped version to 1.0.0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.