Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
AX: display:contents lists don't return the correct subrole
https://bugs.webkit.org/show_bug.cgi?id=256950
rdar://problem/109498423

Reviewed by Andres Gonzalez.

With this patch, any list-type element (ol, ul, dl, role="list", etc)
with display:contents will now properly be created as an
AccessibilityList instance rather than an AccessibilityNodeObject.

The significant effects of this are:
  1. The correct subrole is now computed for these elements, which is
     important because some ATs do change their behavior based on the
     various list subroles.
  2. These elements now use the AccessibilityList::determineAccessibilityRole()
     "is this an AX list or layout-only list" heuristics.

In support of #2, some tests had to be modified to have rendered list items in
order to still be considered lists.

We implement this by allowing the creation of AccessibilityList with only a node
instead of requiring a renderer.

The other, more significant change introduced in this patch is that many
AccessibilityRenderObject methods are now changed to fallback to the
corresponding AccessibilityNodeObject implementation when there is no
renderer rather than returning early.

This is required because we have many subclasses of AccessibilityRenderObject
that also need to support display:contents (AccessibilityList is one of these).
Making this change weakens our definition of AccessibilityRenderObject from
"should have a renderer" to "should have a renderer or a node".

* LayoutTests/platform/wpe/accessibility/aria-visible-element-roles-expected.txt:
* LayoutTests/platform/glib/accessibility/aria-visible-element-roles-expected.txt:
* LayoutTests/platform/glib/accessibility/display-contents-element-roles-expected.txt:
* LayoutTests/platform/ios/accessibility/display-contents-element-roles-expected.txt:
* LayoutTests/platform/mac-wk1/accessibility/aria-visible-element-roles-expected.txt:
* LayoutTests/accessibility/aria-visible-element-roles.html:
* LayoutTests/accessibility/display-contents-element-roles-expected.txt:
* LayoutTests/accessibility/display-contents-element-roles.html:
* LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles-expected.txt:
* Source/WebCore/accessibility/AXObjectCache.cpp:
(WebCore::shouldCreateAccessibilityList):
(WebCore::AXObjectCache::createObjectFromRenderer):
(WebCore::createFromNode):
(WebCore::AXObjectCache::getOrCreate):
* Source/WebCore/accessibility/AccessibilityList.cpp:
(WebCore::AccessibilityList::AccessibilityList):
(WebCore::AccessibilityList::create):
* Source/WebCore/accessibility/AccessibilityList.h:
* Source/WebCore/accessibility/AccessibilityNodeObject.cpp:
(WebCore::AccessibilityNodeObject::checkboxOrRadioRect const):
(WebCore::AccessibilityNodeObject::elementRect const):
(WebCore::AccessibilityNodeObject::addChildren):
(WebCore::AccessibilityNodeObject::canHaveChildren const):
(WebCore::AccessibilityNodeObject::computeAccessibilityIsIgnored const):
* Source/WebCore/accessibility/AccessibilityNodeObject.h:
* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::convertFrameToSpace const):
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::AccessibilityRenderObject):
(WebCore::AccessibilityRenderObject::firstChild const):
(WebCore::AccessibilityRenderObject::lastChild const):
(WebCore::AccessibilityRenderObject::previousSibling const):
(WebCore::AccessibilityRenderObject::nextSibling const):
(WebCore::AccessibilityRenderObject::parentObject const):
(WebCore::AccessibilityRenderObject::helpText const):
(WebCore::AccessibilityRenderObject::textUnderElement const):
(WebCore::AccessibilityRenderObject::node const):
(WebCore::AccessibilityRenderObject::stringValue const):
(WebCore::AccessibilityRenderObject::boundingBoxRect const):
(WebCore::AccessibilityRenderObject::defaultObjectInclusion const):
(WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored const):
(WebCore::AccessibilityRenderObject::document const):
(WebCore::AccessibilityRenderObject::documentFrameView const):
(WebCore::AccessibilityRenderObject::addChildren):
(WebCore::AccessibilityRenderObject::checkboxOrRadioRect const): Deleted.
(WebCore::AccessibilityRenderObject::elementRect const): Deleted.
(WebCore::AccessibilityRenderObject::canHaveChildren const): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.h:

Canonical link: https://commits.webkit.org/264644@main
  • Loading branch information
twilco committed May 29, 2023
1 parent e95baed commit ac7dd8d
Show file tree
Hide file tree
Showing 18 changed files with 145 additions and 112 deletions.
4 changes: 3 additions & 1 deletion LayoutTests/accessibility/aria-visible-element-roles.html
Expand Up @@ -111,7 +111,9 @@ <h2 hidden aria-hidden="false" id="h2">Hello world</h2>

<output hidden aria-hidden="false" id="output">Output</output>

<menu hidden aria-hidden="false" type="toolbar" id="menu-toolbar"></menu>
<menu hidden aria-hidden="false" type="toolbar" id="menu-toolbar">
<li hidden aria-hidden="false">Hello world</li>
</menu>

<hr hidden aria-hidden="false" id="hr"></hr>

Expand Down
Expand Up @@ -51,6 +51,7 @@ This test ensures elements with CSS display: contents have the correct role.

<dl class="testcase" id="dl"></dl>
AXRole: AXList
AXSubrole: AXDescriptionList

<dt class="testcase" id="dt"></dt>
AXRole: AXGroup
Expand Down Expand Up @@ -102,6 +103,11 @@ This test ensures elements with CSS display: contents have the correct role.
<menu class="testcase" type="toolbar" id="menu-toolbar"></menu>
AXRole: AXList
computedRoleString: list
AXSubrole: AXContentList

<li class="testcase" id="ul-li-element"></li>
AXRole: AXGroup
computedRoleString: listitem

<nav class="testcase" id="nav"></nav>
AXRole: AXGroup
Expand All @@ -111,6 +117,7 @@ This test ensures elements with CSS display: contents have the correct role.
<ol class="testcase" id="ol"></ol>
AXRole: AXList
computedRoleString: list
AXSubrole: AXContentList

<li class="testcase" id="ol-li-element"></li>
AXRole: AXGroup
Expand Down Expand Up @@ -155,6 +162,7 @@ This test ensures elements with CSS display: contents have the correct role.
<ul class="testcase" id="ul"></ul>
AXRole: AXList
computedRoleString: list
AXSubrole: AXContentList

<li class="testcase" id="ul-li-element"></li>
AXRole: AXGroup
Expand Down
Expand Up @@ -63,12 +63,16 @@ <h2 class="testcase" id="h2">Hello world</h2>

<mark class="testcase" id="mark">Marked text</mark>

<menu class="testcase" type="toolbar" id="menu-toolbar"></menu>
<menu class="testcase" type="toolbar" id="menu-toolbar">
<li class="testcase" id="ul-li-element">Hello world</li>
<li>list item with renderer</li>
</menu>

<nav class="testcase" id="nav">Nav</nav>

<ol class="testcase" id="ol">
<li class="testcase" id="ol-li-element">Hello world</li>
<li>list item with renderer</li>
</ol>

<output class="testcase" id="output">Output</output>
Expand All @@ -89,6 +93,7 @@ <h2 class="testcase" id="h2">Hello world</h2>

<ul class="testcase" id="ul">
<li class="testcase" id="ul-li-element">Hello world</li>
<li>list item with renderer</li>
</ul>
</div>

Expand Down
Expand Up @@ -105,12 +105,10 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
AXRole: AXDescriptionList

<ol hidden="" aria-hidden="false" id="ol"></ol>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<ul hidden="" aria-hidden="false" id="ul"></ul>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<figure hidden="" aria-hidden="false" id="figure"></figure>
AXRole: AXGroup
Expand Down Expand Up @@ -180,8 +178,7 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
computedRoleString: status

<menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<hr hidden="" aria-hidden="false" id="hr">
AXRole: AXSeparator
Expand Down
Expand Up @@ -89,6 +89,10 @@ This test ensures elements with CSS display: contents have the correct role.
AXRole: AXList
computedRoleString: list

<li class="testcase" id="ul-li-element"></li>
AXRole: AXListItem
computedRoleString: listitem

<nav class="testcase" id="nav"></nav>
AXRole: AXLandmarkNavigation
computedRoleString: navigation
Expand Down
Expand Up @@ -72,6 +72,9 @@ This test ensures elements with CSS display: contents have the correct role.
<menu class="testcase" type="toolbar" id="menu-toolbar"></menu>
List

<li class="testcase" id="ul-li-element"></li>
ListItem

<nav class="testcase" id="nav"></nav>
LandmarkNavigation

Expand Down
Expand Up @@ -114,14 +114,15 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria

<dl hidden="" aria-hidden="false" id="dl"></dl>
AXRole: AXList
AXSubrole: AXDescriptionList

<ol hidden="" aria-hidden="false" id="ol"></ol>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<ul hidden="" aria-hidden="false" id="ul"></ul>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<figure hidden="" aria-hidden="false" id="figure"></figure>
AXRole: AXGroup
Expand Down Expand Up @@ -203,8 +204,8 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
AXSubrole: AXApplicationStatus

<menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<hr hidden="" aria-hidden="false" id="hr">
AXRole: AXSplitter
Expand Down
Expand Up @@ -113,14 +113,15 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria

<dl hidden="" aria-hidden="false" id="dl"></dl>
AXRole: AXList
AXSubrole: AXDescriptionList

<ol hidden="" aria-hidden="false" id="ol"></ol>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<ul hidden="" aria-hidden="false" id="ul"></ul>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<figure hidden="" aria-hidden="false" id="figure"></figure>
AXRole: AXGroup
Expand Down Expand Up @@ -202,8 +203,8 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
AXSubrole: AXApplicationStatus

<menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup
AXSubrole: AXContentList

<hr hidden="" aria-hidden="false" id="hr">
AXRole: AXSplitter
Expand Down
Expand Up @@ -106,12 +106,10 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
AXRole: AXDescriptionList

<ol hidden="" aria-hidden="false" id="ol"></ol>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<ul hidden="" aria-hidden="false" id="ul"></ul>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<figure hidden="" aria-hidden="false" id="figure"></figure>
AXRole: AXGroup
Expand Down Expand Up @@ -181,8 +179,7 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria
computedRoleString: status

<menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu>
AXRole: AXList
computedRoleString: list
AXRole: AXGroup

<hr hidden="" aria-hidden="false" id="hr">
AXRole: AXSeparator
Expand Down
19 changes: 13 additions & 6 deletions Source/WebCore/accessibility/AXObjectCache.cpp
Expand Up @@ -597,15 +597,20 @@ static bool isSimpleImage(const RenderObject& renderer)
return true;
}

static bool isAccessibilityList(Node* node)
{
// If the node is aria role="list" or the aria role is empty and it's a
// ul/ol/dl type (it shouldn't be a list if aria says otherwise).
return (node && ((nodeHasRole(node, "list"_s) || nodeHasRole(node, "directory"_s))
|| (nodeHasRole(node, nullAtom()) && (node->hasTagName(ulTag) || node->hasTagName(olTag) || node->hasTagName(dlTag) || node->hasTagName(menuTag)))));
}

Ref<AccessibilityObject> AXObjectCache::createObjectFromRenderer(RenderObject* renderer)
{
// FIXME: How could renderer->node() ever not be an Element?
Node* node = renderer->node();

// If the node is aria role="list" or the aria role is empty and its a
// menu/ul/ol/dl type (it shouldn't be a list if aria says otherwise).
if (node && ((nodeHasRole(node, "list"_s) || nodeHasRole(node, "directory"_s))
|| (nodeHasRole(node, nullAtom()) && (node->hasTagName(menuTag) || node->hasTagName(ulTag) || node->hasTagName(olTag) || node->hasTagName(dlTag)))))
if (isAccessibilityList(node))
return AccessibilityList::create(renderer);

// aria tables
Expand Down Expand Up @@ -682,6 +687,8 @@ Ref<AccessibilityObject> AXObjectCache::createObjectFromRenderer(RenderObject* r

static Ref<AccessibilityObject> createFromNode(Node& node)
{
if (isAccessibilityList(&node))
return AccessibilityList::create(node);
return AccessibilityNodeObject::create(node);
}

Expand Down Expand Up @@ -742,7 +749,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node* node)

if (!node->parentElement())
return nullptr;

bool isOptionElement = is<HTMLOptionElement>(*node);
if (isOptionElement || is<HTMLOptGroupElement>(*node)) {
auto select = isOptionElement
Expand Down Expand Up @@ -788,7 +795,7 @@ AccessibilityObject* AXObjectCache::getOrCreate(Node* node)
// it will disappear when this function is finished, leading to a use-after-free.
if (newObject->isDetached())
return nullptr;

return newObject.get();
}

Expand Down
10 changes: 10 additions & 0 deletions Source/WebCore/accessibility/AccessibilityList.cpp
Expand Up @@ -46,13 +46,23 @@ AccessibilityList::AccessibilityList(RenderObject* renderer)
{
}

AccessibilityList::AccessibilityList(Node& node)
: AccessibilityRenderObject(node)
{
}

AccessibilityList::~AccessibilityList() = default;

Ref<AccessibilityList> AccessibilityList::create(RenderObject* renderer)
{
return adoptRef(*new AccessibilityList(renderer));
}

Ref<AccessibilityList> AccessibilityList::create(Node& node)
{
return adoptRef(*new AccessibilityList(node));
}

bool AccessibilityList::computeAccessibilityIsIgnored() const
{
return accessibilityIsIgnoredByDefault();
Expand Down
2 changes: 2 additions & 0 deletions Source/WebCore/accessibility/AccessibilityList.h
Expand Up @@ -35,11 +35,13 @@ namespace WebCore {
class AccessibilityList final : public AccessibilityRenderObject {
public:
static Ref<AccessibilityList> create(RenderObject*);
static Ref<AccessibilityList> create(Node&);
virtual ~AccessibilityList();

AccessibilityRole roleValue() const override;
private:
explicit AccessibilityList(RenderObject*);
explicit AccessibilityList(Node&);
bool isList() const override { return true; }
bool isUnorderedList() const override;
bool isOrderedList() const override;
Expand Down
47 changes: 36 additions & 11 deletions Source/WebCore/accessibility/AccessibilityNodeObject.cpp
Expand Up @@ -215,8 +215,27 @@ AccessibilityObject* AccessibilityNodeObject::parentObject() const
return nullptr;
}

LayoutRect AccessibilityNodeObject::checkboxOrRadioRect() const
{
auto* label = labelForElement(dynamicDowncast<Element>(node()));
if (!label || !label->renderer())
return boundingBoxRect();

auto* cache = axObjectCache();
if (!cache)
return boundingBoxRect();

// A checkbox or radio button should encompass its label.
auto labelRect = cache->getOrCreate(label)->elementRect();
labelRect.unite(boundingBoxRect());
return labelRect;
}

LayoutRect AccessibilityNodeObject::elementRect() const
{
if (isCheckboxOrRadio())
return checkboxOrRadioRect();

return boundingBoxRect();
}

Expand Down Expand Up @@ -262,6 +281,13 @@ Document* AccessibilityNodeObject::document() const
return &node()->document();
}

LocalFrameView* AccessibilityNodeObject::documentFrameView() const
{
if (auto* node = this->node())
return node->document().view();
return AccessibilityObject::documentFrameView();
}

AccessibilityRole AccessibilityNodeObject::determineAccessibilityRole()
{
AXTRACE("AccessibilityNodeObject::determineAccessibilityRole"_s);
Expand Down Expand Up @@ -490,13 +516,13 @@ void AccessibilityNodeObject::addChildren()
{
// If the need to add more children in addition to existing children arises,
// childrenChanged should have been called, leaving the object with no children.
ASSERT(!m_childrenInitialized);
ASSERT(!m_childrenInitialized);

if (!m_node)
return;

m_childrenInitialized = true;

if (!canHaveChildren())
return;

// The only time we add children from the DOM tree to a node with a renderer is when it's a canvas.
if (renderer() && !node()->hasTagName(canvasTag))
return;
Expand All @@ -507,22 +533,19 @@ void AccessibilityNodeObject::addChildren()

for (auto* child = node()->firstChild(); child; child = child->nextSibling())
addChild(objectCache->getOrCreate(child));

updateOwnedChildren();
m_subtreeDirty = false;
}

bool AccessibilityNodeObject::canHaveChildren() const
{
// If this is an AccessibilityRenderObject, then it's okay if this object
// doesn't have a node - there are some renderers that don't have associated
// nodes, like scroll areas and css-generated text.
if (!node() && !isAccessibilityRenderObject())
return false;

// When <noscript> is not being used (its renderer() == 0), ignore its children.
if (node() && !renderer() && node()->hasTagName(noscriptTag))
return false;
// If this is an AccessibilityRenderObject, then it's okay if this object
// doesn't have a node - there are some renderers that don't have associated
// nodes, like scroll areas and css-generated text.

// Elements that should not have children.
switch (roleValue()) {
Expand Down Expand Up @@ -572,6 +595,8 @@ bool AccessibilityNodeObject::computeAccessibilityIsIgnored() const
// it's been initialized.
ASSERT(m_initialized);
#endif
if (!node())
return true;

// Handle non-rendered text that is exposed through aria-hidden=false.
if (node() && node()->isTextNode() && !renderer()) {
Expand Down

0 comments on commit ac7dd8d

Please sign in to comment.