Bug 170679 - [ATK] Implement support for DPub ARIA roles
Summary: [ATK] Implement support for DPub ARIA roles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-10 09:48 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2017-04-20 01:48 PDT (History)
9 users (show)

See Also:


Attachments
Patch (39.03 KB, patch)
2017-04-10 10:05 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1015.02 KB, application/zip)
2017-04-10 11:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.79 MB, application/zip)
2017-04-10 11:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.09 MB, application/zip)
2017-04-10 11:10 PDT, Build Bot
no flags Details
Patch (40.51 KB, patch)
2017-04-10 11:33 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (54.06 KB, patch)
2017-04-12 08:20 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (54.07 KB, patch)
2017-04-12 08:24 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (888.71 KB, application/zip)
2017-04-12 10:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2017-04-10 09:48:05 PDT
[ATK] Implement support for DPub ARIA roles
Comment 1 Joanmarie Diggs (irc: joanie) 2017-04-10 10:05:43 PDT
Created attachment 306697 [details]
Patch
Comment 2 Joanmarie Diggs (irc: joanie) 2017-04-10 10:22:35 PDT
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!
Comment 3 Build Bot 2017-04-10 11:05:26 PDT
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
Comment 4 Build Bot 2017-04-10 11:05:27 PDT
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
Comment 5 Build Bot 2017-04-10 11:08:57 PDT
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
Comment 6 Build Bot 2017-04-10 11:08:58 PDT
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
Comment 7 Build Bot 2017-04-10 11:10:32 PDT
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
Comment 8 Build Bot 2017-04-10 11:10:33 PDT
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
Comment 9 Joanmarie Diggs (irc: joanie) 2017-04-10 11:33:19 PDT
Created attachment 306723 [details]
Patch
Comment 10 chris fleizach 2017-04-11 10:42:02 PDT
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)
Comment 11 Joanmarie Diggs (irc: joanie) 2017-04-11 12:03:47 PDT
(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)?
Comment 12 chris fleizach 2017-04-11 13:54:17 PDT
(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.
Comment 13 Joanmarie Diggs (irc: joanie) 2017-04-11 14:54:20 PDT
(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.
Comment 14 chris fleizach 2017-04-11 15:21:08 PDT
(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?
Comment 15 Joanmarie Diggs (irc: joanie) 2017-04-11 16:15:25 PDT
(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?
Comment 16 chris fleizach 2017-04-11 16:56:16 PDT
(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
Comment 17 Joanmarie Diggs (irc: joanie) 2017-04-12 08:20:26 PDT
Created attachment 306910 [details]
Patch
Comment 18 Build Bot 2017-04-12 08:21:34 PDT
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.
Comment 19 Joanmarie Diggs (irc: joanie) 2017-04-12 08:24:00 PDT
Created attachment 306911 [details]
Patch
Comment 20 Build Bot 2017-04-12 10:04:28 PDT
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
Comment 21 Build Bot 2017-04-12 10:04:29 PDT
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
Comment 22 Joanmarie Diggs (irc: joanie) 2017-04-12 10:15:39 PDT
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!
Comment 23 chris fleizach 2017-04-19 23:36:09 PDT
Comment on attachment 306911 [details]
Patch

looks good
Comment 24 WebKit Commit Bot 2017-04-20 01:48:57 PDT
Comment on attachment 306911 [details]
Patch

Clearing flags on attachment: 306911

Committed r215554: <http://trac.webkit.org/changeset/215554>
Comment 25 WebKit Commit Bot 2017-04-20 01:48:59 PDT
All reviewed patches have been landed.  Closing bug.