914892 - Make switch/checkbox/radio options screen reader friendly

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Eitan Isaacson [:eeejay]]

The current composite widget hack does not fair well for screen readers.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Make checkboxes more screen-reader friendly.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 802667 [details] [diff] [review]
Make checkboxes more screen-reader friendly.

This is pretty extensive, so maybe a round of feedback first?
The purpose of these changes are two-fold:

1. Correctly label the checkboxes by:
 (a) Removing the implicit role of the <label> with role="presentation".
 (b) Associating the actual label with the <input> via aria-labelledby

2. Re-sizing the visibly hidden input elements to take up their parent's entire space. This makes the actual checkbox have same geometry as the label, and thus gives better screen reader behavior both for output, by drawing the highlight around the entire option, and input, giving a large explore-by-touch target.

Comment 3

[Marco Zehe (:MarcoZ)]

Comment on attachment 802667 [details] [diff] [review]
Make checkboxes more screen-reader friendly.

What is the reason many of these elements are html:a elements? Do they actually link somewhere apart from the actual checkbox, or is this just to make a bigger touch target?

If it is the latter, I believe the same could be accomplished with much less complicated markup, by just turning these into regular html:label elements with the @for attribute pointing to the id of the associated checkbox, and instead of wrapping things in a label, wrap them in a div or span or something.

If, however, the link actually points somewhere by itself, then this approach is definitely correct. I just noticed that some are not html:a elements, but html:span instead.

If we could go with the alternative approach, that would also get rid of the aria-labelledby and role presentation needs. It wouldn't make the patch smaller, but it wold make the semantic HTML a bit saner. :)

Comment 4

[Fabien Cazenave [:kaze]]

We use <a> elements most of the time. Sometimes they have an @href attribute and really point to some other panel, sometimes they don’t. We left <a> elements in all cases in order not to make the CSS selectors even more complex.

As you mentioned, replacing all <a> elements without @href attributes by <label> elements would be a rather big change, so I wouldn’t recommend it unless it’s actually required for accessibility.

However, if someone proposes an alternative markup to handle our current use cases better without adding complexity, I’d be happy to consider it. To make it clear: I think our CSS selectors are super-ugly, and everything that can help simplifying them will be much appreciated.

Comment 5

[Fabien Cazenave [:kaze]]

Comment on attachment 802667 [details] [diff] [review]
Make checkboxes more screen-reader friendly.

Looks OK to me if Marco confirms we can go on with these <a> elements that don’t have any @href attributes.

I’ve set “f-” because we should not land this without modifying the reference markup in the shared section as well — see shared/style/switches/index.html

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Fabien Cazenave [:kaze] from comment #4)
> We use <a> elements most of the time. Sometimes they have an @href attribute
> and really point to some other panel, sometimes they don’t. We left <a>
> elements in all cases in order not to make the CSS selectors even more
> complex.

Fabien, would you mind jumping in on the related thread on dev-gaia titled "proposed shared switch/checkbox/radio changes"?

Comment 7

[Eitan Isaacson [:eeejay]]

There is another approach I would like to try, as I outlined in the thread on gaia-dev[1]. Once it is ready, I'll flag both Fabien and Arnou as reviewers, since it is pretty extensive.

1. https://groups.google.com/d/msg/mozilla.dev.gaia/o1JfAbT_xBI/1T3Xp2XhaEoJ

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Make pack checkbox/radio/switch use span:after so we could use the span for something useful.

Comment 9

[Eitan Isaacson [:eeejay]]

Attachment: [Settings] Move checkbox labels inside of labels.

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 804070 [details] [diff] [review]
Make pack checkbox/radio/switch use span:after so we could use the span for something useful.

This patch should not really do anything noticeable, but it potentially allows using the <span> for other things besides just the switch/radio/checkbutton state.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 804071 [details] [diff] [review]
[Settings] Move checkbox labels inside of labels.

This is a different approach from before (and yes, we get rid of the anchors here :)

Along with the previous patch to the shared css, this puts the actual text inside the labels and reduces markup complexity. Associating labels with aria-labelledby gets really messy when it is a huge document like in settings where you need to make up a unique id for every single control.

I did some testing, and fixed most cases where things broke:
 1. Put <small> descriptions in the label, and changed the selector for the span to "~". Ultimately the <small> should follow the span, like I outlined in bug #914941.
 2. Made the labels position relative so that elements correctly flow after it, like the <p> tag in the bluetooth switch.
 3. Tweaked the button style slightly to account for an artifact that resulted from some clipping from chaning the label style.

Comment 12

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

Comment on attachment 804070 [details] [diff] [review]
Make pack checkbox/radio/switch use span:after so we could use the span for something useful.

Review of attachment 804070 [details] [diff] [review]:
-----------------------------------------------------------------

f+. Just out of curiosity, in lists BB you are replacing input + span by input + span:after, and in switches BB by input ~ span:after. I understand ~ could allow us having an element between the input and the span, and the span would still have the correct background, but is there any particular reason you do it in one place and not in the other? Could we be consistent? ;) 
It is great to see with that small improvement it works for screen readers and doesn't break our apps' markup!
Thanks.

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

Pointer to Github pull-request

Comment 14

[Eitan Isaacson [:eeejay]]

Comment on attachment 809684 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

I have had these patches here in a local branch for a while, and they seem to work good. I fixed the inconsistencies Arnau saw.

Comment 15

[Eitan Isaacson [:eeejay]]

Hey Evelyn,

This has been waiting for review for over a month. Is this something you could review? Just asking before I [r?] you :)

Comment 16

[Evelyn Hung]

I can help, but I'm not so good at CSS so I may need more time to read the whole story and figure out what's going on.

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 809684 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

I think it makes more sense to have Arnau r? this patch since he knows Building blocks much better than me. He is also seating next to me today and he is fine to r? it :)

Thanks for the patch Eitan.

Comment 18

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

Attachment: active state in bluetooth

Eitan, your proposal looks ok to me to improve accessibility, but I've found a couple of markup issues like the attached screen captures. It would be good if we could fix that before landing this patch, not to cause regressions.

Comment 19

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

Attachment: label in checkbox

Eric, could you tell us if this label should be truncated or shown in two lines of text?

Comment 20

[Eric Pang [:epang] UX]

(In reply to Arnau March from comment #19)
> Created attachment 830156 [details]
> label in checkbox
> 
> Eric, could you tell us if this label should be truncated or shown in two
> lines of text?

Hi Arnau, this is something that should be shown in 2 lines, which will be address with bug 908309.

Comment 21

[Eitan Isaacson [:eeejay]]

(In reply to Arnau March from comment #18)
> Eitan, your proposal looks ok to me to improve accessibility, but I've found
> a couple of markup issues like the attached screen captures. It would be
> good if we could fix that before landing this patch, not to cause
> regressions.

I fixed the ellipses issue, and I am going to push a rebased version in a second. the :active issue does not seem to be a regression in this branch. I am pretty sure I see the same issue in master too.

Comment 22

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

Eitan, you are right, that happens in master too. r+

Comment 23

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

Comment on attachment 809684 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

Please squash your commits before merging.

Comment 24

[Eitan Isaacson [:eeejay]]

https://github.com/mozilla-b2g/gaia/commit/f60bf05ef559ade0f9a7088d386586b9f80aaea7

Tweaked some tests to make Travis happy.

Comment 25

[Jason Smith [:jsmith]]

Lots of VD regressions from this patch. This needs to be backed out.

John - Can you back this out?

Comment 26

[Eitan Isaacson [:eeejay]]

Can we perhaps fix the regressions instead of backing this out? It is a big change, and it is impossible for me to identify all the VD regressions without some help..

Comment 27

[Jason Smith [:jsmith]]

(In reply to Eitan Isaacson [:eeejay] from comment #26)
> Can we perhaps fix the regressions instead of backing this out? It is a big
> change, and it is impossible for me to identify all the VD regressions
> without some help..

Fair enough - but we shouldn't let that regression sit around too long.

Comment 28

[Jason Smith [:jsmith]]

I'm wondering if this patch also caused bug 939070.

Comment 29

[Kevin Grandon :kgrandon]

I also found some more regressions here which I will file. We definitely need better review cycles in the future. Recommending a patch for each app with a review from a peer/owner for future work.

Comment 30

[Kevin Grandon :kgrandon]

(In reply to Kevin Grandon :kgrandon from comment #29)
> I also found some more regressions here which I will file. We definitely
> need better review cycles in the future. Recommending a patch for each app
> with a review from a peer/owner for future work.

Actually, I'm not sure if this would've helped as this was for shared/ work (no calendar changes). I guess we just need to double-check all touch points for shared/ code before landing.

Comment 31

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

guys, I have started a meta bug for fixing layout structure: https://bugzilla.mozilla.org/show_bug.cgi?id=940353
As settings app has so many list which do no scale from a css point of view, I wanted to refactor it using flex box.
We faced the same issue Eitan had, having to create a huge patch, so we decided to split them into screens.

Eitan, in case we need to back this out, maybe Joan Leon (who works with me) and myself could address all accessibility issues screen by screen.

And if we don't back out we could fix all regression caused this patch.

In both cases we won't have time to have it ready for 1.3.

WDYT?