AX: WebKit computes the wrong accessibility clickpoint for display:co…

…ntents links and headings

https://bugs.webkit.org/show_bug.cgi?id=255828
rdar://problem/108409898

Reviewed by Chris Fleizach.

AccessibilityRenderObject had special logic for computing click points
for headings and links and there is no reason this logic is specific to
an AX object with a renderer. To fix this, these implementations were
moved to AccessibilityObject. Other functions called from these click
point methods were moved, too.

* LayoutTests/accessibility/mac/heading-clickpoint-expected.txt:
* LayoutTests/accessibility/mac/heading-clickpoint.html:
Add display:contents testcase.

* Source/WebCore/accessibility/AccessibilityObject.cpp:
(WebCore::AccessibilityObject::selection const):
(WebCore::boundsForRects):
(WebCore::AccessibilityRenderObject::boundsForVisiblePositionRange const):
(WebCore::AccessibilityObject::boundsForRange const):
(WebCore::AccessibilityObject::linkClickPoint):
(WebCore::AccessibilityObject::clickPoint):
(WebCore::AccessibilityObject::clickPointFromElementRect const):
* Source/WebCore/accessibility/AccessibilityObject.h:
* Source/WebCore/accessibility/AccessibilityRenderObject.cpp:
(WebCore::AccessibilityRenderObject::linkClickPoint): Deleted.
(WebCore::AccessibilityRenderObject::clickPoint): Deleted.
(WebCore::AccessibilityRenderObject::selection const): Deleted.
(WebCore::boundsForRects): Deleted.
(WebCore::AccessibilityRenderObject::boundsForVisiblePositionRange const): Deleted.
(WebCore::AccessibilityRenderObject::boundsForRange const): Deleted.
* Source/WebCore/accessibility/AccessibilityRenderObject.h:
* Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h:

Canonical link: https://commits.webkit.org/263287@main

LayoutTests/accessibility/mac/heading-clickpoint-expected.txt

@@ -3,7 +3,9 @@ PASS: heading1.clickPointX === heading1.childAtIndex(0).clickPointX
 PASS: heading1.clickPointY === heading1.childAtIndex(0).clickPointY
 PASS: heading2.clickPointX === heading2.childAtIndex(0).clickPointX
 PASS: heading2.clickPointY === heading2.childAtIndex(0).clickPointY
-PASS: heading3.clickPointX == heading3.childAtIndex(0).clickPointX === false
+PASS: heading3.clickPointX === heading3.childAtIndex(0).clickPointX
+PASS: heading3.clickPointY === heading3.childAtIndex(0).clickPointY
+PASS: heading4.clickPointX == heading4.childAtIndex(0).clickPointX === false
 
 PASS successfullyParsed is true
 

LayoutTests/accessibility/mac/heading-clickpoint.html

@@ -9,8 +9,9 @@
 <div id="content">
 <h1 id="heading1">Small heading</h1>
 <h1 id="heading2"><a href="#">A heading that is just a little bit wider</a></h1>
+<h1 id="heading3" style="display:contents"><a href="#">A heading that is just a little bit wider and has display:contents</a></h1>
 
-<h1 id="heading3"><a href="#">Small heading</a> <a href="#">Small heading</a></h1>
+<h1 id="heading4"><a href="#">Small heading</a> <a href="#">Small heading</a></h1>
 </div>
 
 <script>
@@ -27,8 +28,12 @@ <h1 id="heading3"><a href="#">Small heading</a> <a href="#">Small heading</a></h
         output += expect("heading2.clickPointY", "heading2.childAtIndex(0).clickPointY");
 
         var heading3 = accessibilityController.accessibleElementById("heading3");
+        output += expect("heading3.clickPointX", "heading3.childAtIndex(0).clickPointX");
+        output += expect("heading3.clickPointY", "heading3.childAtIndex(0).clickPointY");
+
+        var heading4 = accessibilityController.accessibleElementById("heading4");
         // If there is more than one child, the click point should be the middle again, which means X point will be different.
