Bug 237834 – AX: Include display: contents elements in the AX tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

Because display: contents intentionally prevents a render object from being generated for the element it's applied to, we don't find it as part of our normal render tree walk. We should include these elements in the tree some other way.

[Radar WebKit Bug Importer]

<rdar://problem/90252150>

[Tyler Wilcock]

Created attachment 454610 [details]
Patch

[Tyler Wilcock]

Created attachment 454611 [details]
Patch

[chris fleizach]

Comment on attachment 454611 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:454
> +    if (!node() || !RuntimeEnabledFeatures::sharedFeatures().cssDisplayContentsAXSupportEnabled())

can we put the runtime enablement check first. maybe on it's own line. it feels substantively different than the code
presumably we'll remove that check soon? or can we do it with this PR?

> Source/WebCore/accessibility/AccessibilityObject.cpp:458
> +    if (!is<Element>(parentNode) || !downcast<Element>(parentNode)->hasDisplayContents())

do we need to worry about a display contents parent who is 
aria-hidden=true or role=presentational?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 454611 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

+AccessibilityObject* AccessibilityObject::displayContentsParent() const
+{
...
+    if (auto* cache = axObjectCache())
+        return cache->getOrCreate(parentNode);
+
+    return nullptr;

Much prefer the two line, clear and compact:

auto* cache = axObjectCache();
return cache ? cache->getOrCreate(parentNode) : nullptr;

@@ -575,12 +591,17 @@ void AccessibilityObject::insertChild(AXCoreObject* newChild, unsigned index, De

+    auto* displayContentsParent = child->displayContentsParent();
+    // To avoid double-inserting a child of a `display: contents` element, only insert if `this` is the rightful parent.
+    if (displayContentsParent && displayContentsParent != this)
+        return;
+

Why would it try to double insert a child in the first place?

--- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

+// Some elements don't have an associated render object, meaning they won't be picked up by a walk of the render tree.
+// For example, nodes that are `aria-hidden="false"` and `hidden`, or elements with `display: contents`.
+// This function will find and add these elements to the AX tree.
+void AccessibilityRenderObject::addNodeOnlyChildren()

This makes me think that this belongs to AXNodeObject and not to AXRenderObject. Can we keep AXRenderObject for nodes that do have a renderer, and move this to AXNodeObject? 

--- a/LayoutTests/accessibility/display-contents-element-roles.html
+++ a/LayoutTests/accessibility/display-contents-element-roles.html

Can we have the elements in alphabetical order when possible? Similar to other tests, I think it makes it easier to find what we are missing later on.

+    function verifyRole(elementId) {
+        const outerHTML = escapeHTML(document.getElementById(elementId).cloneNode().outerHTML);

Do we need to cloneNode to get the outerHTML?

+    if (window.accessibilityController) {
+        verifyRole("p");
+        verifyRole("link");
...

Wondering if we should follow what we do in other tests where we get all elements at once and make verifyRole a visitor on each item of the array.

[Tyler Wilcock]

Created attachment 454717 [details]
Patch

[Tyler Wilcock]

> +    // To avoid double-inserting a child of a `display: contents` element,
> only insert if `this` is the rightful parent.
> +    if (displayContentsParent && displayContentsParent != this)
> +        return;
> +
> 
> Why would it try to double insert a child in the first place?
Because display:contents moves the element's children up a level in the render tree, making them accessible by the render tree parent of display:contents. That means both this grandparent, and the actual display:contents parent (in addNodeOnlyChildren) will try to insert these children.

> --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> 
> +// Some elements don't have an associated render object, meaning they won't
> be picked up by a walk of the render tree.
> +// For example, nodes that are `aria-hidden="false"` and `hidden`, or
> elements with `display: contents`.
> +// This function will find and add these elements to the AX tree.
> +void AccessibilityRenderObject::addNodeOnlyChildren()
> 
> This makes me think that this belongs to AXNodeObject and not to
> AXRenderObject. Can we keep AXRenderObject for nodes that do have a
> renderer, and move this to AXNodeObject? 
Well, the default behavior of AXNodeObject is already to add node only children (that's what AccessibilityNodeObject::addChildren does). Only AXRenderObject needs to addNodeOnlyChildren in this special way, since its normal means of adding children is via the render tree. So I think it makes sense that this method exists on AXRenderObject, but maybe I'm misunderstanding the suggestion.

> +    function verifyRole(elementId) {
> +        const outerHTML =
> escapeHTML(document.getElementById(elementId).cloneNode().outerHTML);
> 
> Do we need to cloneNode to get the outerHTML?
We do. Both outerHTML and innerHTML return an element's contents in the returned string (so outerHTML on <ol> would include the <li> elements). Performing a cloneNode first ensures we only capture the outermost node in the string (since cloneNode is shallow by default).

[Tyler Wilcock]

> can we put the runtime enablement check first. maybe on it's own line. it
> feels substantively different than the code
> presumably we'll remove that check soon? or can we do it with this PR?
I decided to just not use the flag for now. My original thought process was that exposing these elements without porting all functionality might provide a worse experience than status quo (these elements not existing at all in the AX tree), but I'm not so sure about that anymore. I'll remove the flag entirely in a later patch if we still feel we don't need it.

> > Source/WebCore/accessibility/AccessibilityObject.cpp:458
> > +    if (!is<Element>(parentNode) || !downcast<Element>(parentNode)->hasDisplayContents())
> 
> do we need to worry about a display contents parent who is 
> aria-hidden=true or role=presentational?
I don't believe so, similar to how it's OK if we return an aria-hidden or presentational parent from AXCoreObject::parentObject (vs. parentObjectUnignored). Children can be inserted to an ignored display:contents parent, or we can return an ignored display:contents parent, and things will work as they do if the element weren't display:contents. But maybe I'm missing something -- do you see this being problematic?

[chris fleizach]

(In reply to Tyler Wilcock from comment #8)
> > can we put the runtime enablement check first. maybe on it's own line. it
> > feels substantively different than the code
> > presumably we'll remove that check soon? or can we do it with this PR?
> I decided to just not use the flag for now. My original thought process was
> that exposing these elements without porting all functionality might provide
> a worse experience than status quo (these elements not existing at all in
> the AX tree), but I'm not so sure about that anymore. I'll remove the flag
> entirely in a later patch if we still feel we don't need it.
> 

Ok

> > > Source/WebCore/accessibility/AccessibilityObject.cpp:458
> > > +    if (!is<Element>(parentNode) || !downcast<Element>(parentNode)->hasDisplayContents())
> > 
> > do we need to worry about a display contents parent who is 
> > aria-hidden=true or role=presentational?
> I don't believe so, similar to how it's OK if we return an aria-hidden or
> presentational parent from AXCoreObject::parentObject (vs.
> parentObjectUnignored). Children can be inserted to an ignored
> display:contents parent, or we can return an ignored display:contents
> parent, and things will work as they do if the element weren't
> display:contents. But maybe I'm missing something -- do you see this being
> problematic?

Maybe adding a test case to see what happens if an element is display contents and is aria hidden.

[James Craig]

*** Bug 185679 has been marked as a duplicate of this bug. ***

[James Craig]

test case from the dupe may still be useful
https://bugs.webkit.org/attachment.cgi?id=353831

[Tyler Wilcock]

Created attachment 455058 [details]
Patch

[Tyler Wilcock]

Created attachment 455152 [details]
Patch

[Tyler Wilcock]

Created attachment 455165 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> > --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > +++ a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp
> > 
> > +// Some elements don't have an associated render object, meaning they won't
> > be picked up by a walk of the render tree.
> > +// For example, nodes that are `aria-hidden="false"` and `hidden`, or
> > elements with `display: contents`.
> > +// This function will find and add these elements to the AX tree.
> > +void AccessibilityRenderObject::addNodeOnlyChildren()
> > 
> > This makes me think that this belongs to AXNodeObject and not to
> > AXRenderObject. Can we keep AXRenderObject for nodes that do have a
> > renderer, and move this to AXNodeObject? 
> Well, the default behavior of AXNodeObject is already to add node only
> children (that's what AccessibilityNodeObject::addChildren does). Only
> AXRenderObject needs to addNodeOnlyChildren in this special way, since its
> normal means of adding children is via the render tree. So I think it makes
> sense that this method exists on AXRenderObject, but maybe I'm
> misunderstanding the suggestion.

What I'm saying is that it would make more sense that AXRenderObject::addChildren adds the children that have an associated RednerObject, and since an AXRenderObject is an AXNodeObject, then it adds the children from the Node's hierarchy if needed, like the display content elements. I.e., have AXRenderObject::addChildren would call AXNOdeObject::addChildren to add those children that are not part of the RenderObject hierarchy but are part of the Node hierarchy. I believe that would be a cleaner and more maintainable design than mingling the traversals of RenderObjects and Nodes in the AXRenderObject subclass.

[Tyler Wilcock]

> What I'm saying is that it would make more sense that
> AXRenderObject::addChildren adds the children that have an associated
> RednerObject, and since an AXRenderObject is an AXNodeObject, then it adds
> the children from the Node's hierarchy if needed, like the display content
> elements. I.e., have AXRenderObject::addChildren would call
> AXNOdeObject::addChildren to add those children that are not part of the
> RenderObject hierarchy but are part of the Node hierarchy. I believe that
> would be a cleaner and more maintainable design than mingling the traversals
> of RenderObjects and Nodes in the AXRenderObject subclass.
Ah, OK, I think I understand your suggestion better. Agreed, this would probably be better homed in AXNodeObject. However, doing that wouldn’t be as straightforward as changing AXNodeObject::addChildren around a bit to accommodate this usecase, as AXRenderObject::addNodeOnlyObjects does a lot of work to splice the child into the right place, which AXNodeObject::addChildren doesn’t (and shouldn’t) have to worry about. 

This mechanism is not new to this patch, so in the name of incremental improvement I propose we save that refactoring for later.

[EWS]

Committed r291570 (248670@main): <https://commits.webkit.org/248670@main>

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