- Revision
- 291747
- Author
- tyle...@apple.com
- Date
- 2022-03-23 10:00:33 -0700 (Wed, 23 Mar 2022)
Log Message
AccessibilityRenderObject::nextSibling should allow parent differences in the presence of display: contents
https://bugs.webkit.org/show_bug.cgi?id=238184
Reviewed by Andres Gonzalez.
Source/WebCore:
AccessibilityRenderObject::nextSibling currently has this logic to
return nullptr if the computed sibling has a parent different from `this`:
auto* nextObject = objectCache->getOrCreate(nextSibling);
if (nextObject && nextObject->parentObject() != this->parentObject())
return nullptr;
This is problematic in the presence of display: contents since we expect
parent object differences due to the way this property affects the render tree.
Concretely, this breaks the firstChild(), nextSibling() iteration we do throughout
WebKit (e.g. in AccessibilityRenderObject::addChildren), as when we get to an element
with display: contents we get a parent mismatch and iteration stops unnecessarily.
This patch fixes the issue by allowing a parent mismatch when either
object has display: contents, as we account for this difference in the
appropriate places (e.g. AccessibilityObject::insertChild).
Test: accessibility/display-contents-search-traversal.html
* accessibility/AXLogger.cpp:
(WebCore::operator<<):
Log when an object has display: contents. This property affects the
render tree and AX tree, so it's useful to log.
* accessibility/AccessibilityObject.h:
(WebCore::AccessibilityObject::hasDisplayContents const): Added.
* accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::nextSibling const):
LayoutTests:
* accessibility/display-contents-search-traversal-expected.txt: Added.
* accessibility/display-contents-search-traversal.html: Added.
* platform/glib/TestExpectations: Skip new test.
* platform/ios/TestExpectations: Enable new test.
* platform/ios/accessibility/display-contents-search-traversal-expected.txt: Added.
* platform/win/TestExpectations: Skip new test.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (291746 => 291747)
--- trunk/LayoutTests/ChangeLog 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/LayoutTests/ChangeLog 2022-03-23 17:00:33 UTC (rev 291747)
@@ -1,3 +1,17 @@
+2022-03-23 Tyler Wilcock <tyle...@apple.com>
+
+ AccessibilityRenderObject::nextSibling should allow parent differences in the presence of display: contents
+ https://bugs.webkit.org/show_bug.cgi?id=238184
+
+ Reviewed by Andres Gonzalez.
+
+ * accessibility/display-contents-search-traversal-expected.txt: Added.
+ * accessibility/display-contents-search-traversal.html: Added.
+ * platform/glib/TestExpectations: Skip new test.
+ * platform/ios/TestExpectations: Enable new test.
+ * platform/ios/accessibility/display-contents-search-traversal-expected.txt: Added.
+ * platform/win/TestExpectations: Skip new test.
+
2022-03-23 Patrick Angle <pan...@apple.com>
No breakpoints hit on github.com, and some are invalid
Added: trunk/LayoutTests/accessibility/display-contents-search-traversal-expected.txt (0 => 291747)
--- trunk/LayoutTests/accessibility/display-contents-search-traversal-expected.txt (rev 0)
+++ trunk/LayoutTests/accessibility/display-contents-search-traversal-expected.txt 2022-03-23 17:00:33 UTC (rev 291747)
@@ -0,0 +1,68 @@
+This test ensures we can traverse through basic webpages with multiple display: contents elements via search.
+
+AXRole: AXGroup
+
+AXRole: AXStaticText
+AXValue: Content before main.
+
+AXRole: AXGroup
+
+AXRole: AXHeading
+
+AXRole: AXStaticText
+AXValue: No style changes
+
+AXRole: AXToolbar
+
+AXRole: AXGroup
+
+AXRole: AXButton
+
+AXRole: AXHeading
+
+AXRole: AXStaticText
+AXValue: display: contents; on the toolbar
+
+AXRole: AXToolbar
+
+AXRole: AXGroup
+
+AXRole: AXButton
+
+AXRole: AXHeading
+
+AXRole: AXStaticText
+AXValue: display: contents; on the generic div
+
+AXRole: AXToolbar
+
+AXRole: AXGroup
+
+AXRole: AXButton
+
+AXRole: AXHeading
+
+AXRole: AXStaticText
+AXValue: display: contents; on the button
+
+AXRole: AXToolbar
+
+AXRole: AXGroup
+
+AXRole: AXButton
+
+AXRole: AXGroup
+
+AXRole: AXStaticText
+AXValue: Content after main.
+
+Traversed 25 elements.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
+
+
+
+
Added: trunk/LayoutTests/accessibility/display-contents-search-traversal.html (0 => 291747)
--- trunk/LayoutTests/accessibility/display-contents-search-traversal.html (rev 0)
+++ trunk/LayoutTests/accessibility/display-contents-search-traversal.html 2022-03-23 17:00:33 UTC (rev 291747)
@@ -0,0 +1,76 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+<script src=""
+</head>
+<body>
+
+<div id="container" role="group">
+ <p>Content before main.</p>
+
+ <main>
+
+ <h1>No style changes</h1>
+ <div role="toolbar">
+ <div>
+ <button>Foo</button>
+ </div>
+ </div>
+
+ <h1>display: contents; on the toolbar</h1>
+ <div role="toolbar" style="display: contents;">
+ <div>
+ <button>Foo</button>
+ </div>
+ </div>
+
+ <h1>display: contents; on the generic div</h1>
+ <div role="toolbar">
+ <div style="display: contents;">
+ <button>Foo</button>
+ </div>
+ </div>
+
+ <h1>display: contents; on the button</h1>
+ <div role="toolbar">
+ <div>
+ <button style="display: contents;">Foo</button>
+ </div>
+ </div>
+
+ </main>
+
+ <p>Content after main.</p>
+</div>
+
+<script>
+ var testOutput = "This test ensures we can traverse through basic webpages with multiple display: contents elements via search.\n";
+
+ if (window.accessibilityController) {
+ const containerElement = accessibilityController.accessibleElementById("container");
+
+ let elementCount = 0;
+ let searchResult = null;
+ while (true) {
+ searchResult = containerElement.uiElementForSearchPredicate(searchResult, true, "AXAnyTypeSearchKey", "", false);
+ if (!searchResult)
+ break;
+ const role = searchResult.role;
+ testOutput += `\n${role}`;
+ if (role.includes("StaticText")) {
+ let textContent = accessibilityController.platformName === "ios" ? searchResult.description : searchResult.stringValue;
+ testOutput += `\n${textContent}`;
+ }
+
+ testOutput += "\n";
+ elementCount += 1;
+ }
+ testOutput += `\nTraversed ${elementCount} elements.`;
+ document.getElementById("container").style.visibility = "hidden";
+ debug(testOutput);
+ }
+</script>
+</body>
+</html>
+
Modified: trunk/LayoutTests/platform/glib/TestExpectations (291746 => 291747)
--- trunk/LayoutTests/platform/glib/TestExpectations 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/LayoutTests/platform/glib/TestExpectations 2022-03-23 17:00:33 UTC (rev 291747)
@@ -342,6 +342,7 @@
accessibility/focusable-div.html [ Skip ]
# Missing AccessibilityUIElement::uiElementForSearchPredicate implementation.
+accessibility/display-contents-search-traversal.html [ Skip ]
accessibility/table-search-traversal.html [ Skip ]
accessibility/dynamically-changing-iframe-remains-accessible.html [ Skip ]
accessibility/ignore-modals-without-any-content.html [ Skip ]
Modified: trunk/LayoutTests/platform/ios/TestExpectations (291746 => 291747)
--- trunk/LayoutTests/platform/ios/TestExpectations 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/LayoutTests/platform/ios/TestExpectations 2022-03-23 17:00:33 UTC (rev 291747)
@@ -2114,6 +2114,7 @@
# Enable "aria-table-attributes" test for iOS
webkit.org/b/150366 accessibility/aria-table-attributes.html [ Pass ]
+accessibility/display-contents-search-traversal.html [ Pass ]
accessibility/table-search-traversal.html [ Pass ]
accessibility/dynamically-changing-iframe-remains-accessible.html [ Pass ]
accessibility/ignore-modals-without-any-content.html [ Pass ]
Added: trunk/LayoutTests/platform/ios/accessibility/display-contents-search-traversal-expected.txt (0 => 291747)
--- trunk/LayoutTests/platform/ios/accessibility/display-contents-search-traversal-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/ios/accessibility/display-contents-search-traversal-expected.txt 2022-03-23 17:00:33 UTC (rev 291747)
@@ -0,0 +1,38 @@
+This test ensures we can traverse through basic webpages with multiple display: contents elements via search.
+
+StaticText
+AXLabel: Content before main.
+
+StaticText
+AXLabel: No style changes
+
+Button
+
+StaticText
+AXLabel: display: contents; on the toolbar
+
+Button
+
+StaticText
+AXLabel: display: contents; on the generic div
+
+Button
+
+StaticText
+AXLabel: display: contents; on the button
+
+Button
+
+StaticText
+AXLabel: Content after main.
+
+Traversed 10 elements.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
+
+
+
+
Modified: trunk/LayoutTests/platform/win/TestExpectations (291746 => 291747)
--- trunk/LayoutTests/platform/win/TestExpectations 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/LayoutTests/platform/win/TestExpectations 2022-03-23 17:00:33 UTC (rev 291747)
@@ -478,6 +478,7 @@
accessibility/ancestor-computation.html [ Skip ]
# Missing AccessibilityUIElement::uiElementForSearchPredicate implementation.
+accessibility/display-contents-search-traversal.html [ Skip ]
accessibility/table-search-traversal.html [ Skip ]
accessibility/dynamically-changing-iframe-remains-accessible.html [ Skip ]
accessibility/ignore-modals-without-any-content.html [ Skip ]
Modified: trunk/Source/WebCore/ChangeLog (291746 => 291747)
--- trunk/Source/WebCore/ChangeLog 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/Source/WebCore/ChangeLog 2022-03-23 17:00:33 UTC (rev 291747)
@@ -1,3 +1,39 @@
+2022-03-23 Tyler Wilcock <tyle...@apple.com>
+
+ AccessibilityRenderObject::nextSibling should allow parent differences in the presence of display: contents
+ https://bugs.webkit.org/show_bug.cgi?id=238184
+
+ Reviewed by Andres Gonzalez.
+
+ AccessibilityRenderObject::nextSibling currently has this logic to
+ return nullptr if the computed sibling has a parent different from `this`:
+
+ auto* nextObject = objectCache->getOrCreate(nextSibling);
+ if (nextObject && nextObject->parentObject() != this->parentObject())
+ return nullptr;
+
+ This is problematic in the presence of display: contents since we expect
+ parent object differences due to the way this property affects the render tree.
+
+ Concretely, this breaks the firstChild(), nextSibling() iteration we do throughout
+ WebKit (e.g. in AccessibilityRenderObject::addChildren), as when we get to an element
+ with display: contents we get a parent mismatch and iteration stops unnecessarily.
+
+ This patch fixes the issue by allowing a parent mismatch when either
+ object has display: contents, as we account for this difference in the
+ appropriate places (e.g. AccessibilityObject::insertChild).
+
+ Test: accessibility/display-contents-search-traversal.html
+
+ * accessibility/AXLogger.cpp:
+ (WebCore::operator<<):
+ Log when an object has display: contents. This property affects the
+ render tree and AX tree, so it's useful to log.
+ * accessibility/AccessibilityObject.h:
+ (WebCore::AccessibilityObject::hasDisplayContents const): Added.
+ * accessibility/AccessibilityRenderObject.cpp:
+ (WebCore::AccessibilityRenderObject::nextSibling const):
+
2022-03-23 Youenn Fablet <you...@apple.com>
VideoFrame does not need to inherit from MediaSample
Modified: trunk/Source/WebCore/accessibility/AXLogger.cpp (291746 => 291747)
--- trunk/Source/WebCore/accessibility/AXLogger.cpp 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/Source/WebCore/accessibility/AXLogger.cpp 2022-03-23 17:00:33 UTC (rev 291747)
@@ -532,6 +532,8 @@
if (objectWithInterestingHTML)
stream.dumpProperty("outerHTML", objectWithInterestingHTML->outerHTML());
+ if (auto* axObject = dynamicDowncast<AccessibilityObject>(&object); axObject && axObject->hasDisplayContents())
+ stream.dumpProperty("hasDisplayContents", true);
stream.dumpProperty("address", &object);
stream.dumpProperty("wrapper", object.wrapper());
Modified: trunk/Source/WebCore/accessibility/AccessibilityObject.h (291746 => 291747)
--- trunk/Source/WebCore/accessibility/AccessibilityObject.h 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/Source/WebCore/accessibility/AccessibilityObject.h 2022-03-23 17:00:33 UTC (rev 291747)
@@ -544,6 +544,7 @@
int getIntegralAttribute(const QualifiedName&) const;
bool hasTagName(const QualifiedName&) const override;
String tagName() const override;
+ bool hasDisplayContents() const;
VisiblePositionRange visiblePositionRange() const override { return VisiblePositionRange(); }
VisiblePositionRange visiblePositionRangeForLine(unsigned) const override { return VisiblePositionRange(); }
@@ -859,6 +860,11 @@
};
#if ENABLE(ACCESSIBILITY)
+inline bool AccessibilityObject::hasDisplayContents() const
+{
+ return is<Element>(node()) && downcast<Element>(node())->hasDisplayContents();
+}
+
inline std::optional<BoundaryPoint> AccessibilityObject::lastBoundaryPointContainedInRect(const Vector<BoundaryPoint>& boundaryPoints, const BoundaryPoint& startBoundaryPoint, const FloatRect& targetRect) const
{
return lastBoundaryPointContainedInRect(boundaryPoints, startBoundaryPoint, targetRect, 0, boundaryPoints.size() - 1);
@@ -869,6 +875,7 @@
return previousLineStartPositionInternal(position).value_or(VisiblePosition());
}
#else
+inline bool AccessibilityObject::hasDisplayContents() const { return false; }
inline std::optional<BoundaryPoint> AccessibilityObject::lastBoundaryPointContainedInRect(const Vector<BoundaryPoint>&, const BoundaryPoint&, const FloatRect&) const { return std::nullopt; }
inline VisiblePosition AccessibilityObject::previousLineStartPosition(const VisiblePosition& position) const { return { }; }
#endif
Modified: trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp (291746 => 291747)
--- trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp 2022-03-23 16:40:09 UTC (rev 291746)
+++ trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp 2022-03-23 17:00:33 UTC (rev 291747)
@@ -421,11 +421,17 @@
if (!objectCache)
return nullptr;
+ auto* nextObject = objectCache->getOrCreate(nextSibling);
+ auto* nextObjectParent = nextObject ? nextObject->parentObject() : nullptr;
+ auto* thisParent = parentObject();
// Make sure next sibling has the same parent.
- auto* nextObject = objectCache->getOrCreate(nextSibling);
- if (nextObject && nextObject->parentObject() != this->parentObject())
+ if (nextObjectParent && nextObjectParent != thisParent) {
+ // Unless either object has a parent with display: contents, as display: contents can cause parent differences
+ // that we properly account for elsewhere.
+ if (nextObjectParent->hasDisplayContents() || (thisParent && thisParent->hasDisplayContents()))
+ return nextObject;
return nullptr;
-
+ }
return nextObject;
}