-        output += expect("heading3.clickPointX == heading3.childAtIndex(0).clickPointX", "false");
+        output += expect("heading4.clickPointX == heading4.childAtIndex(0).clickPointX", "false");
 
         debug(output);
         document.getElementById("content").style.visibility = "hidden";

Source/WebCore/accessibility/AccessibilityObject.cpp

@@ -52,6 +52,7 @@
 #include "FocusController.h"
 #include "FrameLoader.h"
 #include "FrameSelection.h"
+#include "GeometryUtilities.h"
 #include "HTMLBodyElement.h"
 #include "HTMLDataListElement.h"
 #include "HTMLDetailsElement.h"
@@ -689,6 +690,13 @@ std::optional<SimpleRange> AccessibilityObject::rangeOfStringClosestToRangeInDir
     return closestStringRange;
 }
 
+VisibleSelection AccessibilityObject::selection() const
+{
+    auto* document = this->document();
+    auto* frame = document ? document->frame() : nullptr;
+    return frame ? frame->selection().selection() : VisibleSelection();
+}
+
 // Returns an collapsed range preceding the document contents if there is no selection.
 // FIXME: Why is that behavior more useful than returning null in that case?
 std::optional<SimpleRange> AccessibilityObject::selectionRange() const
@@ -1080,7 +1088,110 @@ bool AccessibilityObject::isMeter() const
     return renderer && renderer->isMeter();
 }
 
+static IntRect boundsForRects(const LayoutRect& rect1, const LayoutRect& rect2, const SimpleRange& dataRange)
+{
+    LayoutRect ourRect = rect1;
+    ourRect.unite(rect2);
+
+    // If the rectangle spans lines and contains multiple text characters, use the range's bounding box intead.
+    if (rect1.maxY() != rect2.maxY() && characterCount(dataRange) > 1) {
+        if (auto boundingBox = unionRect(RenderObject::absoluteTextRects(dataRange)); !boundingBox.isEmpty())
+            ourRect = boundingBox;
+    }
+
+    return snappedIntRect(ourRect);
+}
+
+IntRect AccessibilityRenderObject::boundsForVisiblePositionRange(const VisiblePositionRange& visiblePositionRange) const
+{
+    if (visiblePositionRange.isNull())
+        return IntRect();
+
+    // Create a mutable VisiblePositionRange.
+    VisiblePositionRange range(visiblePositionRange);
+    LayoutRect rect1 = range.start.absoluteCaretBounds();
+    LayoutRect rect2 = range.end.absoluteCaretBounds();
+
+    // Readjust for position at the edge of a line. This is to exclude line rect that doesn't need to be accounted in the range bounds
+    if (rect2.y() != rect1.y()) {
+        VisiblePosition endOfFirstLine = endOfLine(range.start);
+        if (range.start == endOfFirstLine) {
+            range.start.setAffinity(Affinity::Downstream);
+            rect1 = range.start.absoluteCaretBounds();
+        }
+        if (range.end == endOfFirstLine) {
+            range.end.setAffinity(Affinity::Upstream);
+            rect2 = range.end.absoluteCaretBounds();
+        }
+    }
+
+    return boundsForRects(rect1, rect2, *makeSimpleRange(range));
+}
+
+IntRect AccessibilityObject::boundsForRange(const SimpleRange& range) const
+{
+    auto cache = axObjectCache();
+    if (!cache)
+        return { };
+
+    auto start = cache->startOrEndCharacterOffsetForRange(range, true);
+    auto end = cache->startOrEndCharacterOffsetForRange(range, false);
+
+    auto rect1 = cache->absoluteCaretBoundsForCharacterOffset(start);
+    auto rect2 = cache->absoluteCaretBoundsForCharacterOffset(end);
+
+    // Readjust for position at the edge of a line. This is to exclude line rect that doesn't need to be accounted in the range bounds.
+    if (rect2.y() != rect1.y()) {
+        auto endOfFirstLine = cache->endCharacterOffsetOfLine(start);
+        if (start.isEqual(endOfFirstLine)) {
+            start = cache->nextCharacterOffset(start, false);
+            rect1 = cache->absoluteCaretBoundsForCharacterOffset(start);
+        }
+        if (end.isEqual(endOfFirstLine)) {
+            end = cache->previousCharacterOffset(end, false);
+            rect2 = cache->absoluteCaretBoundsForCharacterOffset(end);
+        }
+    }
+
+    return boundsForRects(rect1, rect2, range);
+}
+
+IntPoint AccessibilityObject::linkClickPoint()
+{
+    ASSERT(isLink());
+    /* A link bounding rect can contain points that are not part of the link.
+     For instance, a link that starts at the end of a line and finishes at the
+     beginning of the next line will have a bounding rect that includes the
+     entire two lines. In such a case, the middle point of the bounding rect
+     may not belong to the link element and thus may not activate the link.
+     Hence, return the middle point of the first character in the link if exists.
+     */
+    if (auto range = simpleRange()) {
+        auto start = VisiblePosition { makeContainerOffsetPosition(range->start) };
+        auto end = start.next();
+        if (contains<ComposedTree>(*range, makeBoundaryPoint(end)))
+            return { boundsForRange(*makeSimpleRange(start, end)).center() };
+    }
+    return clickPointFromElementRect();
+}
+
 IntPoint AccessibilityObject::clickPoint()
