If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make switch/checkbox/radio options screen reader friendly

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
The current composite widget hack does not fair well for screen readers.
(Assignee)

Comment 1

4 years ago
Created attachment 802667 [details] [diff] [review]
Make checkboxes more screen-reader friendly.
(Assignee)

Comment 2

4 years ago
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.
Attachment #802667 - Flags: feedback?(kaze)

Comment 3

4 years ago
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. :)
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 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
Attachment #802667 - Flags: feedback?(kaze) → feedback-

Comment 6

4 years ago
(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"?
(Assignee)

Comment 7

4 years ago
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
(Assignee)

Comment 8

4 years ago
Created attachment 804070 [details] [diff] [review]
Make pack checkbox/radio/switch use span:after so we could use the span for something useful.
(Assignee)

Comment 9

4 years ago
Created attachment 804071 [details] [diff] [review]
[Settings] Move checkbox labels inside of labels.
Attachment #802667 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
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.
Attachment #804070 - Flags: feedback?(arnau)
(Assignee)

Comment 11

4 years ago
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.
Attachment #804071 - Flags: feedback?(kaze)
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.
Attachment #804070 - Flags: feedback?(arnau) → feedback+
(Assignee)

Updated

4 years ago
Blocks: 916231
(Assignee)

Comment 13

4 years ago
Created attachment 809684 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

Pointer to Github pull-request
(Assignee)

Comment 14

4 years ago
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.
Attachment #809684 - Flags: review?(kaze)
(Assignee)

Updated

4 years ago
Attachment #804070 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #804071 - Attachment is obsolete: true
Attachment #804071 - Flags: feedback?(kaze)
(Assignee)

Comment 15

4 years ago
Hey Evelyn,

This has been waiting for review for over a month. Is this something you could review? Just asking before I [r?] you :)
Flags: needinfo?(ehung)
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.
Flags: needinfo?(ehung)
(Assignee)

Updated

4 years ago
Attachment #809684 - Flags: review?(kaze)
Attachment #809684 - Flags: review?(ehung)
Attachment #809684 - Flags: review?(21)
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.
Attachment #809684 - Flags: review?(ehung)
Attachment #809684 - Flags: review?(arnau)
Attachment #809684 - Flags: review?(21)
Created attachment 830152 [details]
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.
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?
Flags: needinfo?(epang)
(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.
Flags: needinfo?(epang)
(Assignee)

Comment 21

4 years ago
(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.
Eitan, you are right, that happens in master too. r+
Comment on attachment 809684 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12418

Please squash your commits before merging.
Attachment #809684 - Flags: review?(arnau) → review+
(Assignee)

Comment 24

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

Tweaked some tests to make Travis happy.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 938943
Lots of VD regressions from this patch. This needs to be backed out.

John - Can you back this out?
Flags: needinfo?(jhford)
(Assignee)

Comment 26

4 years ago
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..
(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.
Flags: needinfo?(jhford)
I'm wondering if this patch also caused bug 939070.

Updated

4 years ago
Depends on: 939070

Updated

4 years ago
Depends on: 939307

Updated

4 years ago
Depends on: 938931
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.
Depends on: 939849
(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.
(Assignee)

Updated

4 years ago
Depends on: 938846

Updated

4 years ago
Depends on: 940633
(Assignee)

Updated

4 years ago
Depends on: 941142

Updated

4 years ago
Depends on: 941967
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?

Updated

4 years ago
Depends on: 943123
Depends on: 943176
Depends on: 943701

Updated

4 years ago
Depends on: 943880

Updated

4 years ago
No longer depends on: 943880

Updated

4 years ago
Depends on: 947028

Updated

4 years ago
Depends on: 947032

Updated

4 years ago
Depends on: 947472

Updated

4 years ago
No longer depends on: 947472

Updated

4 years ago
Depends on: 949718

Updated

4 years ago
No longer depends on: 949718
(Assignee)

Updated

4 years ago
Depends on: 951123
Depends on: 942618
You need to log in before you can comment on or make changes to this bug.