Bug 254317 - AX: article speaks "landmark" and shows up in landmarks rotor on iOS-only
Summary: AX: article speaks "landmark" and shows up in landmarks rotor on iOS-only
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-03-22 23:02 PDT by James Craig
Modified: 2023-04-09 23:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.55 KB, patch)
2023-03-31 15:08 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2023-03-31 15:13 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2023-03-31 23:34 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (5.21 KB, patch)
2023-04-07 15:20 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2023-03-22 23:02:13 PDT
There were some comments on a public accessibility slack that WebKit/VoiceOver vends landmarks inconsistently between the Mac and iOS systems. In particular, role="article" is not in the landmarks list for the ARIA spec, but iOS treats it similar to one. The role description is spoken as "article, landmark" and articles appear in the landmark rotor. Neither of these is true on Mac.

Of note, the Mac platform roles for landmarks all start with AXLandmark*: AXLandmarkMain, AXLandmarkNavigation, etc... but article is exposed as "AXDocumentArticle" so the bug is probably either downstream in the iOS mapping or outside of WebKit.

The commenter's motivation is not clear to me the yet. I'm unsure if there is real user workflow hindered by the verbosity or rotor, or if the report is purely academic. 

In any case, I agree that the behavior between platforms should be consistent, unless the VO users on the QA team requested this difference for some reason.
Comment 1 Radar WebKit Bug Importer 2023-03-22 23:02:34 PDT
<rdar://problem/107118745>
Comment 2 James Craig 2023-03-23 14:54:45 PDT
Commenter has clarified it's the academic reason. He considers this a bug because articles are in the landmark rotor even though they aren't technically landmarks. 

Tracing it back, this decision was made on purpose (around 10 years ago and perhaps with the input of VO users on the Accessibility Design and Quality team) to reduce the proliferation of additional rotors, but we're now revisiting that topic.

Outcome indeterminate.
Comment 3 chris fleizach 2023-03-31 15:08:13 PDT
Created attachment 465715 [details]
Patch
Comment 4 chris fleizach 2023-03-31 15:13:15 PDT
Created attachment 465716 [details]
Patch
Comment 5 James Craig 2023-03-31 15:20:07 PDT
Comment on attachment 465716 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=465716&action=review

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:-670
> -        AccessibilityRole::LandmarkDocRegion,

What's the ARIA computedrole for LandmarkDocRegion? "region"? If so, that's included with the landmarks list https://w3c.github.io/aria/#landmark_roles
Comment 6 chris fleizach 2023-03-31 15:24:42 PDT
(In reply to James Craig from comment #5)
> Comment on attachment 465716 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465716&action=review
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:-670
> > -        AccessibilityRole::LandmarkDocRegion,
> 
> What's the ARIA computedrole for LandmarkDocRegion? "region"? If so, that's
> included with the landmarks list https://w3c.github.io/aria/#landmark_roles

Group role

{ AccessibilityRole::LandmarkDocRegion, NSAccessibilityGroupRole },
Comment 7 Tyler Wilcock 2023-03-31 16:00:53 PDT
(In reply to chris fleizach from comment #6)
> (In reply to James Craig from comment #5)
> > Comment on attachment 465716 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=465716&action=review
> > 
> > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:-670
> > > -        AccessibilityRole::LandmarkDocRegion,
> > 
> > What's the ARIA computedrole for LandmarkDocRegion? "region"? If so, that's
> > included with the landmarks list https://w3c.github.io/aria/#landmark_roles
> 
> Group role
> 
> { AccessibilityRole::LandmarkDocRegion, NSAccessibilityGroupRole },
The ARIA computed role for LandmarkDocRegion is "region" based on this sequence:

AccessibilityObject::computedRoleString() special-cases accessibilityrole::LandmarkDocRegion to defer to AccessibilityRole::LandmarkRegion:

https://github.com/WebKit/WebKit/blob/656c0aeae78bfa54346b7a5f5ed224574dd0b78d/Source/WebCore/accessibility/AccessibilityObject.cpp#L2573#L2574

Then we look up AccessibilityRole::LandmarkRegion in the reverse role map:

https://github.com/WebKit/WebKit/blob/656c0aeae78bfa54346b7a5f5ed224574dd0b78d/Source/WebCore/accessibility/AccessibilityObject.cpp#L2475

So to James' point, it seems like we shouldn't remove AccessibilityRole::LandmarkDocRegion from `_accessibilityLandmarkAncestor`.
Comment 8 chris fleizach 2023-03-31 17:11:41 PDT
(In reply to Tyler Wilcock from comment #7)
> (In reply to chris fleizach from comment #6)
> > (In reply to James Craig from comment #5)
> > > Comment on attachment 465716 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=465716&action=review
> > > 
> > > > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:-670
> > > > -        AccessibilityRole::LandmarkDocRegion,
> > > 
> > > What's the ARIA computedrole for LandmarkDocRegion? "region"? If so, that's
> > > included with the landmarks list https://w3c.github.io/aria/#landmark_roles
> > 
> > Group role
> > 
> > { AccessibilityRole::LandmarkDocRegion, NSAccessibilityGroupRole },
> The ARIA computed role for LandmarkDocRegion is "region" based on this
> sequence:
> 
> AccessibilityObject::computedRoleString() special-cases
> accessibilityrole::LandmarkDocRegion to defer to
> AccessibilityRole::LandmarkRegion:
> 
> https://github.com/WebKit/WebKit/blob/
> 656c0aeae78bfa54346b7a5f5ed224574dd0b78d/Source/WebCore/accessibility/
> AccessibilityObject.cpp#L2573#L2574
> 
> Then we look up AccessibilityRole::LandmarkRegion in the reverse role map:
> 
> https://github.com/WebKit/WebKit/blob/
> 656c0aeae78bfa54346b7a5f5ed224574dd0b78d/Source/WebCore/accessibility/
> AccessibilityObject.cpp#L2475
> 
> So to James' point, it seems like we shouldn't remove
> AccessibilityRole::LandmarkDocRegion from `_accessibilityLandmarkAncestor`.

Ok sounds good
Comment 9 chris fleizach 2023-03-31 23:34:26 PDT
Created attachment 465721 [details]
Patch
Comment 10 chris fleizach 2023-04-07 15:20:42 PDT
Created attachment 465814 [details]
Patch
Comment 11 Tyler Wilcock 2023-04-07 15:29:04 PDT
Comment on attachment 465814 [details]
Patch

r+ after iOS-simulator tests pass
Comment 12 EWS 2023-04-09 23:58:22 PDT
Committed 262763@main (ff11799319a9): <https://commits.webkit.org/262763@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465814 [details].