1349835 - Add macros to ensure that accessibles are created for checkboxes with -moz-appearance: none,

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

I believe since bug 605985 landed, checkboxes that have (-moz-)appearance: none are not recognized correctly by our accessibility service, and so we don't create the right accessible elements. These checkboxes effectively become invisible.

There are a few places in our in-content UI's where we have specially styled checkboxes, which use appearance: none to bypass the native styling. For those, we added role="checkbox" in bug 1339775 so that we created the right accessibles. This, however, doesn't fix the larger problem where checkboxes on the web might have appearance: none, and are suddenly no longer visible via the accessibility API.

surkov laid out a solution in bug 1339775 comment 17, and this bug is to track that work. I'm going to copy surkov's plan verbatim:

-- snip --

1) add HasAttr(attr, value) macros similar to Attr and other macroses [1]
2) make sure to check attr and value when accessible is created [2]
3) change markup macros for input@type='checkbox' like
MARKUPMAP(
  nsGkAtoms::input,
  HasAttr(nsGkAtoms::type, nsGkAtoms::checkbox),
  // rest part
);

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/base/nsAccessibilityService.cpp?q=MARKUPMAP&redirect_type=direct#248
[2] https://dxr.mozilla.org/mozilla-central/source/accessible/base/nsAccessibilityService.cpp?q=MARKUPMAP&redirect_type=direct#1096

-- snip --

Once this bug is fixed, the role="checkbox" attributes added in bug 1339775 can be removed.

Comment 1

[Mats Palmgren (inactive)]

What do other browsers do in the appearance:none case?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Mats Palmgren (:mats) from comment #1)
> What do other browsers do in the appearance:none case?

As far as I know there is no spec for this right?

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco can you find out what other browsers do here?

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to Mats Palmgren (:mats) from comment #1)
> What do other browsers do in the appearance:none case?

I tested Chrome, Edge, and IE, and in all cases, the checkboxes worked as expected without any difference to other checkboxes. It's only us who do it wrong. :-(

Comment 5

[Marco Zehe (:MarcoZ)]

To be clear, this includes checking and unchecking them with the space bar, which appears to work in Firefox as well, at least visually. In all the above other browsers, I can toggle them normally.

Comment 6

[Mats Palmgren (inactive)]

I think you're right that this is a regression from bug 605985, so it would be good to fix this for v54.
The suggested fix in comment 0 seems like the right way to fix this.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Any chance anyone on your team has cycles for this, davidb, before this regression goes out in 54?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Mike Conley (:mconley) from comment #7)
> Any chance anyone on your team has cycles for this, davidb, before this
> regression goes out in 54?

