[menu a11y] Expose checkable <menuitem>s as checkbox/radio roles [chromium/src : main]
David Grogan (Gerrit)
David Grogan added 3 comments![]( "Open in Gerrit")
data/accessibility/html/meta-expected-mac.txtIs it ok that our menu* stuff doesn't run on other platforms?
if (auto* menu_item = DynamicTo<HTMLMenuItemElement>(GetNode())) {This had to go before the `if (!GetLayoutObject())` check because the menuitems in third_party/blink/web_tests/accessibility/computed-role.html don't have a LayoutObject.
DCHECK(EqualIgnoringASCIICase(checkable_type, keywords::kMultiple));Is this right? There are two mutually exclusive and exhaustive options?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 7 comments![]( "Open in Gerrit")
This mostly looks good, a few comments follow (half of which are responses to yours).
data/accessibility/html/meta-expected-mac.txtIs it ok that our menu* stuff doesn't run on other platforms?
I think it probably is? I think in some cases it's probably OK to have *only* a `-blink.txt` and none of the platform-specific trees. But it might also be useful to have more coverage... it just requires generating all the baselines!
</fieldset>Maybe also throw in:
- a fieldset without `checkable`, with the same menuitems (including `defaultchecked`)
- the same pair of menuitems (one with `defaultchecked`) not in a `fieldset`?
if (auto* menu_item = DynamicTo<HTMLMenuItemElement>(GetNode())) {This had to go before the `if (!GetLayoutObject())` check because the menuitems in third_party/blink/web_tests/accessibility/computed-role.html don't have a LayoutObject.
Maybe it makes more sense to put this in `AXNodeObject::NativeRoleIgnoringAria()`? That has a decent number of element-type specific cases?
(I suspect it may make sense do the same for permission/menubar/menulist?)
DCHECK(EqualIgnoringASCIICase(checkable_type, keywords::kMultiple));Is this right? There are two mutually exclusive and exhaustive options?
Given that `IsCheckable()` is true, yes, there are two mutually exclusive options.
However, you shouldn't use `DCHECK()` to check that the markup is correct, which I think is what this `DCHECK()` is doing. I think it looks like most (but not all!) of the existing code treats any value other than "single" as multiple, which I think is fine.
// First test for native checked state
if (IsA<HTMLInputElement>(*node)) {
const auto* input = DynamicTo<HTMLInputElement>(node);While you're here, better to do:
```suggestion
// First test for native checked state
if (const auto* input = DynamicTo<HTMLInputElement>(node)) {
```
(It does fewer `IsA` operations underneath, and it's shorter!)
I suspect this should be in a separate computed-role-menus.tentative.html test because it needs to be in a test with tentative in the name.
Related details
- David Grogan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Grogan (Gerrit)
David Grogan added 3 comments![]( "Open in Gerrit")
data/accessibility/html/meta-expected-mac.txtDavid BaronIs it ok that our menu* stuff doesn't run on other platforms?
I think it probably is? I think in some cases it's probably OK to have *only* a `-blink.txt` and none of the platform-specific trees. But it might also be useful to have more coverage... it just requires generating all the baselines!
Acknowledged
// First test for native checked state
if (IsA<HTMLInputElement>(*node)) {
const auto* input = DynamicTo<HTMLInputElement>(node);While you're here, better to do:
```suggestion
// First test for native checked state
if (const auto* input = DynamicTo<HTMLInputElement>(node)) {
```(It does fewer `IsA` operations underneath, and it's shorter!)
Fix applied.
I suspect this should be in a separate computed-role-menus.tentative.html test because it needs to be in a test with tentative in the name.
This file isn't in WPT, so 'tentative' doesn't really matter? But do you think I should put these tests in a tenative wpt instead of here?
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 1 comment![]( "Open in Gerrit")
David GroganI suspect this should be in a separate computed-role-menus.tentative.html test because it needs to be in a test with tentative in the name.
This file isn't in WPT, so 'tentative' doesn't really matter? But do you think I should put these tests in a tenative wpt instead of here?
- David Grogan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Grogan (Gerrit)
David Grogan added 4 comments![]( "Open in Gerrit")
Maybe also throw in:
- a fieldset without `checkable`, with the same menuitems (including `defaultchecked`)
- the same pair of menuitems (one with `defaultchecked`) not in a `fieldset`?
Good idea, added. They even seem to work.
if (auto* menu_item = DynamicTo<HTMLMenuItemElement>(GetNode())) {David BaronThis had to go before the `if (!GetLayoutObject())` check because the menuitems in third_party/blink/web_tests/accessibility/computed-role.html don't have a LayoutObject.
Maybe it makes more sense to put this in `AXNodeObject::NativeRoleIgnoringAria()`? That has a decent number of element-type specific cases?
(I suspect it may make sense do the same for permission/menubar/menulist?)
Done
DCHECK(EqualIgnoringASCIICase(checkable_type, keywords::kMultiple));Is this right? There are two mutually exclusive and exhaustive options?
Given that `IsCheckable()` is true, yes, there are two mutually exclusive options.
However, you shouldn't use `DCHECK()` to check that the markup is correct, which I think is what this `DCHECK()` is doing. I think it looks like most (but not all!) of the existing code treats any value other than "single" as multiple, which I think is fine.
However, you shouldn't use DCHECK() to check that the markup is correct, which I think is what this DCHECK() is doing
🤦
Done
David GroganI suspect this should be in a separate computed-role-menus.tentative.html test because it needs to be in a test with tentative in the name.
David BaronThis file isn't in WPT, so 'tentative' doesn't really matter? But do you think I should put these tests in a tenative wpt instead of here?
Oops, never mind, I was thinking it was in WPT.
- David Baron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |