[ATK] Implement support for DPub ARIA roles
Created attachment 306697 [details] Patch
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 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
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 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
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 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
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
Created attachment 306723 [details] Patch
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)
(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)?
(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.
(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.
(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?
(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?
(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
Created attachment 306910 [details] Patch
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.
Created attachment 306911 [details] Patch
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
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 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 on attachment 306911 [details] Patch looks good
Comment on attachment 306911 [details] Patch Clearing flags on attachment: 306911 Committed r215554: <http://trac.webkit.org/changeset/215554>
All reviewed patches have been landed. Closing bug.