[iOS][apcv2] Add ARIA role extraction the iOS APC TreeWalker [chromium/src : main]
Fikre Mengistu (Gerrit)
Fikre Mengistu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 6 comments![]( "Open in Gerrit")
thanks! your help is greatly appreciated!
We also do adjustments to annotated roles: more than 1 and handle the hidden category
function getAnnotatedRoleForAriaRole(ariaRoleAttr: string):I think they fill multiple annotated roles based on that aria role in blink: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=2297;drc=963dd5d56d0e95e1df13f86fa8a6f1eb62e47fa4
const roles = ariaRoleAttr.trim().split(/\s+/);Make this a constant, e.g. ARIAL_ROLE_DELIMITER
const style = windowObj.getComputedStyle(domNode);note: I think we could limit the calls to getComputedStyle by sharing the style across calls but I am not sure if we'd expect any performance gains from doing that since (1) that there is caching done internally for the computed style and (2) all the extraction is done in one single synced call where the layout won't be refreshed.
filled http://crbug.com/494180380 for that
annotatedRoles.push(roleFromAria);we could consider making this a set then make it an array before returning the results
Related details
- Fikre Mengistu
- Nicolas MacBeth
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. |
Vincent Boisselle (Gerrit)
Vincent Boisselle added 1 comment![]( "Open in Gerrit")
Do we want to set the aria role in the proto as well ?
Nicolas MacBeth (Gerrit)
Nicolas MacBeth added 3 comments![]( "Open in Gerrit")
Thanks Fikre!!
const roles = ariaRoleAttr.trim().split(/\s+/);Make this a constant, e.g. ARIAL_ROLE_DELIMITER
++ in the comment can we add what this regex looks for
annotatedRoles: [],do we want to throwaway the annotated roles in this case?
can we make sure that with the annotated roles modifications we don't throw them away in other spots as well? thanks!
Related details
- Fikre Mengistu
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. |
Fikre Mengistu (Gerrit)
Fikre Mengistu added 6 comments![]( "Open in Gerrit")
Do we want to set the aria role in the proto as well ?
We also do adjustments to annotated roles: more than 1 and handle the hidden category
Done
function getAnnotatedRoleForAriaRole(ariaRoleAttr: string):I think they fill multiple annotated roles based on that aria role in blink: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;l=2297;drc=963dd5d56d0e95e1df13f86fa8a6f1eb62e47fa4
Although it looks like that, I don't believe they do. Their [AXObject::DetermineRawAriaRole](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/content_extraction/ai_page_content_agent.cc;drc=963dd5d56d0e95e1df13f86fa8a6f1eb62e47fa4;l=2313) function parses the role string and resolves it to a single ARIA role enum (it just picks the first valid one and ignores the rest).
It runs through a series of independent if statements checking both the HTML tag and that single resolved ARIA role.
Because every if block is evaluated, an element can accumulate multiple annotated roles, one from the aria role and others from the html tag, visibility, and paid content status. It does not pull multiple roles from the role attribute string itself.
The current logic here achieves this exact same output for feature parity.
Nicolas MacBethMake this a constant, e.g. ARIAL_ROLE_DELIMITER
++ in the comment can we add what this regex looks for
Done
do we want to throwaway the annotated roles in this case?
can we make sure that with the annotated roles modifications we don't throw them away in other spots as well? thanks!
They actually aren't thrown away. Right below this block (around line 1750), there's a check that re-assigns them: `contentNode.contentAttributes.annotatedRoles = annotatedRoles;`.
However, I'll remove the empty `annotatedRoles: []` from the initialization block here so it's clearer.
we could consider making this a set then make it an array before returning the results
we use an `if (!annotatedRoles.includes(roleFromAria))` check. This mimics Blink's duplicate-prevention behavior without adding the overhead of converting `Sets` back and forth to `Arrays` for a list that will only ever hold up to 4 items.
Related details
- Nicolas MacBeth
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas MacBeth (Gerrit)
Nicolas MacBeth voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nicolas MacBeth
| Code-Review | +1 |
3 comments
const ATTR_VALUE_ROLE_BANNER = 'banner';
const ATTR_VALUE_ROLE_NAVIGATION = 'navigation';
const ATTR_VALUE_ROLE_SEARCH = 'search';
const ATTR_VALUE_ROLE_MAIN = 'main';nit: I think these are already defined in other constants, should we consolidate?
Related details
- Fikre Mengistu
- Vincent Boisselle
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. |
Fikre Mengistu (Gerrit)
Fikre Mengistu added 2 comments![]( "Open in Gerrit")
const ATTR_VALUE_ROLE_BANNER = 'banner';
const ATTR_VALUE_ROLE_NAVIGATION = 'navigation';
const ATTR_VALUE_ROLE_SEARCH = 'search';
const ATTR_VALUE_ROLE_MAIN = 'main';nit: I think these are already defined in other constants, should we consolidate?
Keeping the constants separate helps readability, so I think I'll keep it as is.
Related details
- Vincent Boisselle
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vincent Boisselle (Gerrit)
Vincent Boisselle voted Code-Review+1![]( "Open in Gerrit")
- Fikre Mengistu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fikre Mengistu (Gerrit)
Fikre Mengistu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fikre Mengistu (Gerrit)
Fikre Mengistu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fikre Mengistu (Gerrit)
Fikre Mengistu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
14 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/intelligence/proto_wrappers/resources/annotated_page_content_extraction.ts
Insertions: 9, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/intelligence/proto_wrappers/page_context_wrapper_unittest.mm
Insertions: 58, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/intelligence/proto_wrappers/page_context_extractor_java_script_feature_unittest.mm
Insertions: 116, Deletions: 32.
The diff is too large to show. Please review the diff.
```
Change information
[iOS][apcv2] Add ARIA role extraction the iOS APC TreeWalker
- Updated the iOS APC extractor to support ARIA roles and
`content-visibility: hidden` styles.
- Adjusted annotated roles logic to support accumulating multiple
roles per element (e.g., combining tag-based roles, ARIA roles,
and the hidden category) to match Blink's exact behavior.
- Added explicit mapping from raw ARIA roles to the `AXRole` proto
enum when actionable mode is enabled.
- Refactored structure so that role extraction is only done once. Roles are now calculated early and passed down the tree,
eliminating redundant DOM queries during generic container checks.
- M ios/chrome/browser/intelligence/proto_wrappers/annotated_page_content_extraction_utils.mm
- M ios/chrome/browser/intelligence/proto_wrappers/page_context_extractor_java_script_feature_unittest.mm
- M ios/chrome/browser/intelligence/proto_wrappers/page_context_wrapper_unittest.mm
- M ios/chrome/browser/intelligence/proto_wrappers/resources/annotated_page_content_extraction.ts
- M ios/chrome/browser/intelligence/proto_wrappers/resources/page_content_types.ts
Code-Review: +1 by Nicolas MacBeth, +1 by Vincent Boisselle
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |