Closed
Bug 1162040
Opened 10 years ago
Closed 9 years ago
[accessibility][gaia-icons] Make sure icons are properly localized for accessibility.
Categories
(Firefox OS Graveyard :: Gaia::Components, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: yzen, Assigned: yzen)
References
Details
(Keywords: access)
Attachments
(14 files)
53 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
yzen
:
feedback+
|
Details | Review |
53 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
wilsonpage
:
feedback+
gasolin
:
feedback+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
mikehenrty
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
arthurcc
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
mmedeiros
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
drs
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
mai
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
dkuo
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
etienne
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
squib
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
julienw
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
gsvelto
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
|
Details | Review |
53 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
|
Details | Review |
Currently the only content available to screen reader from an icon is defined by the CSS content property read from data-icon attribute. This is not localized and this not supported well by the screen reader.
We need to ensure that the correct localized aria-label or other meaningful attribute is localized.
Comment 1•10 years ago
|
||
This is the job of the app owner, no?
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #1)
> This is the job of the app owner, no?
Which part exactly? If you mean identifying which icons are for decoration only, that right. I am suggesting to either let the app owner apply aria-hidden to icons themselves, or have some other attribute supported within gaia-button available to the author that is more meaningfully named (I am pretty sure aria-hidden is not).
Comment 3•10 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #2)
> (In reply to Wilson Page [:wilsonpage] from comment #1)
> > This is the job of the app owner, no?
>
> Which part exactly? If you mean identifying which icons are for decoration
> only, that right. I am suggesting to either let the app owner apply
> aria-hidden to icons themselves, or have some other attribute supported
> within gaia-button available to the author that is more meaningfully named
> (I am pretty sure aria-hidden is not).
Can you give me an example of what you mean?
Assignee | ||
Comment 4•10 years ago
|
||
Ah, sorry I confused this bug with the one for buttons (so ignore comment 2).
I agree, in this case it is indeed authors responsibility. That can generally be said about accessibility as well. However, I feel like there's little incentive for an author to add the label. So, I'm worried that we are setting ourselves up for accessibility issues down the road, when icons are used more widely. My hope is that we can provide a toolkit (such as gaia-components) for authors to use that has accessibility in mind from the very beginning.
So I have a couple of suggestions (possible some that are more radical than the other ones):
* I think things could improve already even if we always and only demonstrate usage examples with aria-label (or data-l10n-id in Gaia context) used for icons (currently within both the README and the gh-pages).
* I wonder what you think about some helpful diagnostics when gaia icons are used. For example, have an icon style (or CSS content) that indicates warning/error when the rule is something like:
[data-icon]:not([aria-label]),
[data-icon]:not([data-l10n-id]) {...}
Comment 5•10 years ago
|
||
Might be cool to have:
<gaia-icon name="camera"></gaia-icon>
So then we can handle accessibility inside the component. This may not be as performant as what we have right now though, as each icon would need to run some script.
Comment 6•10 years ago
|
||
current gaia-icons is just a .css to include webfont,
For performance concern I prefer do that handling in each component.
suggest rule for related components:
* If has `data-l10n-id` in <i ..>, add `aria-label` with data-l10n-id, else add `aria-hidden="true"`
So app owner only needs add essential `data-l10n-id` to indicate their explicit purpose, the component do aria treatment for them automatically.
make sense?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #5)
> Might be cool to have:
>
> <gaia-icon name="camera"></gaia-icon>
>
> So then we can handle accessibility inside the component. This may not be as
> performant as what we have right now though, as each icon would need to run
> some script.
Yes, I think since both you and Fred suggest negative affect on performance this approach would be undesirable.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #6)
> current gaia-icons is just a .css to include webfont,
> For performance concern I prefer do that handling in each component.
>
> suggest rule for related components:
>
> * If has `data-l10n-id` in <i ..>, add `aria-label` with data-l10n-id, else
> add `aria-hidden="true"`
>
> So app owner only needs add essential `data-l10n-id` to indicate their
> explicit purpose, the component do aria treatment for them automatically.
>
> make sense?
I think this approach brings us back to square one, where data-l10n-id's are simply going to be omitted by the author.
I would propose:
* always enforcing data-l10n-id that maps to entry.ariaLabel in the message bundle for *all* icons.
* opt out of the label within the related components by explicitly indicating which icons are for presentation only.
For example:
* icons will always have data-l10n-id that will resolve aria-label (already supported by l10n):
<i data-l10n-id="camera" aria-label="Camera" data-icon="camera"></i>
* if related component only uses the icon for presentation, it should be indicated via some sort of attribute:
<gaia-button>
<i data-l10n-id="camera" aria-label="Camera" data-icon="camera" {someattribute, maybe even
role="presentation"}></i>
Camera
</gaia-button>
Then gaia-button (and other components) could apply aria-hidden for all presentational icons.
Comment 9•10 years ago
|
||
Currently in my survey in bug 1162138 comment 5, the best way to make sure icon with localizable text is put a side <span> with icon tag, and hide the span with style.
If we input
`<i data-icon="camera" data-l10n-id="camera desc"></i>`
It make rendered DOM looks like:
```
<i data-icon="camera" aria-hidden="true"></i>
<span class="icon-aria-note" data-l10n-id="camera desc">Camera</span>
```
If we take this approach to reach localizable icon text, it means add `data-l10n-id` to each icon is costly.
For the issue `data-l10n-id's are simply going to be omitted by the author`, to eliminate the root cause, we may driven the act if UX can include accessibility concern in their future released UX spec (since a11y also need to be well thought in design phase)? And app owner will follow that spec.
Comment 10•10 years ago
|
||
I got a new proposal: always declare icon into some component's attribute.
Refer to existing <gaia-header> element,
```
```html
<gaia-header action="back" title-start="50" title-end="100">
<h1>title</h1>
<button data-icon="add"></button>
<button data-icon="settings"></button>
</gaia-header>
```
```
Which already define icon in button element
we could redefine gaia-button as:
```
<button data-icon="" data-icon-end="" data-l10n-id="">
```
It means button has icon at start and end.
```
<button data-icon-only="" data-l10n-id="">
```
It means button only shows icon, but provide localized text for ScreenReader.
So we can remove all `<i>` element, embedded icons into that component, and enforce adding `data-l10n-id` attribute in target gaia-element.
Comment 11•10 years ago
|
||
Thanks for keeping provide valuable advises for a11y situations.
I got a proposal to remove all `<i>` element, embedded icons into some component's attribute, and enforce adding `data-l10n-id` attribute in target gaia-element.
Refer to existing <gaia-header> element,
```
```html
<gaia-header action="back" title-start="50" title-end="100">
<h1>title</h1>
<button data-icon="add"></button>
<button data-icon="settings"></button>
</gaia-header>
```
```
Which already define data-icon in button element instead of standalone <i> element.
we could redefine gaia-button as:
```
<button data-icon="" data-icon-end="" data-l10n-id="">
```
It means button has icon at start and end.
```
<button data-icon-only="" data-l10n-id="">
```
It means button only shows icon, but provide localized text for ScreenReader. (I'm not sure if icon-only button can works with data-l10n-id this way now, but its a proposal)
Comment 12•10 years ago
|
||
sorry I've some network connection issue, please ignore comment 10
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #9)
> Currently in my survey in bug 1162138 comment 5, the best way to make sure
> icon with localizable text is put a side <span> with icon tag, and hide the
> span with style.
>
> If we input
>
> `<i data-icon="camera" data-l10n-id="camera desc"></i>`
>
> It make rendered DOM looks like:
>
> ```
> <i data-icon="camera" aria-hidden="true"></i>
> <span class="icon-aria-note" data-l10n-id="camera desc">Camera</span>
> ```
In case of screen reader specific localization, I think it is not the case, since it depends on what data-l10n-id points to. So for example if data-l10n-id="test" points to test.ariaLabel=Test in the message bundle, then l10n will create an aria-label="Test" attribute on the node instead of touching inner HTML. But I see your comment about removing <i>'s.
>
> If we take this approach to reach localizable icon text, it means add
> `data-l10n-id` to each icon is costly.
>
>
>
> For the issue `data-l10n-id's are simply going to be omitted by the author`,
> to eliminate the root cause, we may driven the act if UX can include
> accessibility concern in their future released UX spec (since a11y also need
> to be well thought in design phase)? And app owner will follow that spec.
Assignee | ||
Comment 14•10 years ago
|
||
I think there are still two separate problems here:
1) Identifying gaia-icons for presentation only.
2) Localizing gaia-icons
The first problem can be solved via aria-hidden="true" attribute. If it is present, localized or not, our screen reader will not be able to reach the icon. It is a good indicator of whether the icon needs localization.
The second problem depends on the context:
* If gaia-icons are used as a standalone, e.g. data-icon attributed element.
Unless aria-hidden attribute is used on the icon, it will always be accessible to the screen reader, which means it must be localized (if it is not, the screen reader will read the text from the CSS content property which is not localized). It can be achieved by adding data-l10n-id to the element with data-icon that points to {property}.ariaLabel in the properties file (that will add an aria-label attribute to the same element and will not touch inner HTML).
* If gaia-icons are used in context of another gaia-component
Pretty much the same rules apply, but we, as a component author, can infer certain things about icon visibility and localization, or have some abstraction where gaia element attributes are propagated to icons, buttons etc.
So to reply to your comment about removing <i>, I think it's mostly orthogonal problem:
(In reply to Fred Lin [:gasolin] from comment #11)
> Thanks for keeping provide valuable advises for a11y situations.
>
> I got a proposal to remove all `<i>` element, embedded icons into some
> component's attribute, and enforce adding `data-l10n-id` attribute in target
> gaia-element.
This is good, I'm especially in support of enforcing data-l10n-id attribute. As far as I understand, this means that we enforce localization of all elements with data-icon? This would help with (2).
>
> Refer to existing <gaia-header> element,
>
> ```
> ```html
> <gaia-header action="back" title-start="50" title-end="100">
> <h1>title</h1>
> <button data-icon="add"></button>
> <button data-icon="settings"></button>
> </gaia-header>
> ```
> ```
>
> Which already define data-icon in button element instead of standalone <i>
> element.
>
> we could redefine gaia-button as:
>
> ```
> <button data-icon="" data-icon-end="" data-l10n-id="">
> ```
>
> It means button has icon at start and end.
AFAIK, there're a lot of permutations of possible textual descriptions here:
* {Start icon label} button text {End icon label}
* {Start icon label} {End icon label}
* {Start icon label} button text
* button text {End icon label}
* button text
If data-l10n-id specifies aria-label then it will override everything for the screen reader: inner text and icon content CSS properties. I guess this is author's responsibility to always provide a meaningful label for the screen reader and as long as we have examples showing that it should be reasonable.
>
> ```
> <button data-icon-only="" data-l10n-id="">
> ```
I think this is simply a case of
```
<button data-icon="" data-l10n-id="">
```
>
> It means button only shows icon, but provide localized text for
> ScreenReader. (I'm not sure if icon-only button can works with data-l10n-id
> this way now, but its a proposal)
Yes data-l10n-id supports aria-label see comment 9.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #14)
> Yes data-l10n-id supports aria-label see comment 9.
Sorry comment 13
Assignee | ||
Comment 16•10 years ago
|
||
So to move forward in terms of this bug, I think the scope here is simply:
* updating the documentation that shows only examples with aria-label/data-l10n-id specified for elements that use data-icon (button, i, etc). it fixes both where icon is complementary to text or is standalone (like the button example), aria-label will override both icon content text and inner HTML of the element.
* when icon is present for presentation only, show examples where aria-hidden is used to make it unreachable by the screen reader.
What do you think?
Maybe in cases where icons are used in other components we can have data-icon-presentation="" or something like that that can be interpreted as 'apply aria-hidden="true" here'?
Comment 17•10 years ago
|
||
Thanks for providing related knowledge about data-l10n-id. I wrote the accessibility section of gaia-icon README, to identify `aria-label`, `aria-hidden`, and `{property}.ariaLabel` usage scenarios.
For icons are used in other components, I think each component should has its own treatment for icons (but may follow some conventions that we could figure out in gaia-button or gaia-header)
Attachment #8607949 -
Flags: feedback?(yzenevich)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8607949 [details] [review]
pull request redirect to github
Thanks for this, it is super helpful
Attachment #8607949 -
Flags: feedback?(yzenevich) → feedback+
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #17)
> For icons are used in other components, I think each component should has
> its own treatment for icons (but may follow some conventions that we could
> figure out in gaia-button or gaia-header)
Perhaps we can even add some sort of logic in gaia-component is all of our components inherit from it?
Comment 20•10 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #19)
> Perhaps we can even add some sort of logic in gaia-component is all of our
> components inherit from it?
Since not all components use icon in it, we could review if there is any thing we found its useful from the practice with gaia-button or gaia-header.
Comment 21•10 years ago
|
||
Comment on attachment 8607949 [details] [review]
pull request redirect to github
According the discussion from comment 13 to comment 16, we come out the icon related usage and design treatment. Please helping check it.
Attachment #8607949 -
Flags: review?(wilsonpage)
Comment 22•10 years ago
|
||
Comment on attachment 8607949 [details] [review]
pull request redirect to github
Some nits inline, but looks good.
Attachment #8607949 -
Flags: review?(wilsonpage) → review+
Comment 23•10 years ago
|
||
Comment on attachment 8607949 [details] [review]
pull request redirect to github
LANDED https://github.com/gaia-components/gaia-icons/commit/aac506e760a343701ac6a84b8fd22371e4c8a915
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•10 years ago
|
||
Wondering what you guys think of this approach in addition to the documentation user guide that's been updated for a11y?
Attachment #8614078 -
Flags: feedback?(wilsonpage)
Attachment #8614078 -
Flags: feedback?(gasolin)
Comment 25•10 years ago
|
||
Comment on attachment 8614078 [details] [review]
Proposed warning/diagnostics
I like the approach, but this will require a lot of changes across Gaia as I don't think (m)any apps are using gaia-icons in this way yet.
Attachment #8614078 -
Flags: feedback?(wilsonpage) → feedback+
Assignee | ||
Comment 26•10 years ago
|
||
Reopening to carry out some extra improvements based on the proposed warning highlighting.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•10 years ago
|
Attachment #8614078 -
Flags: review?(wilsonpage)
Comment 27•10 years ago
|
||
Comment on attachment 8614078 [details] [review]
Proposed warning/diagnostics
It looks well to me. I took it into button stress test (added related data-l10n-id) and it goes with no obvious performance impact
9647ms vs 9575ms(with patch)
https://github.com/gaia-components/gaia-button/blob/master/test/stress.html
(myth: I've test twice and the result with patch always better than the origin :-/).
Though in buttons the notification text is relatively large and the description is truncated.
Attachment #8614078 -
Flags: feedback?(gasolin) → feedback+
Comment 28•9 years ago
|
||
Comment on attachment 8614078 [details] [review]
Proposed warning/diagnostics
LANDED https://github.com/gaia-components/gaia-icons/commit/de969d9a6350535e29b80eb2cc2e7c4b4922b1dd
STAMPED v0.9.0 https://github.com/gaia-components/gaia-icons/releases/tag/v0.9.0
Attachment #8614078 -
Flags: review?(wilsonpage) → review+
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8615362 -
Flags: review?(alive)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → yzenevich
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8615474 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8615490 -
Flags: review?(mmedeiros)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8615509 -
Flags: review?(drs)
Updated•9 years ago
|
Attachment #8615490 -
Flags: review?(mmedeiros) → review+
Assignee | ||
Comment 33•9 years ago
|
||
Attachment #8615511 -
Flags: review?(marina.rodrigueziglesias)
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8615525 -
Flags: review?(dkuo)
Assignee | ||
Comment 35•9 years ago
|
||
Assignee | ||
Comment 36•9 years ago
|
||
Attachment #8615534 -
Flags: review?(alive)
Assignee | ||
Comment 37•9 years ago
|
||
Attachment #8615536 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Comment 38•9 years ago
|
||
Attachment #8615548 -
Flags: review?(felash)
Assignee | ||
Comment 39•9 years ago
|
||
Attachment #8615551 -
Flags: review?(felash)
Assignee | ||
Comment 40•9 years ago
|
||
This should lend after all app specific PR's are in.
Attachment #8615561 -
Flags: review?(wilsonpage)
Comment 41•9 years ago
|
||
Comment on attachment 8615509 [details] [review]
Dialer PR
I didn't try this, but it looks fine.
Attachment #8615509 -
Flags: review?(drs) → review+
Assignee | ||
Comment 42•9 years ago
|
||
Comment 43•9 years ago
|
||
Comment on attachment 8615534 [details] [review]
Privacy Panel PR
I am not sure if I could review this. Tim, please help to redirect.
Attachment #8615534 -
Flags: review?(alive) → review?(timdream)
Comment 44•9 years ago
|
||
Comment on attachment 8615362 [details] [review]
System PR
Cool, Michael, please help to review this. Thanks.
Attachment #8615362 -
Flags: review?(alive) → review?(mhenretty)
Comment 45•9 years ago
|
||
Comment on attachment 8615511 [details] [review]
Costcontrol PR
LGTM. Please correct the nit on GitHub.
Attachment #8615511 -
Flags: review?(marina.rodrigueziglesias) → review+
Updated•9 years ago
|
Attachment #8615534 -
Flags: review?(timdream) → review?(etienne)
Comment 46•9 years ago
|
||
Comment on attachment 8615474 [details] [review]
Settings PR
The patch is looking good. The only question I got would be why it is required adding `aria-describedby` on the icons of the menu items on the root panel. The icons are not functional and the screen reader should be able to read the text on the menu items.
Attachment #8615474 -
Flags: review?(arthur.chen)
Comment 47•9 years ago
|
||
Comment on attachment 8615551 [details] [review]
Wappush PR
Looks good but I'll defer to Gabriele because I seldom know this code.
Attachment #8615551 -
Flags: review?(felash) → review?(gsvelto)
Comment 48•9 years ago
|
||
Hey Matej, can you please have a look at my string proposals in https://github.com/mozilla-b2g/gaia/pull/30431/files#r31806953 ? Thanks a lot !
Flags: needinfo?(matej)
Comment 49•9 years ago
|
||
Comment on attachment 8615551 [details] [review]
Wappush PR
Yup, that element should serve no purpose besides being a visual hint IIRC.
Attachment #8615551 -
Flags: review?(gsvelto) → review+
Comment 50•9 years ago
|
||
Comment on attachment 8615534 [details] [review]
Privacy Panel PR
lgtm
Attachment #8615534 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 51•9 years ago
|
||
Assignee | ||
Comment 52•9 years ago
|
||
Comment on attachment 8615474 [details] [review]
Settings PR
(In reply to Arthur Chen [:arthurcc] from comment #46)
> Comment on attachment 8615474 [details] [review]
> Settings PR
>
> The patch is looking good. The only question I got would be why it is
> required adding `aria-describedby` on the icons of the menu items on the
> root panel. The icons are not functional and the screen reader should be
> able to read the text on the menu items.
I left some comments in Github. But to answer your question:
We can't hide the element with data-icon with aria-hidden because is it will hide the element (including its text and sub-tree) from screen reader. I'm actually overriding the label on the link itself which means we replace all of content of the label including the description. To not loose description, I have to use aria-describedby to make sure it is included when the screen reader lands on the menu item.
Attachment #8615474 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 53•9 years ago
|
||
Assignee | ||
Comment 54•9 years ago
|
||
Assignee | ||
Comment 55•9 years ago
|
||
Replied to your comment for SMS PR, thanks!
Flags: needinfo?(felash)
Comment 56•9 years ago
|
||
Comment on attachment 8615362 [details] [review]
System PR
Left one small fix on github, but LGTM. Would be great if we had some tests for this, so that we didn't break these aria labels in the future. But I won't block this bug on it.
Attachment #8615362 -
Flags: review?(mhenretty) → review+
Assignee | ||
Comment 57•9 years ago
|
||
Comment 58•9 years ago
|
||
Replied on github -- I'd also like to wait for Matej's answer before landing :)
Flags: needinfo?(felash)
Comment 59•9 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #52)
> Comment on attachment 8615474 [details] [review]
> Settings PR
>
> (In reply to Arthur Chen [:arthurcc] from comment #46)
> > Comment on attachment 8615474 [details] [review]
> > Settings PR
> >
> > The patch is looking good. The only question I got would be why it is
> > required adding `aria-describedby` on the icons of the menu items on the
> > root panel. The icons are not functional and the screen reader should be
> > able to read the text on the menu items.
>
> I left some comments in Github. But to answer your question:
> We can't hide the element with data-icon with aria-hidden because is it will
> hide the element (including its text and sub-tree) from screen reader. I'm
> actually overriding the label on the link itself which means we replace all
> of content of the label including the description. To not loose description,
> I have to use aria-describedby to make sure it is included when the screen
> reader lands on the menu item.
I see. However, it seems not a good idea adding an l10n id on an element with child elements. Is it possible making screen reader skip the icon if it cannot find an l10n id?
Flags: needinfo?(yzenevich)
Comment 60•9 years ago
|
||
Comment on attachment 8615525 [details] [review]
Music PR
Though I don't know why we didn't add all the shuffle icon ids in bug 1136393, but I am glad we add them now, looks good to me and thanks for working this.
Attachment #8615525 -
Flags: review?(dkuo) → review+
Updated•9 years ago
|
Attachment #8615561 -
Flags: review?(wilsonpage) → review+
Assignee | ||
Comment 61•9 years ago
|
||
Assignee | ||
Comment 62•9 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #59)
> (In reply to Yura Zenevich [:yzen] from comment #52)
> > Comment on attachment 8615474 [details] [review]
> > Settings PR
> >
> > (In reply to Arthur Chen [:arthurcc] from comment #46)
> > > Comment on attachment 8615474 [details] [review]
> > > Settings PR
> > >
> > > The patch is looking good. The only question I got would be why it is
> > > required adding `aria-describedby` on the icons of the menu items on the
> > > root panel. The icons are not functional and the screen reader should be
> > > able to read the text on the menu items.
> >
> > I left some comments in Github. But to answer your question:
> > We can't hide the element with data-icon with aria-hidden because is it will
> > hide the element (including its text and sub-tree) from screen reader. I'm
> > actually overriding the label on the link itself which means we replace all
> > of content of the label including the description. To not loose description,
> > I have to use aria-describedby to make sure it is included when the screen
> > reader lands on the menu item.
>
> I see. However, it seems not a good idea adding an l10n id on an element
> with child elements. Is it possible making screen reader skip the icon if it
> cannot find an l10n id?
Unfortunately it's impossible :(. The css context field can only be hidden with aria-hidden, but if it is applied on the menu item, all of the menu item is going to become invisible to the screen reader. This is unfortunately the problem with Gaia icons atm where if it is not meaningful and needs to be hidden it should have aria-hidden applied to it. But if it has inner content, aria-hidden can not be applied and we should have an aria-label overriding all of the inner content and the icon altogether. Let me know what you think.
Flags: needinfo?(yzenevich) → needinfo?(arthur.chen)
Comment 63•9 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #48)
> Hey Matej, can you please have a look at my string proposals in
> https://github.com/mozilla-b2g/gaia/pull/30431/files#r31806953 ? Thanks a
> lot !
I like the idea of verbs, but I actually think it starts to introduce some complications. Instead of "show options," it could be "view options," which is shift in the user's role in the action.
I also think "compose" sounds a bit technical. I would actually go back to the original set, but use "new message" instead of "compose."
Options
Settings
New message
Call
The one thing that isn't clear to me is the difference between "options" and "settings." Those feel roughly synonymous and I wouldn't know what to expect to find in each.
Hope that helps.
Flags: needinfo?(matej)
Comment 64•9 years ago
|
||
(In reply to Matej Novak [:matej] from comment #63)
> (In reply to Julien Wajsberg [:julienw] from comment #48)
>
> Options
> Settings
> New message
> Call
>
> The one thing that isn't clear to me is the difference between "options" and
> "settings." Those feel roughly synonymous and I wouldn't know what to expect
> to find in each.
>
You're totally right, the "options" button is really showing an "additional actions" menu. In the Inbox, the "actions" menu contains "settings", "select conversations" (to be deleted/marked as read/unread). In a Conversation, the "actions" menu contains "select messages" and "add a subject".
I'm not sure what the right label could be. Maybe "actions" ?
Flags: needinfo?(matej)
Comment 65•9 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #64)
> (In reply to Matej Novak [:matej] from comment #63)
> > (In reply to Julien Wajsberg [:julienw] from comment #48)
> >
> > Options
> > Settings
> > New message
> > Call
> >
> > The one thing that isn't clear to me is the difference between "options" and
> > "settings." Those feel roughly synonymous and I wouldn't know what to expect
> > to find in each.
> >
>
> You're totally right, the "options" button is really showing an "additional
> actions" menu. In the Inbox, the "actions" menu contains "settings", "select
> conversations" (to be deleted/marked as read/unread). In a Conversation, the
> "actions" menu contains "select messages" and "add a subject".
>
> I'm not sure what the right label could be. Maybe "actions" ?
Yeah, I think actions is a better representation of that and helps distinguish it from "Settings."
Flags: needinfo?(matej)
Comment 66•9 years ago
|
||
(In reply to Yura Zenevich [:yzen] from comment #62)
> Unfortunately it's impossible :(. The css context field can only be hidden
> with aria-hidden, but if it is applied on the menu item, all of the menu
> item is going to become invisible to the screen reader. This is
> unfortunately the problem with Gaia icons atm where if it is not meaningful
> and needs to be hidden it should have aria-hidden applied to it. But if it
> has inner content, aria-hidden can not be applied and we should have an
> aria-label overriding all of the inner content and the icon altogether. Let
> me know what you think.
I'd just like to ensure there won't be problems when adding l10n ids on elements with inner content.
Stas, I remember that l10n throws an exception under this condition but I can't find related code anymore. Could you shed some light on this? Thanks!
Flags: needinfo?(arthur.chen) → needinfo?(stas)
Updated•9 years ago
|
Attachment #8615536 -
Flags: review?(squibblyflabbetydoo) → review+
Comment 67•9 years ago
|
||
Comment on attachment 8615548 [details] [review]
SMS PR
r=me with "Actions" instead of "Options".
Thanks Matej and Yura !
Attachment #8615548 -
Flags: review?(felash) → review+
Assignee | ||
Comment 68•9 years ago
|
||
Assignee | ||
Comment 69•9 years ago
|
||
Comment 70•9 years ago
|
||
(In reply to Arthur Chen [:arthurcc] from comment #66)
> Stas, I remember that l10n throws an exception under this condition but I
> can't find related code anymore. Could you shed some light on this? Thanks!
We used to warn about this and then we'd even throw in order to prepare for a change in the behavior which already landed.
Currently, the logic is as follows:
1. if the value of the localization string is null (there's no foo=... where ... might be empty), then l10n.js doesn't touch the subtree of the element and only localizes its attributes,
2. if the value if the localization string is not null, i.e. is a string (possibly an empty one!), we do one of the following things:
2.1. if there's no "<" nor "&" in the string value, we replace the elements textContent. All the subtree will be removed in this case.
2.2. otherwise, we assume the string value of the translation contains HTML which needs to be sanitized. We then try to match HTML elements from the translation with the element in the subtree. The elements in the translation are the authoritative ones: if an element is missing from the translation, it will be removed from source.
Here, #1 applies for translations with no value and aria-* attributes, so everything should work as expected.
Flags: needinfo?(stas)
Assignee | ||
Comment 71•9 years ago
|
||
Arthur, let me know if the PR looks good now, thanks!
Flags: needinfo?(arthur.chen)
Comment 72•9 years ago
|
||
Comment on attachment 8615474 [details] [review]
Settings PR
Thanks for the explanation, stas!
r=me, thanks yura!
Flags: needinfo?(arthur.chen)
Attachment #8615474 -
Flags: review?(arthur.chen) → review+
Assignee | ||
Comment 73•9 years ago
|
||
Assignee | ||
Comment 74•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 76•9 years ago
|
||
Attachment #8670295 -
Flags: review?(wilsonpage)
Comment 77•9 years ago
|
||
Comment on attachment 8670295 [details] [review]
A Small follow up.
https://github.com/gaia-components/gaia-icons/releases/tag/v0.10.2
Attachment #8670295 -
Flags: review?(wilsonpage) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•