Closed
Bug 1162138
Opened 10 years ago
Closed 7 years ago
[accessibility][gaia-button] Make sure gaia-button is accessible to screen reader.
Categories
(Firefox OS Graveyard :: Gaia::Components, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: yzen, Unassigned)
References
Details
(Keywords: access)
Attachments
(6 files, 1 obsolete file)
54 bytes,
text/x-github-pull-request
|
Details | Review | |
52 bytes,
text/plain
|
wilsonpage
:
feedback+
|
Details |
54 bytes,
text/x-github-pull-request
|
Details | Review | |
53 bytes,
text/x-github-pull-request
|
Details | Review | |
54 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
|
Details | Review |
54 bytes,
text/x-github-pull-request
|
wilsonpage
:
review+
|
Details | Review |
Several issues found: * Disabled button state is not indicated to the screen reader * Ensure buttons that have icons are properly localized: just-icon-buttons need to have a good localized label; buttons that include an icon and a text need to have a single atomic textual description.
Comment 1•10 years ago
|
||
Thanks for start firing accessibility related bugs! After read http://marcysutton.github.io/accessibility-of-web-components/slides.html to learn 101 about web component accessibility I found current gaia-button is not extended from the button(native element), and the base `gaia-component` module does not support that (we could fix it!).
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #1) > Thanks for start firing accessibility related bugs! > > After read > http://marcysutton.github.io/accessibility-of-web-components/slides.html to > learn 101 about web component accessibility > > I found current gaia-button is not extended from the button(native element), > and the base `gaia-component` module does not support that (we could fix > it!). Yeah, lots of semantic issues with gaia-components can be fixed if we inherit from existing elements, including switches, etc.
Comment 3•10 years ago
|
||
After some experiment I found current gecko seems not support `extends: 'button'` option in document.registerElement. After checking gaia-button code, it use `role="button"` to ask browser treat it as a button. https://github.com/gaia-components/gaia-button/blob/master/gaia-button.js#L17 So we may apply some tricks from https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_button_role
Comment 4•10 years ago
|
||
add guide in https://github.com/gaia-components/gaia-button/pull/9 Could fix following issue: * buttons that include an icon and a text need to have a single atomic textual description.
Comment 5•10 years ago
|
||
Read http://www.filamentgroup.com/lab/bulletproof_icon_fonts.html If just-icon-buttons need to have a good localized label, we could come with a Fallback text span tag.
Comment 6•10 years ago
|
||
I've automate the aria treatment so devs don't have to manually add aria- attributes. gaia-button will add them automatically. @yzen could you help check if buttons work properly on ScreenReader? (In my test, the button disabled state is speaken, and the button with icon&text will only read the text) demo page http://gaia-components.github.io/gaia-button/ (the patch and the demo did not include the standalone icon alternative text. It could be done later once we have a good solution)
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8605059 [details] [review] pull request redirect to github Hi Fred, overall it looks good , thanks for taking care of disabled state as well. I only have 1 suggestion to make it more bulletproof: There are 2 use cases with icons in buttons (or rather icons in general): decorative, and otherwise. With the solution you have we make an assumption that if there are more than 1 child elements and there's an icon, than it should be hidden from screen reader. It falls short in cases where there are just 2 icons without any text, or where the icon complements text in a meaningful way for example: * '★ this comment' * '♥ location' So, may I suggest the following solution: * Ensure that we have proper localization for the gaia icons, independent of their use inside a button * Have an explicit attribute for icons used inside a button that indicate that they are there for decoration only and that would imply that we need to apply aria-hidden="true" for them. What do you think?
Attachment #8605059 -
Flags: feedback?(yzenevich)
Reporter | ||
Comment 8•10 years ago
|
||
So to add to the above comment, I think the only work that gaia button should do is to check for the attribute on the icon element and if it's there, hide it from screen reader. I already opened a separate bug concerning icons so it can be handled there.
Reporter | ||
Comment 9•10 years ago
|
||
Depends on proper gaia-icons localization support.
Depends on: 1162040
Comment 10•9 years ago
|
||
I've referenced above comments and icon proposal to come out the new gaia-button syntax proposal, which remove the <i> tag inside the button and cover all use cases. The gist shows what new gaia-button README looks like. When feedbacks are positive, we can do actual coding then. PS: For practice I think currently gaia does not have "no text and 2 icons in button/with meaningful icon" scenario, so I list these cases but not tend to support theses use case at this stage.
Attachment #8609977 -
Flags: feedback?(yzenevich)
Attachment #8609977 -
Flags: feedback?(wilsonpage)
Comment 11•9 years ago
|
||
Ooh I found `{property}.ariaLabel` could also support above mentions 2 cases, so its also updated in README :)
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8609977 [details]
button syntax proposal
It looks pretty good. The only confusing part is the sentence describing 'meaningful icon' that says it can not be read. It will be read but it will read un-localized CSS content field. Perhaps it worth saying that ?
Attachment #8609977 -
Flags: feedback?(yzenevich)
Comment 13•9 years ago
|
||
Thanks for suggestion! After second thought I try to add explanation about what meaningful icon is instead of describe what will happen when we did not provide the proper treatment.
Comment 14•9 years ago
|
||
Comment on attachment 8609977 [details]
button syntax proposal
Looks cleaner :)
Are you sure the CSS `content` string will never be read by the screen reader if an `arialabel` is present?
Attachment #8609977 -
Flags: feedback?(wilsonpage) → feedback+
Reporter | ||
Comment 15•9 years ago
|
||
(In reply to Wilson Page [:wilsonpage] from comment #14) > Comment on attachment 8609977 [details] > button syntax proposal > > Looks cleaner :) > > Are you sure the CSS `content` string will never be read by the screen > reader if an `arialabel` is present? Yes, the screen reader will read aria-label as a the name of the control that has button semantics (either via role button or inherited from button element).
Comment 16•9 years ago
|
||
As zibi reminds l10n in dev-gaia[1], we'll take this concern into revised proposal. Have data- attribute in specific element do help us identify what element should apply the certain style. So the result format might be looks like: ``` <gaia-button> <span data-icon="ok" data-l10n-id="button_ok"/> </gaia-button> ``` [1] https://groups.google.com/forum/#!topic/mozilla.dev.gaia/74TTKxLjoLA
Comment 17•9 years ago
|
||
I made a PR with proposed syntax. The UI looks fine but I don't know if l20n.js is ready(in bower it's 1.0.x version) for parse data-l10n-id in gaia-external project yet. So the ScreenReader will still read "forward forward button" instead of "forward". I'll give l20n a try and see how it goes.
Attachment #8614558 -
Flags: feedback?(yzenevich)
Attachment #8614558 -
Flags: feedback?(wilsonpage)
Comment 18•9 years ago
|
||
Attachment #8614561 -
Flags: review?(wilsonpage)
Comment 19•9 years ago
|
||
Hi stas, I'm trying add l20n.js into gaia-button example to test with ScreenReader support, not sure if l20n.js 1.0.2 (which can be installed from bower) support the .ariaLabel?
Flags: needinfo?(stas)
Reporter | ||
Comment 20•9 years ago
|
||
Comment on attachment 8614558 [details] [review] pull request redirect to github v2 Left feedback in github. Feel free to request more f?
Attachment #8614558 -
Flags: feedback?(yzenevich)
Comment 21•9 years ago
|
||
Comment on attachment 8614561 [details]
gaia-icon PR
This link is broken
Attachment #8614561 -
Flags: review?(wilsonpage)
Comment 22•9 years ago
|
||
Comment on attachment 8614561 [details] gaia-icon PR https://github.com/gaia-components/gaia-icons/pull/42
Comment 23•9 years ago
|
||
Sorry for inconvenient. I should double check the link.
Attachment #8614561 -
Attachment is obsolete: true
Attachment #8617063 -
Flags: review?(wilsonpage)
Comment 24•9 years ago
|
||
Comment on attachment 8617063 [details] [review] gaia-icon PR Waiting on updated README.
Attachment #8617063 -
Flags: review?(wilsonpage)
Comment 25•9 years ago
|
||
Comment on attachment 8614558 [details] [review] pull request redirect to github v2 If everyone is in agreement, we need to update examples to less verbose syntax. FROM: <gaia-button><span data-icon="icon"></span><span data-l10n-id="foo">Text</span></gaia-button> TO: <gaia-button data-icon="icon" data-l10n-id="foo"></gaia-button>
Attachment #8614558 -
Flags: feedback?(wilsonpage)
Comment 26•9 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #19) > Hi stas, I'm trying add l20n.js into gaia-button example to test with > ScreenReader support, not sure if l20n.js 1.0.2 (which can be installed from > bower) support the .ariaLabel? Hi Fred, I wouldn't use 1.0.x at this point for this. We're very close to landing v3.x in Gaia's shared/js and that would get much more support in the future than 1.0.x. I think next week is a reasonable ETA for the landing. (In reply to Wilson Page [:wilsonpage] from comment #25) > TO: <gaia-button data-icon="icon" data-l10n-id="foo"></gaia-button> Just to make sure I understand this correctly: l10n.js will insert the translation as the textContent of the gaia-button element, which will be projected into the <content> element of the shadow DOM. Is that correct?
Flags: needinfo?(stas)
Comment 27•9 years ago
|
||
(In reply to Staś Małolepszy :stas from comment #26) > > TO: <gaia-button data-icon="icon" data-l10n-id="foo"></gaia-button> > > Just to make sure I understand this correctly: l10n.js will insert the > translation as the textContent of the gaia-button element, which will be > projected into the <content> element of the shadow DOM. Is that correct? Correct.
Comment 28•9 years ago
|
||
Thanks for feedback. I saw the button component for smart TV come with the identical syntax http://smart-components.github.io/demo/ <smart-button type="text-and-icon" data-icon="launch">Icon Button</smart-button> So I might refer some parts from that component.
Updated•9 years ago
|
Status: ASSIGNED → NEW
Reporter | ||
Comment 29•9 years ago
|
||
Attachment #8687372 -
Flags: review?(wilsonpage)
Comment 30•9 years ago
|
||
Comment on attachment 8687372 [details] [review] Adding more a11y improvements + tests Seeing a Marionette failure in Travis. Aside from this alllllllll gooood stuff! Happy to land once fixed :)
Attachment #8687372 -
Flags: review?(wilsonpage) → review+
Reporter | ||
Comment 31•9 years ago
|
||
https://github.com/gaia-components/gaia-button/commit/1947d820e3237847efea086a8985095bccd58a92
Reporter | ||
Comment 32•9 years ago
|
||
Attachment #8695325 -
Flags: review?(wilsonpage)
Reporter | ||
Updated•9 years ago
|
Attachment #8695325 -
Flags: review?(wilsonpage)
Comment 33•9 years ago
|
||
Comment on attachment 8695325 [details] [review] Re-enabling integration tests. https://github.com/fxos-components/gaia-button/commit/1a1d12653c757659423bfeb0dd3f6d7a2df92553
Attachment #8695325 -
Flags: review+
Updated•9 years ago
|
Assignee: gasolin → nobody
Comment 34•7 years ago
|
||
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•