Bug 254317

Summary:

AX: article speaks "landmark" and shows up in landmarks rotor on iOS-only

Product:

WebKit

Reporter:

James Craig <jcraig>

Component:

Accessibility

Assignee:

chris fleizach <cfleizach>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jdiggs, samuel_white, tyler_w, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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].