1339775 - Checkboxes to submit a crashed tab or to include URL of page no longer accessible.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

When a single tab crashes, the UI that comes up used to have two checkbox, similar to the full crash reporter, which tell whether Firefox should submit the report, and which page URL was loaded when the crash occurred.

A recent change broke this. The text is still there, an element there even has a tab stop, but it is no longer a checkbox. It is not exposing the role as a checkbox, and it is not emitting the checked or not checked states.

It looks like the actual native checkbox got replaced with something that is supposed to look like one, but is not truly an html:input type="checkbox" element any more. First question is why, and second question is how this got a proper tab stop, but no role and state associated with it.

Gijs, where does this go? Which component is responsible?

Comment 1

[:Gijs (he/him)]

Tabbed browser, I think, if it's really a markup problem caused by that page itself. But the markup looks fine to me when looking at the source - I see 3 <input type="checkbox"> elements, all with corresponding labels that have a for= attribute pointing to the checkbox. There's also other input types on the page, like a text field for email and some buttons...

What exact element are you having trouble with? When did this regress?

(CC'ing mconley because all the recent changes to that page are his.)

Comment 2

[Marco Zehe (:MarcoZ)]

I don't know when this regressed, I don't get single tab crashes every day, just noticed it when I actually got on in the 2017-02-14 nightly build. The items have the word "tag" in their IDs, and are exposed like text frames, not real inputs. To me they look like a <span> element with an aria-label which correspond to the text in the next accessible object, but no role or states.

Comment 3

[:Gijs (he/him)]

(In reply to Marco Zehe (:MarcoZ) from comment #2)
> I don't know when this regressed, I don't get single tab crashes every day,
> just noticed it when I actually got on in the 2017-02-14 nightly build. The
> items have the word "tag" in their IDs, and are exposed like text frames,
> not real inputs. To me they look like a <span> element with an aria-label
> which correspond to the text in the next accessible object, but no role or
> states.

I get no hits searching for "tag" in aboutTabCrashed.xhtml. Mike, any ideas what's going on here?

Marco, I don't suppose there's a way for you to use NVDA or the devtools inspector to copy-paste the full markup of the element in question?

Comment 4

[Marco Zehe (:MarcoZ)]

When I hit this next time, I will try to get as much info about this as I can. Without a crash, about:tabcrashed doesn't give me those elements at all, just some explanatory text.

Comment 5

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

Hm. The only change that occurred with these checkboxes is that we stopped doing a hacky pseudoelement style to make them look like the spec (since until bug 418833 landed, checkboxes and radios couldn't be styled).

We did similar restyling of checkboxes in other pages. Perhaps we can see if the problem is there as well - these are typically easier to get to than trying to simulate or provoke a crash (although such add-ons exist - example: https://addons.mozilla.org/en-US/firefox/addon/crash-me-now-simple/).

Hey MarcoZ: if you visit about:networking, there should be a warning the first time it's visited, talking about the experimental nature of the page. After that warning should be a checkbox for whether or not to display the warning again next time. Is the checkbox there and usable for you?

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to Mike Conley (:mconley) from comment #5)
> Hey MarcoZ: if you visit about:networking, there should be a warning the
> first time it's visited, talking about the experimental nature of the page.
> After that warning should be a checkbox for whether or not to display the
> warning again next time. Is the checkbox there and usable for you?

No, this one also shows the problem! It looks like it is an Accessibility APIs problem after all. The developer info for this element looks like this, gotten from NVDA's protocol viewer:

Developer info for navigator object:
name: u'Diese Warnung beim n\xe4chsten Mal anzeigen'
role: ROLE_TEXTFRAME
states: STATE_FOCUSABLE, STATE_FOCUSED
isFocusable: True
hasFocus: True
Python object: <NVDAObjects.IAccessible.mozilla.Mozilla object at 0x05946E30>
Python class mro: (<class 'NVDAObjects.IAccessible.mozilla.Mozilla'>, <class 'NVDAObjects.IAccessible.ia2Web.Ia2Web'>, <class 'NVDAObjects.IAccessible.IAccessible'>, <class 'NVDAObjects.window.Window'>, <class 'NVDAObjects.NVDAObject'>, <class 'baseObject.ScriptableObject'>, <class 'baseObject.AutoPropertyObject'>, <type 'object'>)
description: u''
location: (120, 1072, 46, 46)
value: None
appModule: <'firefox' (appName u'firefox', process ID 5816) at address 501ff10>
appModule.productName: u'Nightly'
appModule.productVersion: u'54.0a1'
TextInfo: <class 'NVDAObjects.IAccessible.IA2TextTextInfo'>
windowHandle: 3343738L
windowClassName: u'MozillaWindowClass'
windowControlID: 0
windowStyle: 382664704
windowThreadID: 6972
windowText: u'Netzwerkverbindungen - Nightly'
displayText: u''
IAccessibleObject: <POINTER(IAccessible2) ptr=0xc12e4a4 at 5393cb0>
IAccessibleChildID: 0
IAccessible event parameters: windowHandle=3343738L, objectID=-4, childID=-1040
IAccessible accName: u'Diese Warnung beim n\xe4chsten Mal anzeigen'
IAccessible accRole: u'input'
IAccessible accState: STATE_SYSTEM_FOCUSED, STATE_SYSTEM_FOCUSABLE, STATE_SYSTEM_VALID (1048580)
IAccessible accDescription: u''
IAccessible accValue: None
IAccessible2 windowHandle: 3343738
IAccessible2 uniqueID: -1040
IAccessible2 role: IA2_ROLE_TEXT_FRAME
IAccessible2 states: IA2_STATE_OPAQUE (1024)
IAccessible2 attributes: u'margin-left:4px;text-align:start;text-indent:0px;formatting:block;id:warncheck;tag:input;margin-right:10px;margin-top:3px;margin-bottom:3px;display:block;explicit-name:true;'

Alex, as you can see from above, an input is incorrectly mapped to a textframe accessible instead of one with role checkbox.
You can get to this by simply entering about:networking in the address bar of a new tab and hitting ENTER.

Comment 7

[Marco Zehe (:MarcoZ)]

Also happens with E10S off.

Comment 8

[Marco Zehe (:MarcoZ)]

Other checkboxes, however, are not affected, such as the ones on Bugzilla.

Comment 9

[alexander :surkov (:asurkov)]

Having no DOMi at hands to check a11y part, here's some observations on DOM level:

input@type="checkbox" have styles:
opacity: 0;
width: 0;
pointer-events: none;
position: absolute;

which I assume makes it inaccessilbe. The visual checkbox itself is implemented by ::before style inside the label, so it appears there's no checkbox semantics on this page at all. I'm curious if that HTML:input can be replaced on role="checkbox".

Comment 10

[Marco Zehe (:MarcoZ)]

Yes, the zero width is certainly suspicious. Note, however, that keyboard tabbing lands on this thing exactly. It gets DOM focus when tabbing to it, not the label. Mike, what's up with these changes? Why is the checkbox styled the way it is?

Comment 11

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

(In reply to Marco Zehe (:MarcoZ) from comment #10)
> Why is the checkbox styled the way it is?

I believe it was the only way to achieve the visual spec that UX provided.

The good news is that with bug 418833 and bug 1309316 landed, we are able to get the effect without pseudoelements and hacky workarounds like this. Those changes landed in Firefox 53, and should be on both Developer Edition and Nightly.

So I suspect what :surkov was examining was either Beta or Release, which is what we've been shipping for a while. I don't think it explains why MarcoZ wasn't able to get to the checkbox on Nightly, which shouldn't have the same hacky styling rules in comment 9.

If you look at the checkbox in Nightly (even the one in about:networking is sufficient, as they act the same way), does it help illuminate what's going wrong here?

Comment 12

[alexander :surkov (:asurkov)]

Unfortunately DOMi stopped working on Nightly (even with e10s disabled), so I don't have a reliable tool to check the accessible tree anymore. I do see however -moz-appearance:none style applied on the element. I'd be curious to know if turning off this style solves the problem. If it is, then nsGfxCheckboxControlFrame is likely not created for the element, and we fail to create a proper accessible.

We can try these approaches if my assumptions are correct:
1) put role='checkbox' on the element, probably the easiest but ugliest fix
2) extend MARKUP macros to allow attribute checks, and add a rule for input@type='checkbox'

Comment 13

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

Who should own this? (Is it in the right component?)

Comment 14

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #13)
> Who should own this? (Is it in the right component?)

It's not yet clear, where the problem is, see comment #12. As long as it's done the component and ownership will be detected, before that it can stay in a11y component for course.

Comment 15

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

I'm reasonably certain that nsGfxCheckboxControlFrame is not being created when -moz-appearance is none here.

I'm happy to put role="checkbox" in those checkboxes, but if the better solution is to extend the MARKUP macros, we should just do that.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #15)
> I'm happy to put role="checkbox" in those checkboxes, but if the better
> solution is to extend the MARKUP macros, we should just do that.

Can you extend those MARKUP macros? =D

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Nathan Froyd [:froydnj] from comment #16)
> (In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos)
> from comment #15)
> > I'm happy to put role="checkbox" in those checkboxes, but if the better
> > solution is to extend the MARKUP macros, we should just do that.
> 
> Can you extend those MARKUP macros? =D

I'm not sure if I can jump into implementation myself anytime soon, but if Mike or anyone else is keen to try it out, then here's more details:

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

If no one is around to take this approach, then probably file a bug for markupmap macros thing and do role='checkbox' is the way to go

Comment 18

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

Attachment: Bug 1339775 - Add role="checkbox" for specially styled in-content checkboxes.

Review commit: https://reviewboard.mozilla.org/r/122918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122918/

Comment 19

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

Thanks surkov - I'm going to opt for deferral, in the interests of getting something landed that we might even be able to uplift (since I believe this affects 54 and up).

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8850293 [details]
Bug 1339775 - Add role="checkbox" for specially styled in-content checkboxes.

https://reviewboard.mozilla.org/r/122918/#review125510

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3be6c2cbcf85
Add role="checkbox" for specially styled in-content checkboxes. r=jaws

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3be6c2cbcf85

Comment 23

[Nathan Froyd [:froydnj]]

Can you request uplift on this, please?

Comment 24

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

Hey MarcoZ, are you able to confirm in a recent Nightly that this has been addressed for the in-content checkboxes?

Comment 25

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

I hope to verify this tomorrow.

Comment 26

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

I checked about:networking (per comment 5), and the checkbox has role checkbox. I logged the speech output via NVDA speech viewer, the relevant part (my comments in square braces):
[I focused FF]
About Networking - Nightly  busy
About Networking  document
Show this warning next time  check box  not checked
[I don't know why it keeps talking below:]
About Networking
check box  not checked
clickable
Show this warning next time
button
OK
clickable
HTTP
Sockets
...
[it eventually stops talking after about 10 more lines of stuff I can't see]

When I toggle the checkbox I don't get the expected feedback (only visual feedback).

Comment 27

[Gerry Chang [:gchang]]

Hi :davidb,
Does that mean this fix is not what we expected?

Comment 28

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

Right it doesn't seem fixed in my manual testing. Marco can you take over here?

Comment 29

[Marco Zehe (:MarcoZ)]

The principal announcement is correct, the problem that the state doesn't get spoken when toggled is probably due to aria-checked not being set to true or false the moment it gets toggled. Remember this is a JS thing to do once we use ARIA, the whole thing no longer announces itself to screen readers otherwise. So the bug is partially fixed, but not fully.

Comment 30

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

Okay, sounds like there's more to do here. Is there anything else I can do in the front-end, or do we need to get bug 1349835 fixed? Or something else entirely?

Comment 31

[Marco Zehe (:MarcoZ)]

You can add a keypress event listener that toggles the aria-checked state when space is pressed on any of the checkboxes (the same ones you pur role="checkbox" on. The toggling happens in an inaccessible element and thus doesn't fire an event that screen readers can pick up on. So, we have to fake it by toggling aria-checked from true to false and vice versa.

Comment 32

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

(In reply to Marco Zehe (:MarcoZ) from comment #31)

Thanks MarcoZ.

It sounds to me like we're somehow broken a11y events when we hide the native checkbox with -moz-appearance: none ... :/

We'll likely want to fix this in the platform, rather than just fixing the in-content -moz-appearance: none checkboxes that we ship with the event handler aria-checked hack.

mats, do you know how we broke a11y here with the -moz-appearance: none work on these form fields?

Comment 33

[Mats Palmgren (inactive)]

I'd guess bug 605985 - now we create an inline box instead of nsGfxCheckbox/RadioControlFrame.
This is for web compat, so it's not something we can change.

In general, a11y should use the DOM and not use layout stuff.  In this case,
<input type="checkbox"> makes it a checkbox *regardless* of what styles it has.
So the suggested fix in bug 1349835 looks like the right way to fix this.

Comment 34

[Gerry Chang [:gchang]]

Mark 54 won't fix as this is late for Beta54.