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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mconley, Assigned: mats)

Tracking

(Blocks 1 bug, {regression})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 fixed, firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
What do other browsers do in the appearance:none case?
(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?
Marco can you find out what other browsers do here?
Flags: needinfo?(mzehe)
(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. :-(
Flags: needinfo?(mzehe)
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.
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.
Any chance anyone on your team has cycles for this, davidb, before this regression goes out in 54?
Flags: needinfo?(dbolter)
(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?
Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)
(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
Flags: needinfo?(surkov.alexander)
I can try a stab at this next week.
Assignee: nobody → mzehe
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.
Assignee: mzehe → nobody
(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.
Nice-to-have for 54, but it's unlikely it'll make it in.
Duplicate of this bug: 1366928
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.
FYI, bug 1333482 was backed out in bug 1365614 (it will also be backed out in v54).
(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)
Posted patch WIP (obsolete) — Splinter Review
Something like this might work for now. (it compiles, but I haven't tested it)

Marco, can you take it from here?
Flags: needinfo?(mzehe)
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).
Flags: needinfo?(mzehe) → needinfo?(mats)
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!
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.
Depends on: 1368555
Posted patch fixSplinter Review
I think this is good enough as a temporary solution at least.
Attachment #8870723 - Attachment is obsolete: true
Attachment #8873022 - Flags: review?(surkov.alexander)
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
Attachment #8873022 - Flags: review?(surkov.alexander) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc6012cad03e
Create accessibles for <input type=checkbox/radio> with -moz-appearance:none.  r=surkov
> 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 :-)
Flags: needinfo?(mzehe)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/fc6012cad03e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Can this ride the 55 train or should we try to uplift to 54 still? The RC build is happening on Monday, FWIW.
Assignee: nobody → mats
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats) → needinfo?(surkov.alexander)
Flags: needinfo?(mzehe)
Attachment #8873413 - Flags: review?(surkov.alexander)
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 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)
Attachment #8873413 - Flags: review?(surkov.alexander) → review+
(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?
Flags: needinfo?(mats)
-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.
Flags: needinfo?(mats)
And yes, -moz-appearance:none works the same for both checkbox and radio.
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
https://hg.mozilla.org/mozilla-central/rev/4df99c3d89d7
Flags: in-testsuite? → in-testsuite+
(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?
Flags: needinfo?(surkov.alexander)
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.
(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.
All web pages that use -moz-appearance:none on a checkbox or radio control.
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.
(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 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
Attachment #8873022 - Flags: approval-mozilla-beta?
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.
Attachment #8873413 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
> I'm curious if there's any data how widespread -moz-appearance style on the web.

Fairly common I'm afraid.
Summary: Add macros to ensure that accessibles are created for checkboxes with appearance: none, → Add macros to ensure that accessibles are created for checkboxes with -moz-appearance: none,
Comment on attachment 8873022 [details] [diff] [review]
fix

Fix a new regression in 54. Let's take it in 54 RC2.
Attachment #8873022 - Flags: approval-mozilla-release+
Attachment #8873022 - Flags: approval-mozilla-beta?
Attachment #8873022 - Flags: approval-mozilla-beta+
Attachment #8873413 - Flags: approval-mozilla-release+
Attachment #8873413 - Flags: approval-mozilla-beta?
Attachment #8873413 - Flags: approval-mozilla-beta+
(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.
Flags: qe-verify-
Blocks: 1398520
You need to log in before you can comment on or make changes to this bug.