Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Description • 10 years ago

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.

	gasolin@mozilla.com
	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!).

	Yura Zenevich [:yzen] (please use "needinfo") 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.

	gasolin@mozilla.com
	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

	gasolin@mozilla.com
	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.

	gasolin@mozilla.com
	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.

	gasolin@mozilla.com
	Comment 6 • 9 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)

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 7 • 9 years ago

Comment on attachment 8605059 [details]
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?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 8 • 9 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.

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 9 • 9 years ago

Depends on proper gaia-icons localization support.

	gasolin@mozilla.com
	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.

	gasolin@mozilla.com
	Comment 11 • 9 years ago

Ooh I found `{property}.ariaLabel` could also support above mentions 2 cases, so its also updated in README :)

	Yura Zenevich [:yzen] (please use "needinfo") 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 ?

	gasolin@mozilla.com
	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.

	Wilson Page [:wilsonpage]
	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?

	Yura Zenevich [:yzen] (please use "needinfo") 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).

	gasolin@mozilla.com
	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

	gasolin@mozilla.com
	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.

	gasolin@mozilla.com
	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?

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 20 • 9 years ago

Comment on attachment 8614558 [details]
pull request redirect to github v2

Left feedback in github. Feel free to request more f?

	Wilson Page [:wilsonpage]
	Comment 21 • 9 years ago

Comment on attachment 8614561 [details]
gaia-icon PR

This link is broken

	gasolin@mozilla.com
	Comment 22 • 9 years ago

Comment on attachment 8614561 [details]
gaia-icon PR

https://github.com/gaia-components/gaia-icons/pull/42

	gasolin@mozilla.com
	Comment 23 • 9 years ago

Sorry for inconvenient. I should double check the link.

	Wilson Page [:wilsonpage]
	Comment 24 • 9 years ago

Comment on attachment 8617063 [details]
gaia-icon PR

Waiting on updated README.

	Wilson Page [:wilsonpage]
	Comment 25 • 9 years ago

Comment on attachment 8614558 [details]
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>

	Staś Małolepszy :stas
	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?

	Wilson Page [:wilsonpage]
	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.

	gasolin@mozilla.com
	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.

	Wilson Page [:wilsonpage]
	Comment 30 • 9 years ago

Comment on attachment 8687372 [details]
Adding more a11y improvements + tests

Seeing a Marionette failure in Travis. Aside from this alllllllll gooood stuff! Happy to land once fixed :)

	Yura Zenevich [:yzen] (please use "needinfo") Reporter
	Comment 31 • 9 years ago

https://github.com/gaia-components/gaia-button/commit/1947d820e3237847efea086a8985095bccd58a92

	Wilson Page [:wilsonpage]
	Comment 33 • 9 years ago

Comment on attachment 8695325 [details]
Re-enabling integration tests.

https://github.com/fxos-components/gaia-button/commit/1a1d12653c757659423bfeb0dd3f6d7a2df92553

	BMO Automation
	Comment 34 • 7 years ago

Firefox OS is not being worked on