Bug 170679 – [ATK] Implement support for DPub ARIA roles

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joanmarie Diggs]

[ATK] Implement support for DPub ARIA roles

[Joanmarie Diggs]

Created attachment 306697 [details]
Patch

[Joanmarie Diggs]

James and Chris: Mind taking a look at this? A couple of the changes are those I had mentioned to James via email (doc-abstract not being a landmark; doc-pagebreak being a separator). A couple more are new, as I'd not noticed them until I was doing the implementation for ATK:

1. doc-biblioentry and doc-endnote subclass the ARIA listitem role. That, plus the fact that they map to my platform's list-item role, suggests to me that they should be mapped to ListItemRole rather than ApplicationGroupRole. But that wound up in your platform mapping being AXGroup without any AXSubrole value. That's consistent with how other listitem-role elements are mapped on your platform, but it seemed inconsistent with the nature of what you did in your implementation. So I forced the subrole. Should I have?

2. doc-notice and doc-tip subclass the ARIA note role and have corresponding mappings on my platform. But making this change results in a completely different mapping on your platform. For this one, I forced nothing and updated the expectations. If you agree with the new mappings, I'll update DPub AAM; if you don't, I'll do a new patch. Just let me know which. :)

Thanks!

[Build Bot]

Comment on attachment 306697 [details]
Patch

Attachment 306697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3512212

New failing tests:
accessibility/roles-exposed.html

[Build Bot]

Created attachment 306714 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306697 [details]
Patch

Attachment 306697 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3512167

New failing tests:
accessibility/roles-exposed.html

[Build Bot]

Created attachment 306717 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306697 [details]
Patch

Attachment 306697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3512236

New failing tests:
accessibility/roles-exposed.html

[Build Bot]

Created attachment 306719 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joanmarie Diggs]

Created attachment 306723 [details]
Patch

[chris fleizach]

Comment on attachment 306723 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3163
> +bool AccessibilityObject::isTextBlockGroup() const

this name seems a bit generic for what its doing. what do you want this capture?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2107
> +    if (role == ListItemRole && m_object->isTextBlockGroup())

not sure if this is correct. if we want to map doc-biblioentry to a list item, I think we want to expose the subrole to be whatever it would normally be (maybe nothing)

[Joanmarie Diggs]

(In reply to chris fleizach from comment #10)
> Comment on attachment 306723 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306723&action=review
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:3163
> > +bool AccessibilityObject::isTextBlockGroup() const
> 
> this name seems a bit generic for what its doing. what do you want this
> capture?

Anything that is (now or in the future) that is assigned GroupRole or ApplicationGroupRole and contains a block of text rather than a collection of user interface elements. On (at least) my platform, these two types of containers are exposed and treated differently. That's why we have things like:

*https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L2700
*https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp#L2803

Maybe what we should have are two WebCore AccessibilityRole values: one for text-container groups and one for ui-element groups. Thoughts?

> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2107
> > +    if (role == ListItemRole && m_object->isTextBlockGroup())
> 
> not sure if this is correct. if we want to map doc-biblioentry to a list
> item, I think we want to expose the subrole to be whatever it would normally
> be (maybe nothing)

It is normally (currently) nothing.

On a related note, why is there no subrole for list items (regardless of their origin/markup)?

[chris fleizach]

(In reply to Joanmarie Diggs (irc: joanie) from comment #11)
> (In reply to chris fleizach from comment #10)
> > Comment on attachment 306723 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=306723&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:3163
> > > +bool AccessibilityObject::isTextBlockGroup() const
> > 
> > this name seems a bit generic for what its doing. what do you want this
> > capture?
> 
> Anything that is (now or in the future) that is assigned GroupRole or
> ApplicationGroupRole and contains a block of text rather than a collection
> of user interface elements. On (at least) my platform, these two types of
> containers are exposed and treated differently. That's why we have things
> like:
> 
> *https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/accessibility/
> AccessibilityRenderObject.cpp#L2700
> *https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/accessibility/
> AccessibilityRenderObject.cpp#L2803
> 
> Maybe what we should have are two WebCore AccessibilityRole values: one for
> text-container groups and one for ui-element groups. Thoughts?

Won't it be hard to tell? It doesn't seem out of the realm of possibility that someone could have a link inside one of these <doc> elements

> 
> > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2107
> > > +    if (role == ListItemRole && m_object->isTextBlockGroup())
> > 
> > not sure if this is correct. if we want to map doc-biblioentry to a list
> > item, I think we want to expose the subrole to be whatever it would normally
> > be (maybe nothing)
> 
> It is normally (currently) nothing.
> 
> On a related note, why is there no subrole for list items (regardless of
> their origin/markup)?

Don't know, probably didn't find a need for it so far.

[Joanmarie Diggs]

(In reply to chris fleizach from comment #12)

> > Maybe what we should have are two WebCore AccessibilityRole values: one for
> > text-container groups and one for ui-element groups. Thoughts?
> 
> Won't it be hard to tell? It doesn't seem out of the realm of possibility
> that someone could have a link inside one of these <doc> elements

Not sure I follow.

A link inside an element doesn't change its text-block nature. To me (and my platform's mappings), all doc- elements which are currently mapped to ApplicationGroupRole are text-block elements and need to be mapped to ATK_ROLE_SECTION because their purpose is to display content for reading.

In contrast, the ARIA group role is not a text-block element. It needs to be mapped to ATK_ROLE_PANEL. Similarly, SVG's foreignObject (which currently maps to GroupRole, but perhaps should map to the new ApplicationGroupRole) is not a text-block element and also needs to be mapped to ATK_ROLE_PANEL. Ditto for the new ARIA figure role. There are probably a few others.

[chris fleizach]

(In reply to Joanmarie Diggs (irc: joanie) from comment #13)
> (In reply to chris fleizach from comment #12)
> 
> > > Maybe what we should have are two WebCore AccessibilityRole values: one for
> > > text-container groups and one for ui-element groups. Thoughts?
> > 
> > Won't it be hard to tell? It doesn't seem out of the realm of possibility
> > that someone could have a link inside one of these <doc> elements
> 
> Not sure I follow.
> 
> A link inside an element doesn't change its text-block nature. To me (and my
> platform's mappings), all doc- elements which are currently mapped to
> ApplicationGroupRole are text-block elements and need to be mapped to
> ATK_ROLE_SECTION because their purpose is to display content for reading.
> 
> In contrast, the ARIA group role is not a text-block element. It needs to be
> mapped to ATK_ROLE_PANEL. Similarly, SVG's foreignObject (which currently
> maps to GroupRole, but perhaps should map to the new ApplicationGroupRole)
> is not a text-block element and also needs to be mapped to ATK_ROLE_PANEL.
> Ditto for the new ARIA figure role. There are probably a few others.

That logic sounds correct. The only question in my mind is whether we can know if a group is a "text-block" element. semantically someone could set the role to one of the text-block groups, but they could put anything inside... does that matter?

[Joanmarie Diggs]

(In reply to chris fleizach from comment #14)
> (In reply to Joanmarie Diggs (irc: joanie) from comment #13)
> > (In reply to chris fleizach from comment #12)
> > 
> > > > Maybe what we should have are two WebCore AccessibilityRole values: one for
> > > > text-container groups and one for ui-element groups. Thoughts?
> > > 
> > > Won't it be hard to tell? It doesn't seem out of the realm of possibility
> > > that someone could have a link inside one of these <doc> elements
> > 
> > Not sure I follow.
> > 
> > A link inside an element doesn't change its text-block nature. To me (and my
> > platform's mappings), all doc- elements which are currently mapped to
> > ApplicationGroupRole are text-block elements and need to be mapped to
> > ATK_ROLE_SECTION because their purpose is to display content for reading.
> > 
> > In contrast, the ARIA group role is not a text-block element. It needs to be
> > mapped to ATK_ROLE_PANEL. Similarly, SVG's foreignObject (which currently
> > maps to GroupRole, but perhaps should map to the new ApplicationGroupRole)
> > is not a text-block element and also needs to be mapped to ATK_ROLE_PANEL.
> > Ditto for the new ARIA figure role. There are probably a few others.
> 
> That logic sounds correct. The only question in my mind is whether we can
> know if a group is a "text-block" element. semantically someone could set
> the role to one of the text-block groups, but they could put anything
> inside... does that matter?

If authors use ARIA roles (explicitly or implicitly) in ways for which they were not intended, I don't think it's our jobs (implementors, AT developers) to correct that. We should be consistent with the "AAM" specs.

Getting back to my earlier "two WebCore AccessibilityRole values" idea, assuming you're ok with that, what about Group and TextGroup?

Mind you, we may actually need a total of four roles, because what my platform cares about is the nature of the role (text content versus UI element container). But if my understanding is correct, what your platform cares about is the source of the role (explicit/author-provided versus implicit). So maybe:

* GroupRole (implicit, UI element container)
  - Examples: <foreignObject>, <svg>, things without another AccessibilityRole but are focusable
  - AXAPI: AXGroup, no subrole
  - ATK:   ATK_ROLE_PANEL

* ApplicationGroupRole (explicit, UI element container)
  - Examples: role="group", role="figure"
  - AXAPI: AXGroup, AXApplicationGroup
  - ATK:   ATK_ROLE_PANEL

* TextGroupRole (implicit, text content)
  - Examples: <p>, <div>, <td> when not exposed as table descendant, style format group elements
  - AXAPI: AXGroup, no subrole
  - ATK:   ATK_ROLE_SECTION

* ApplicationTextGroupRole (explicit, text content)
  - Examples: doc-* which are currently mapped to ApplicationGroupRole
  - AXAPI: AXGroup, AXApplicationGroup
  - ATK:   ATK_ROLE_SECTION

Thoughts?

[chris fleizach]

(In reply to Joanmarie Diggs (irc: joanie) from comment #15)
> (In reply to chris fleizach from comment #14)
> > (In reply to Joanmarie Diggs (irc: joanie) from comment #13)
> > > (In reply to chris fleizach from comment #12)
> > > 
> > > > > Maybe what we should have are two WebCore AccessibilityRole values: one for
> > > > > text-container groups and one for ui-element groups. Thoughts?
> > > > 
> > > > Won't it be hard to tell? It doesn't seem out of the realm of possibility
> > > > that someone could have a link inside one of these <doc> elements
> > > 
> > > Not sure I follow.
> > > 
> > > A link inside an element doesn't change its text-block nature. To me (and my
> > > platform's mappings), all doc- elements which are currently mapped to
> > > ApplicationGroupRole are text-block elements and need to be mapped to
> > > ATK_ROLE_SECTION because their purpose is to display content for reading.
> > > 
> > > In contrast, the ARIA group role is not a text-block element. It needs to be
> > > mapped to ATK_ROLE_PANEL. Similarly, SVG's foreignObject (which currently
> > > maps to GroupRole, but perhaps should map to the new ApplicationGroupRole)
> > > is not a text-block element and also needs to be mapped to ATK_ROLE_PANEL.
> > > Ditto for the new ARIA figure role. There are probably a few others.
> > 
> > That logic sounds correct. The only question in my mind is whether we can
> > know if a group is a "text-block" element. semantically someone could set
> > the role to one of the text-block groups, but they could put anything
> > inside... does that matter?
> 
> If authors use ARIA roles (explicitly or implicitly) in ways for which they
> were not intended, I don't think it's our jobs (implementors, AT developers)
> to correct that. We should be consistent with the "AAM" specs.
> 
> Getting back to my earlier "two WebCore AccessibilityRole values" idea,
> assuming you're ok with that, what about Group and TextGroup?
> 
> Mind you, we may actually need a total of four roles, because what my
> platform cares about is the nature of the role (text content versus UI
> element container). But if my understanding is correct, what your platform
> cares about is the source of the role (explicit/author-provided versus
> implicit). So maybe:
> 
> * GroupRole (implicit, UI element container)
>   - Examples: <foreignObject>, <svg>, things without another
> AccessibilityRole but are focusable
>   - AXAPI: AXGroup, no subrole
>   - ATK:   ATK_ROLE_PANEL
> 
> * ApplicationGroupRole (explicit, UI element container)
>   - Examples: role="group", role="figure"
>   - AXAPI: AXGroup, AXApplicationGroup
>   - ATK:   ATK_ROLE_PANEL
> 
> * TextGroupRole (implicit, text content)
>   - Examples: <p>, <div>, <td> when not exposed as table descendant, style
> format group elements
>   - AXAPI: AXGroup, no subrole
>   - ATK:   ATK_ROLE_SECTION
> 
> * ApplicationTextGroupRole (explicit, text content)
>   - Examples: doc-* which are currently mapped to ApplicationGroupRole
>   - AXAPI: AXGroup, AXApplicationGroup
>   - ATK:   ATK_ROLE_SECTION
> 
> Thoughts?

This works for me, although I would think that <Div> would go into the GroupRole by default

[Joanmarie Diggs]

Created attachment 306910 [details]
Patch

[Build Bot]

Attachment 306910 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joanmarie Diggs]

Created attachment 306911 [details]
Patch

[Build Bot]

Comment on attachment 306911 [details]
Patch

Attachment 306911 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3524610

New failing tests:
webrtc/negotiatedneeded-event-addStream.html

[Build Bot]

Created attachment 306916 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Joanmarie Diggs]

Comment on attachment 306911 [details]
Patch

(In reply to Build Bot from comment #20)
> Comment on attachment 306911 [details]
> Patch
> 
> Attachment 306911 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/3524610
> 
> New failing tests:
> webrtc/negotiatedneeded-event-addStream.html

I suspect this to be a false positive.

Chris: Please review when you have a chance. Thanks!

[chris fleizach]

Comment on attachment 306911 [details]
Patch

looks good

[WebKit Commit Bot]

Comment on attachment 306911 [details]
Patch

Clearing flags on attachment: 306911

Committed r215554: <http://trac.webkit.org/changeset/215554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.