Add testdriver-vendor.js support for get_accessibility_properties_* [chromium/src : main]
Valerie Young (Gerrit)
Valerie Young added 1 comment![]( "Open in Gerrit")
Hi David and Lucas, could you take a look at this CL?
Related details
- David Tseng
- Lucas Radaelli
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucas Radaelli (Gerrit)
Lucas Radaelli added 4 comments![]( "Open in Gerrit")
Add testdriver-vendor.js support for get_accessibility_properties_*nit: can you please add [a11y] tag?
Can we please document this, or at least post a comment where this is coming from? (the explaner would be fine), or, if t is being finalized, just a TODO(xxxx) would be fine, just so that we can point out that this is coming from the spec.
if (AXObject* parent = ax_object->ParentObject()) {nit: perhaps ParentObjectIncludedInTree?
children_ids.push_back(child->AXObjectID());nit: this for loop might add children that are marked as ignored. Above we are skipping objects that are marked as ignored. Should we do the same here?
Related details
- David Tseng
- Valerie Young
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. |
Valerie Young (Gerrit)
Valerie Young added 5 comments![]( "Open in Gerrit")
Fixed some things and left comments on your comments, thanks Lucas!
Add testdriver-vendor.js support for get_accessibility_properties_*nit: can you please add [a11y] tag?
Done
Can we please document this, or at least post a comment where this is coming from? (the explaner would be fine), or, if t is being finalized, just a TODO(xxxx) would be fine, just so that we can point out that this is coming from the spec.
Done
if (AXObject* parent = ax_object->ParentObject()) {nit: perhaps ParentObjectIncludedInTree?
Do you mean ParentObjectIfPresent? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.h;l=1249
This also returns ignored parents. If I wanted to get unignored parents, it would be ParentObjectUnignored().
children_ids.push_back(child->AXObjectID());nit: this for loop might add children that are marked as ignored. Above we are skipping objects that are marked as ignored. Should we do the same here?
Hmm I didn't mean to skip over objects that are ignored, in the current implementation, I decided to include ignored nodes -- although just now I just realize this returns an AccessibilityProperties with role set to "none" for two scenarios: one where the request object doesn't exist, and the other when it's ignored. This was the logic of the implementation of the webdriver GetComputedRole before I got here though.
I don't actually know if I should return only unignored parents and children for this API. In the devtools, you can see ignored some ignored nodes, which inspired me to choose to return the tree with ignored nodes in it.
But my mind is not made up on this, and I really love to hear your opinion -- whether to return the tree with or without ignored nodes. The ARIA specification has a concept of an accessibility tree that specifically does not include ignored nodes, so maybe that is the way we should go -- matching this ARIA concept: https://w3c.github.io/aria/#accessibility_tree
Related details
- David Tseng
- Lucas Radaelli
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 Tseng (Gerrit)
David Tseng added 5 comments![]( "Open in Gerrit")
AXObject* GetAXObject(const Element* element, int32_t accessibility_id) {Consider having the caller supply the Document here (otherwise, this call signature may be somewhat difficult to parse without peeking at the implementation).
ax_object_cache->UpdateAXForAllDocuments();nit: CHECK(ax_object_cache);
(I haven't read the context around this file but assume we're expecting accessibility to already be enabled on the document).
return AXObject::AriaRoleName(ax::mojom::Role::kNone);Is there a need to distinguish between no object vs an ignored object?
props->setChildren(children_ids);SHould we set this if there are no children?
if (checked_state != ax::mojom::CheckedState::kNone) {I worry a bit about encoding attribute logic like this. Could we attempt to re-use what's already within AXObject for this purpose e.g. some of the tree serialization logic? Ultimately, this goes to using a full unserialized AXTree which encodes many of these rules, depending on exactly which attributes we're interested in.
InspectorAccessibilityAgent (which I erroneously pointed you to for WPT) does something like this in order to surface data to dev tools.
Related details
- Lucas Radaelli
- Valerie Young
Lucas Radaelli (Gerrit)
Lucas Radaelli added 2 comments![]( "Open in Gerrit")
if (AXObject* parent = ax_object->ParentObject()) {Valerie Youngnit: perhaps ParentObjectIncludedInTree?
Do you mean ParentObjectIfPresent? https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.h;l=1249
This also returns ignored parents. If I wanted to get unignored parents, it would be ParentObjectUnignored().
will close this thread and we discuss ignored objects in the other thread, we can keep this one as it is.
children_ids.push_back(child->AXObjectID());Valerie Youngnit: this for loop might add children that are marked as ignored. Above we are skipping objects that are marked as ignored. Should we do the same here?
Hmm I didn't mean to skip over objects that are ignored, in the current implementation, I decided to include ignored nodes -- although just now I just realize this returns an AccessibilityProperties with role set to "none" for two scenarios: one where the request object doesn't exist, and the other when it's ignored. This was the logic of the implementation of the webdriver GetComputedRole before I got here though.
I don't actually know if I should return only unignored parents and children for this API. In the devtools, you can see ignored some ignored nodes, which inspired me to choose to return the tree with ignored nodes in it.
But my mind is not made up on this, and I really love to hear your opinion -- whether to return the tree with or without ignored nodes. The ARIA specification has a concept of an accessibility tree that specifically does not include ignored nodes, so maybe that is the way we should go -- matching this ARIA concept: https://w3c.github.io/aria/#accessibility_tree
All right, so I will describe my mental model and we try to come up with something that make sense. I also don't know the right thing to do here, but we can start with what seems to be reasonable, and we come up with tests that stretch our understanding and we look at the results later.
I will define ignored nodes. Ignored nodes are nodes that should not be included in the a11y tree (using blink terminology here). What makes things a bit harder is that there was a concept introduced in blink called Ignored but included in tree. There is even a helper method to do that.
As the need suggests, ignored but included in tree are nodes that for some reason were ignored, but they need to be present in order to make the tree structure valid. This is the best explanation I can give at the moment.
Without the test cases, it will be a bit hard to know at this point what makes sense or not. What about we continue as it is, but file a bug to add tests that **trigger ignored subtrees**, so we can check their expectations and see if they make sense?
Without test examples I found hard to judge what we should do. Should we include d ignored but included in tree? Should we skip all? I don't know.
Related details
- Valerie Young
Valerie Young (Gerrit)
Valerie Young added 6 comments![]( "Open in Gerrit")
hi David! Fixed some things and replied to others, thanks!
AXObject* GetAXObject(const Element* element, int32_t accessibility_id) {Consider having the caller supply the Document here (otherwise, this call signature may be somewhat difficult to parse without peeking at the implementation).
good idea, thanks!
nit: CHECK(ax_object_cache);
(I haven't read the context around this file but assume we're expecting accessibility to already be enabled on the document).
Done
return AXObject::AriaRoleName(ax::mojom::Role::kNone);Is there a need to distinguish between no object vs an ignored object?
I would guess, eventually, yes, we need to distinguish. Right now I was just copying what was returned by getComputedRole, which is none in both cases.
I think we might want to return "ignored: true" on ignored nodes. What exactly we return in these property bags it's not defined yet - it's currently being discussed in the comments of https://github.com/WICG/aom/issues/203#issuecomment-3963870273 and in the accessibility interop meetings. Any and all feedback welcome!
SHould we set this if there are no children?
good catch.
if (checked_state != ax::mojom::CheckedState::kNone) {I worry a bit about encoding attribute logic like this. Could we attempt to re-use what's already within AXObject for this purpose e.g. some of the tree serialization logic? Ultimately, this goes to using a full unserialized AXTree which encodes many of these rules, depending on exactly which attributes we're interested in.
InspectorAccessibilityAgent (which I erroneously pointed you to for WPT) does something like this in order to surface data to dev tools.
Unfortunately... we can't get around this specific check, whether checked means checked or pressed is calculated at the platform node level, for example: https://source.chromium.org/chromium/chromium/src/+/main:ui/accessibility/platform/ax_platform_node_auralinux.cc;l=4571?q=auralinux%20f:platform%20f:accessi&ss=chromium%2Fchromium%2Fsrc
Btw I can't remember if we talked about it -- but the eventual webdriver endpoint will use the devtool protocol to get this data (like get computed role and get computed label already do) and the devtool code has this special logic in it, in inspector_type_builder_helper.cc, see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/inspector_type_builder_helper.cc;l=511;drc=44eee492c8b0a8ca17bb286a3826308a10ef0c3f
But I think you are a right for other kinds of calculations that happen during serialization -- and the devtools code does start with the AXNodeData calculated from AXObject::Serialize. So that would probably be a better starting point for getting the rest of the properties we end up supporting here.
Related details
- David Tseng
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 Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
if (checked_state != ax::mojom::CheckedState::kNone) {Valerie YoungI worry a bit about encoding attribute logic like this. Could we attempt to re-use what's already within AXObject for this purpose e.g. some of the tree serialization logic? Ultimately, this goes to using a full unserialized AXTree which encodes many of these rules, depending on exactly which attributes we're interested in.
InspectorAccessibilityAgent (which I erroneously pointed you to for WPT) does something like this in order to surface data to dev tools.
Unfortunately... we can't get around this specific check, whether checked means checked or pressed is calculated at the platform node level, for example: https://source.chromium.org/chromium/chromium/src/+/main:ui/accessibility/platform/ax_platform_node_auralinux.cc;l=4571?q=auralinux%20f:platform%20f:accessi&ss=chromium%2Fchromium%2Fsrc
Btw I can't remember if we talked about it -- but the eventual webdriver endpoint will use the devtool protocol to get this data (like get computed role and get computed label already do) and the devtool code has this special logic in it, in inspector_type_builder_helper.cc, see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/inspector_type_builder_helper.cc;l=511;drc=44eee492c8b0a8ca17bb286a3826308a10ef0c3f
But I think you are a right for other kinds of calculations that happen during serialization -- and the devtools code does start with the AXNodeData calculated from AXObject::Serialize. So that would probably be a better starting point for getting the rest of the properties we end up supporting here.
Unfortunately... we can't get around this specific check, whether checked means checked or pressed is calculated at the platform node level, for example: https://source.chromium.org/chromium/chromium/src/+/main:ui/accessibility/platform/ax_platform_node_auralinux.cc;l=4571?q=auralinux%20f:platform%20f:accessi&ss=chromium%2Fchromium%2Fsrc
Chrome uses aria-pressed in its role computation[1] and uses it in surfacing a generic checked state[2].
If we wanted to more accurately encode this relationship, CHrome could separate the ideas of pressed and checked on serialization (pressed becomes a boolean, checked is still a enum true, false, mixed).
Then, in this test layer, we would rely upon the pressed boolean directly.
>
> Btw I can't remember if we talked about it -- but the eventual webdriver endpoint will use the devtool protocol to get this data (like get computed role and get computed label already do) and the devtool code has this special logic in it, in inspector_type_builder_helper.cc, see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/inspector_type_builder_helper.cc;l=511;drc=44eee492c8b0a8ca17bb286a3826308a10ef0c3f
>
Ack!
But I think you are a right for other kinds of calculations that happen during serialization -- and the devtools code does start with the AXNodeData calculated from AXObject::Serialize. So that would probably be a better starting point for getting the rest of the properties we end up supporting here.
Yeah, I guess I'm generally not a fan of writing test only logic since it doesn't represent actual production behaviors (in this case it maybe does, but it's hard to tell). If we need to, maybe we could just flag it in some way via a comment.
1. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=8498?q=ariapressed%20togglebutton%20ax_object.cc&ss=chromium
2. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object.cc;l=3232?q=ariapressed%20togglebutton%20ax_object.cc&ss=chromium
Related details
- Valerie Young