Changeset 291747 in webkit

Timestamp:

Mar 23, 2022, 10:00:33 AM (5 years ago)

Author:

Tyler Wilcock

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/accessibility/display-contents-search-traversal-expected.txt (added)
• LayoutTests/accessibility/display-contents-search-traversal.html (added)
• LayoutTests/platform/glib/TestExpectations (modified) (1 diff)
• LayoutTests/platform/ios/TestExpectations (modified) (1 diff)
• LayoutTests/platform/ios/accessibility/display-contents-search-traversal-expected.txt (added)
• LayoutTests/platform/win/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/accessibility/AXLogger.cpp (modified) (1 diff)
• Source/WebCore/accessibility/AccessibilityObject.h (modified) (3 diffs)
• Source/WebCore/accessibility/AccessibilityRenderObject.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-03-23  Tyler Wilcock  <tyler_w@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  <pangle@apple.com>
 

trunk/LayoutTests/platform/glib/TestExpectations

@@ -343 +343 @@
 
 # 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 ]

trunk/LayoutTests/platform/ios/TestExpectations

@@ -2115 +2115 @@
 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 ]

trunk/LayoutTests/platform/win/TestExpectations

@@ -479 +479 @@
 
 # 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 ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-03-23  Tyler Wilcock  <tyler_w@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  <youenn@apple.com>
 

trunk/Source/WebCore/accessibility/AXLogger.cpp

@@ -533 +533 @@
         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());

trunk/Source/WebCore/accessibility/AccessibilityObject.h

@@ -545 +545 @@
     bool hasTagName(const QualifiedName&) const override;
     String tagName() const override;
+    bool hasDisplayContents() const;
 
     VisiblePositionRange visiblePositionRange() const override { return VisiblePositionRange(); }
@@ -860 +861 @@
 
 #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
 {
@@ -870 +876 @@
 }
 #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 { }; }

trunk/Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -422 +422 @@
         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())
-        return nullptr;
-
+    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;
 }