We're down to two engineers for a while, working on top crashers and e10s a11y support...  Alex is this something you could fit into your queue?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #8)
> (In reply to Mike Conley (:mconley) from comment #7)
> > Any chance anyone on your team has cycles for this, davidb, before this
> > regression goes out in 54?
> 
> We're down to two engineers for a while, working on top crashers and e10s
> a11y support...  Alex is this something you could fit into your queue?

the queue has the cardinality of the continuum, but unfortunately I don't anticipate it happening anytime soon if being realistic

Comment 10

[Marco Zehe (:MarcoZ)]

I can try a stab at this next week.

Comment 11

[Marco Zehe (:MarcoZ)]

I am stuck on trying to understand Surkov's instructions. Part of it is the inaccessibility of DXR, which is documented in bug 676875, and which was not dealt with before retiring MXR. So, I have no chance of using the tools I need to complete this work. If somebody else who is more fluent in C++ than I am wants to pick this up, be my guest. I'm happy to review and test if somebody picks it up.

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to Marco Zehe (:MarcoZ) from comment #11)
> I am stuck on trying to understand Surkov's instructions. Part of it is the
> inaccessibility of DXR, which is documented in bug 676875, and which was not
> dealt with before retiring MXR. So, I have no chance of using the tools I

dxr url include file and line number so its pretty easy to look at the sited location with $editor.

Comment 13

[Nathan Froyd [:froydnj]]

Nice-to-have for 54, but it's unlikely it'll make it in.

Comment 15

[James Teh [:Jamie]]

This is a lot worse in the wild after bug 1333482, which ships the unprefixed "appearance". It breaks the accessibiliy of all check boxes in Wordpress, for example.

Comment 16

[Mats Palmgren (inactive)]

FYI, bug 1333482 was backed out in bug 1365614 (it will also be backed out in v54).

Comment 17

[Mats Palmgren (inactive)]

(In reply to Mike Conley (:mconley) from comment #0)
> 3) change markup macros for input@type='checkbox' like
> MARKUPMAP(
>   nsGkAtoms::input,
>   HasAttr(nsGkAtoms::type, nsGkAtoms::checkbox),
>   // rest part
> );

mMarkupMaps is a hashtable indexed by the tag name, so I don't see how we can
have multiple entries for nsGkAtoms::input.  (We would need one for checkbox
and one for radio, IIUC)

Comment 18

[Mats Palmgren (inactive)]

Attachment: WIP

Something like this might work for now. (it compiles, but I haven't tested it)

Marco, can you take it from here?

Comment 19

[Marco Zehe (:MarcoZ)]

Mats, I am currently dealing with a broken build system on Windows. Are you in a position to kick off a try build with this WIP from your end? I could then more easily test that build and then take it from there (add tests in the ideal case).

Comment 20

[Mats Palmgren (inactive)]

Sure, here you go:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ee44643b4a67b194b0af17bf4baa2e35985550f

Comment 21

[Marco Zehe (:MarcoZ)]

Yes, this one works! My local test file now renders the checkbox correctly that has appearance:none; set, and also fires the correct events when toggling.

I have a long weekend coming up, so if anyone wants to tackle the test for this in the interim, I'd be happy to review. If not, I'll write one on Monday. Mats, thanks for picking this up!

Comment 22

[Marco Zehe (:MarcoZ)]

Additional info: A test is probably as simple as testing a checkbox with appearance:none; in the same place we test accessibles for a regular input @type="checkbox" in our Mochitests. Don't think we have the browser test equivalent for that yet.

Comment 23

[Mats Palmgren (inactive)]

Attachment: fix

I think this is good enough as a temporary solution at least.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 8873022 [details] [diff] [review]
fix

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

thank you for fixing it, it'd be nice to add a simple test, for example, into [1]

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/role/test_general.html

::: accessible/base/MarkupMap.h
@@ +100,5 @@
>            roles::LABEL)
>  
> +MARKUPMAP(input,
> +          New_HTMLInput,
> +          0)

the list is in alphabetical order, please move it before label

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6012cad03e
Create accessibles for <input type=checkbox/radio> with -moz-appearance:none.  r=surkov

Comment 26

[Mats Palmgren (inactive)]

> the list is in alphabetical order, please move it before label

Fixed.

> it'd be nice to add a simple test

I'm leaving that for Marco since promised to write one :-)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc6012cad03e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Can this ride the 55 train or should we try to uplift to 54 still? The RC build is happening on Monday, FWIW.

Comment 29

[Mats Palmgren (inactive)]

I'm not very familiar with the a11y code so I can't say what the risk is.
I'll that decision to Alexander and Marco.

Comment 30

[Marco Zehe (:MarcoZ)]

Attachment: Test for checkboxes with and without appearance: none;

Comment 31

[Mats Palmgren (inactive)]

Marco, I think appearance:none in the test should use -moz-appearance instead.
FYI, we backed out support for the unprefixed appearance and -webkit-appearance for now (bug 1365614).

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 8873413 [details] [diff] [review]
Test for checkboxes with and without appearance: none;

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

::: accessible/tests/mochitest/role/test_general.html
@@ +56,5 @@
>        // After fix for bug 471356, it was temporarily exposed as a paragraph,
>        // breaking JAWS compatibility.
>        testRole("data", ROLE_TEXT_CONTAINER);
>  
> +      // Test that input type="checkbox" is exposed as such regardless of appearance style.

nit: 80 characters per line

@@ +58,5 @@
>        testRole("data", ROLE_TEXT_CONTAINER);
>  
> +      // Test that input type="checkbox" is exposed as such regardless of appearance style.
> +      testRole("checkbox_regular", ROLE_CHECKBUTTON);
> +      testRole("checkbox_appearance_none", ROLE_CHECKBUTTON);

please add tests for radio button too (to cover the patch changes)

Comment 33

[Marco Zehe (:MarcoZ)]

(In reply to Mats Palmgren (:mats) from comment #31)
> Marco, I think appearance:none in the test should use -moz-appearance
> instead.

What will happen if the support relands? Will the -moz-appearance just continue to work? Or will the test need to be adjusted then?

And does input @type="radio" support that style, too?

Comment 34

[Mats Palmgren (inactive)]

-moz-appearance will continue to work for the foreseeable future.  The plan is to add
support for -webkit-appearance as an alias with all the values supported by both, and
then probably drop -moz-appearance some time after that.  If we do that, then I'm sure
we'll make a script to change all uses of -moz-appearance to -webkit-appearance in
the whole tree, so all tests should continue to work as before.

I don't think we will support the unprefixed appearance unless Chrome adds support
for it because it would cause web-compat issues for us otherwise.

Comment 35

[Mats Palmgren (inactive)]

And yes, -moz-appearance:none works the same for both checkbox and radio.

Comment 36

[Marco Zehe (:MarcoZ)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4df99c3d89d7eccefd0f348ac5dd9db530c64ac8
Bug 1349835 Part 2 - Test that an input of type checkbox or radio with -moz-appearance:none; creates correct accessibles, r=surkov

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4df99c3d89d7

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #29)
> I'm not very familiar with the a11y code so I can't say what the risk is.
> I'll that decision to Alexander and Marco.

there's an interim fix (bug 605985) for this one, and then no uplift is required, right?

Comment 39

[Mats Palmgren (inactive)]

No, the opposite, this bug is a regression from bug 605985 which is in v54.
So the fix here needs to be uplifted, unless it's too risky.

Comment 40

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #39)
> No, the opposite, this bug is a regression from bug 605985 which is in v54.
> So the fix here needs to be uplifted, unless it's too risky.

sorry, I meant to say bug 1339775. I was curious if there are any regressions from bug 605985 other than bug 1339775.

Comment 41

[Mats Palmgren (inactive)]

All web pages that use -moz-appearance:none on a checkbox or radio control.

Comment 42

[Marco Zehe (:MarcoZ)]

I've run with this in for a few days, and the test has also been running on Central since Friday without problems. I think it is OK to uplift both to 54.

Comment 43

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #41)
> All web pages that use -moz-appearance:none on a checkbox or radio control.

sorry for being ignorant, but this style is not mozilla-internals specific as I was used to think and used on the web? then it totally makes sense to backport it, the patch is looks safe with me

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 8873022 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]:bug 605985
[User impact if declined]:inaccessible content on the web (input with -moz-apperance:none style)
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:n/a
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:small and transparent enough code tweaks
[String changes made/needed]:no

Comment 45

[Marco Zehe (:MarcoZ)]

Comment on attachment 8873413 [details] [diff] [review]
Test for checkboxes with and without appearance: none;

Approval Request Comment
This is the automated test for the Patch surkov asked approval for in comment #44 and should be uplifted along with it.

Comment 46

[Gerry Chang [:gchang]]

Hi :surkov,
Per your uplift request, can you share how noticeable this regression to be seen by users? We've already built 54 RC, I'd like to know how much impact for users if we don't take this one.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to Gerry Chang [:gchang] from comment #46)
> Hi :surkov,
> Per your uplift request, can you share how noticeable this regression to be
> seen by users? We've already built 54 RC, I'd like to know how much impact
> for users if we don't take this one.

I don't have the web sites names and numbers of affected of web sites, but as long as the web author puts appearance:none style on some inputs elements, then those elements won't be properly exposed to the assistive technologies.

I'm curious if there's any data how widespread -moz-appearance style on the web.

Comment 48

[Mats Palmgren (inactive)]

> I'm curious if there's any data how widespread -moz-appearance style on the web.

Fairly common I'm afraid.

Comment 49

[Gerry Chang [:gchang]]

Comment on attachment 8873022 [details] [diff] [review]
fix

Fix a new regression in 54. Let's take it in 54 RC2.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d55cc3fe796
https://hg.mozilla.org/releases/mozilla-beta/rev/26375e7c4d42

https://hg.mozilla.org/releases/mozilla-release/rev/7d55cc3fe796
https://hg.mozilla.org/releases/mozilla-release/rev/26375e7c4d42

Comment 51

[Andrei Vaida [:avaida]]

(In reply to alexander :surkov from comment #44)
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Alexander's assessment on manual testing needs and the fact that this fix has automated coverage.