+{
+    // Headings are usually much wider than their textual content. If the mid point is used, often it can be wrong.
+    if (isHeading() && children().size() == 1)
+        return children().first()->clickPoint();
+
+    if (isLink())
+        return linkClickPoint();
+
+    // use the default position unless this is an editable web area, in which case we use the selection bounds.
+    if (!isWebArea() || !canSetValueAttribute())
+        return clickPointFromElementRect();
+
+    return boundsForVisiblePositionRange(selection()).center();
+}
+
+IntPoint AccessibilityObject::clickPointFromElementRect() const
 {
     return roundedIntPoint(elementRect().center());
 }

Source/WebCore/accessibility/AccessibilityObject.h

@@ -429,7 +429,8 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     FloatPoint screenRelativePosition() const final { return convertFrameToSpace(elementRect(), AccessibilityConversionSpace::Screen).location(); }
 #endif
     IntSize size() const final { return snappedIntRect(elementRect()).size(); }
-    IntPoint clickPoint() override;
+    IntPoint clickPoint() final;
+    IntPoint clickPointFromElementRect() const;
     static IntRect boundingBoxForQuads(RenderObject*, const Vector<FloatQuad>&);
     Path elementPath() const override { return Path(); }
     bool supportsPath() const override { return false; }
@@ -439,7 +440,7 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     int insertionPointLineNumber() const override { return -1; }
 
     URL url() const override { return URL(); }
-    VisibleSelection selection() const override { return VisibleSelection(); }
+    VisibleSelection selection() const final;
     String selectedText() const override { return String(); }
     String accessKey() const override { return nullAtom(); }
     String localizedActionVerb() const override;
@@ -552,7 +553,7 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     static String stringForVisiblePositionRange(const VisiblePositionRange&);
     String stringForRange(const SimpleRange&) const override;
     IntRect boundsForVisiblePositionRange(const VisiblePositionRange&) const override { return IntRect(); }
-    IntRect boundsForRange(const SimpleRange&) const override { return IntRect(); }
+    IntRect boundsForRange(const SimpleRange&) const final;
     void setSelectedVisiblePositionRange(const VisiblePositionRange&) const override { }
 
     VisiblePosition visiblePositionForPoint(const IntPoint&) const override { return VisiblePosition(); }
@@ -799,6 +800,9 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr<Accessibi
     void setLastKnownIsIgnoredValue(bool);
     void ariaTreeRows(AccessibilityChildrenVector& rows, AccessibilityChildrenVector& ancestors);
 
+    // Special handling of click point for links.
+    IntPoint linkClickPoint();
+
 protected: // FIXME: Make the data members private.
     AccessibilityChildrenVector m_children;
     mutable bool m_childrenInitialized { false };

Source/WebCore/accessibility/AccessibilityRenderObject.cpp

