1074224 - [Accessibility] Get rid of [role="button"] in CSS everywhere.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Roles must not be used as CSS selectors. If they are, they are a subject of misuse, or in case when the role needs to be applied in markup, the styling comes with it, when it should not.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Github pull request.

Marking the following reviewers for these parts:
Alive - System app
Ismael - Building blocks
Doug - Communications
Julien - SMS
Arthur - Settings
Kevin - Calendar
Eric - Bluetooth

Comment 2

[Julien Wajsberg [:julienw]]

Comment on attachment 8498236 [details] [review]
Github pull request.

Redirecting to Oleg because I'm away for some days

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8498236 [details] [review]
Github pull request.

Calendar part looks fine to me. Thanks.

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498236 [details] [review]
Github pull request.

Dialer part LGTM. I'm redirecting the Contacts part to Francisco as I'm not a Contacts peer.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8498236 [details] [review]
Github pull request.

Redirect the review to Ian Liu, Bluetooth app owner, since this PR is not related to Bluetooth protocol or logic.

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8498236 [details] [review]
Github pull request.

EJ, could you help review the patch? Thanks.

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8498236 [details] [review]
Github pull request.

Thanks Yura,

You forgot to change one more line here : https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/settings_large.css#L162

Please remember to update the selector on that file. 

r+

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Github pull request.

Carrying over r+ from alive, drs, ejchen, kgrandon,

Comment 9

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8498866 [details] [review]
Github pull request.

Redirecting to Jose, since I won't be able to take a look to this quickly.

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8498866 [details] [review]
Github pull request.

Thanks Yura! r=me with one nit on GitHub.

Comment 11

[[e-mail], [e-mail]]

Comment on attachment 8498866 [details] [review]
Github pull request.

After applied the patch, the button displays normally in Bluetooth app. r+ with me

Comment 12

[Jose Manuel Cantera]

Comment on attachment 8498866 [details] [review]
Github pull request.

can we have an independent PR for the changes in communications?

Comment 13

[Jose Manuel Cantera]

Comment on attachment 8498866 [details] [review]
Github pull request.

Ismael is no longer working on the project ...

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Jose Manuel Cantera from comment #12)
> Comment on attachment 8498866 [details] [review]
> Github pull request.
> 
> can we have an independent PR for the changes in communications?

Yes this is why the PR was split by commits by application, see:
https://github.com/yzen/gaia/commit/e03cd419117c749102f8a422c23c9c50c0f8ed3d

Comment 15

[Jose Manuel Cantera]

(In reply to Yura Zenevich [:yzen] from comment #14)
> (In reply to Jose Manuel Cantera from comment #12)
> > Comment on attachment 8498866 [details] [review]
> > Github pull request.
> > 
> > can we have an independent PR for the changes in communications?
> 
> Yes this is why the PR was split by commits by application, see:
> https://github.com/yzen/gaia/commit/e03cd419117c749102f8a422c23c9c50c0f8ed3d

Yes but I think it would be better to have it in an independent PR. We did it for other similar patches such as those related to L10N.

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Github pull request for communications app.

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8498866 [details] [review]
Github pull request.

Marking for review for the Building Blocks part.

Comment 18

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8498866 [details] [review]
Github pull request.

Kaze is gone. Moving the BB review to Arnau.

Comment 19

[Jose Manuel Cantera]

Comment on attachment 8499613 [details] [review]
Github pull request for communications app.

r=me for the contacts part.

German,

Please could you have a look at the dialer part?

thanks

Comment 20

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8499613 [details] [review]
Github pull request for communications app.

Germán is not a dialer peer, and I've already reviewed this. See comment 4 and comment 8.

Comment 21

[Jose Manuel Cantera]

(In reply to Doug Sherk (:drs) from comment #20)
> Comment on attachment 8499613 [details] [review]
> Github pull request for communications app.
> 
> Germán is not a dialer peer, and I've already reviewed this. See comment 4
> and comment 8.
then that patch can be landed

Comment 22

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8498866 [details] [review]
Github pull request.

Sorry guys it took me so long to answer, but my gaia is not working properly and need to clone it again.
Without trying it in a phone the patch looks good.
But please, before landing could you test a case I'm not sure you are covering?
In some apps, for instance settings/languages they are using "fake selects" using a span.button:
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/elements/languages.html#L12
That's why in buttons BB we have 3 cases when applying rules:
button, [role="button"] and .button
if you are replacing .button with .bb-button, you would need to scan all class="* button *" in all apps before we make sure nothing breaks.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Arnau March  [:arnau] from comment #22)
> Comment on attachment 8498866 [details] [review]
> Github pull request.
> 
> Sorry guys it took me so long to answer, but my gaia is not working properly
> and need to clone it again.
> Without trying it in a phone the patch looks good.
> But please, before landing could you test a case I'm not sure you are
> covering?
> In some apps, for instance settings/languages they are using "fake selects"
> using a span.button:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/elements/
> languages.html#L12
> That's why in buttons BB we have 3 cases when applying rules:
> button, [role="button"] and .button
> if you are replacing .button with .bb-button, you would need to scan all
> class="* button *" in all apps before we make sure nothing breaks.

Hi Arnau, I am only removing cases like [role="button"] so it looks like it's all good.

Comment 24

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/80883eb40c12b04fa571c384a4b03afac09fc817
https://github.com/mozilla-b2g/gaia/commit/9073d27596ac507e11cdafec1d71f42d9a3f0259