@@ -45,7 +45,6 @@
 #include "FloatRect.h"
 #include "FrameLoader.h"
 #include "FrameSelection.h"
-#include "GeometryUtilities.h"
 #include "HTMLAreaElement.h"
 #include "HTMLBRElement.h"
 #include "HTMLDetailsElement.h"
@@ -961,41 +960,6 @@ Path AccessibilityRenderObject::elementPath() const
     return Path();
 }
 
-IntPoint AccessibilityRenderObject::linkClickPoint()
-{
-    ASSERT(isLink());
-    /* A link bounding rect can contain points that are not part of the link.
-     For instance, a link that starts at the end of a line and finishes at the
-     beginning of the next line will have a bounding rect that includes the
-     entire two lines. In such a case, the middle point of the bounding rect
-     may not belong to the link element and thus may not activate the link.
-     Hence, return the middle point of the first character in the link if exists.
-     */
-    if (auto range = simpleRange()) {
-        auto start = VisiblePosition { makeContainerOffsetPosition(range->start) };
-        auto end = start.next();
-        if (contains<ComposedTree>(*range, makeBoundaryPoint(end)))
-            return { boundsForRange(*makeSimpleRange(start, end)).center() };
-    }
-    return AccessibilityObject::clickPoint();
-}
-
-IntPoint AccessibilityRenderObject::clickPoint()
-{
-    // Headings are usually much wider than their textual content. If the mid point is used, often it can be wrong.
-    if (isHeading() && children().size() == 1)
-        return children().first()->clickPoint();
-
-    if (isLink())
-        return linkClickPoint();
-
-    // use the default position unless this is an editable web area, in which case we use the selection bounds.
-    if (!isWebArea() || !canSetValueAttribute())
-        return AccessibilityObject::clickPoint();
-
-    return boundsForVisiblePositionRange(selection()).center();
-}
-
 AccessibilityObject* AccessibilityRenderObject::internalLinkElement() const
 {
     auto element = anchorElement();
@@ -1508,11 +1472,6 @@ String AccessibilityRenderObject::selectedText() const
     return doAXStringForRange(documentBasedSelectedTextRange());
 }
 
-VisibleSelection AccessibilityRenderObject::selection() const
-{
-    return m_renderer ? m_renderer->frame().selection().selection() : VisibleSelection();
-}
-
 PlainTextRange AccessibilityRenderObject::selectedTextRange() const
 {
     ASSERT(isTextControl());
@@ -1893,74 +1852,6 @@ bool AccessibilityRenderObject::nodeIsTextControl(const Node* node) const
     return false;
 }
 
-static IntRect boundsForRects(const LayoutRect& rect1, const LayoutRect& rect2, const SimpleRange& dataRange)
-{
-    LayoutRect ourRect = rect1;
-    ourRect.unite(rect2);
-
-    // If the rectangle spans lines and contains multiple text characters, use the range's bounding box intead.
-    if (rect1.maxY() != rect2.maxY() && characterCount(dataRange) > 1) {
-        if (auto boundingBox = unionRect(RenderObject::absoluteTextRects(dataRange)); !boundingBox.isEmpty())
-            ourRect = boundingBox;
-    }
-
-    return snappedIntRect(ourRect);
-}
-
-IntRect AccessibilityRenderObject::boundsForVisiblePositionRange(const VisiblePositionRange& visiblePositionRange) const
-{
-    if (visiblePositionRange.isNull())
-        return IntRect();
-
-    // Create a mutable VisiblePositionRange.
-    VisiblePositionRange range(visiblePositionRange);
-    LayoutRect rect1 = range.start.absoluteCaretBounds();
-    LayoutRect rect2 = range.end.absoluteCaretBounds();
-
-    // readjust for position at the edge of a line.  This is to exclude line rect that doesn't need to be accounted in the range bounds
-    if (rect2.y() != rect1.y()) {
-        VisiblePosition endOfFirstLine = endOfLine(range.start);
-        if (range.start == endOfFirstLine) {
-            range.start.setAffinity(Affinity::Downstream);
-            rect1 = range.start.absoluteCaretBounds();
-        }
-        if (range.end == endOfFirstLine) {
-            range.end.setAffinity(Affinity::Upstream);
-            rect2 = range.end.absoluteCaretBounds();
-        }
-    }
-
-    return boundsForRects(rect1, rect2, *makeSimpleRange(range));
-}
-
-IntRect AccessibilityRenderObject::boundsForRange(const SimpleRange& range) const
-{
-    auto cache = axObjectCache();
-    if (!cache)
-        return { };
-
-    auto start = cache->startOrEndCharacterOffsetForRange(range, true);
-    auto end = cache->startOrEndCharacterOffsetForRange(range, false);
-
-    auto rect1 = cache->absoluteCaretBoundsForCharacterOffset(start);
-    auto rect2 = cache->absoluteCaretBoundsForCharacterOffset(end);
-
-    // Readjust for position at the edge of a line. This is to exclude line rect that doesn't need to be accounted in the range bounds.
-    if (rect2.y() != rect1.y()) {
-        auto endOfFirstLine = cache->endCharacterOffsetOfLine(start);
-        if (start.isEqual(endOfFirstLine)) {
-            start = cache->nextCharacterOffset(start, false);
-            rect1 = cache->absoluteCaretBoundsForCharacterOffset(start);
-        }
-        if (end.isEqual(endOfFirstLine)) {
-            end = cache->previousCharacterOffset(end, false);
-            rect2 = cache->absoluteCaretBoundsForCharacterOffset(end);
-        }
-    }
-
-    return boundsForRects(rect1, rect2, range);
-}
-
 bool AccessibilityRenderObject::isVisiblePositionRangeInDifferentDocument(const VisiblePositionRange& range) const
 {
     if (range.start.isNull() || range.end.isNull())

Source/WebCore/accessibility/AccessibilityRenderObject.h

@@ -90,8 +90,7 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
 
     LayoutRect boundingBoxRect() const override;
     LayoutRect elementRect() const override;
-    IntPoint clickPoint() override;
-
+
     RenderObject* renderer() const override { return m_renderer.get(); }
     RenderBoxModelObject* renderBoxModelObject() const;
     Node* node() const override;
@@ -104,7 +103,6 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
     URL url() const override;
     PlainTextRange selectedTextRange() const override;
     int insertionPointLineNumber() const override;
-    VisibleSelection selection() const override;
     String stringValue() const override;
     String helpText() const override;
     String textUnderElement(AccessibilityTextUnderElementMode = AccessibilityTextUnderElementMode()) const override;
@@ -127,7 +125,6 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
     VisiblePositionRange visiblePositionRange() const override;
     VisiblePositionRange visiblePositionRangeForLine(unsigned) const override;
     IntRect boundsForVisiblePositionRange(const VisiblePositionRange&) const override;
-    IntRect boundsForRange(const SimpleRange&) const override;
     VisiblePositionRange selectedVisiblePositionRange() const override;
     void setSelectedVisiblePositionRange(const VisiblePositionRange&) const override;
     bool isVisiblePositionRangeInDifferentDocument(const VisiblePositionRange&) const;
@@ -227,8 +224,6 @@ class AccessibilityRenderObject : public AccessibilityNodeObject {
 #endif
 
     bool canHavePlainText() const;
-    // Special handling of click point for links.
-    IntPoint linkClickPoint();
 };
 
 } // namespace WebCore

Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

@@ -233,7 +233,7 @@ class AXIsolatedObject final : public AXCoreObject {
     String roleDescription() const override { return stringAttributeValue(AXPropertyName::RoleDescription); }
     String subrolePlatformString() const override { return stringAttributeValue(AXPropertyName::SubrolePlatformString); }
     LayoutRect elementRect() const override;
-    IntPoint clickPoint() override;
+    IntPoint clickPoint() final;
     void accessibilityText(Vector<AccessibilityText>& texts) const override;
     String brailleLabel() const override { return stringAttributeValue(AXPropertyName::BrailleLabel); }
     String brailleRoleDescription() const override { return stringAttributeValue(AXPropertyName::BrailleRoleDescription); }