AX: Unexpected speech synthesis behavior for unordered lists

https://bugs.webkit.org/show_bug.cgi?id=259030 rdar://112085797 Reviewed by Chris Fleizach. As noted in the FIXME comment in AccessibilityObject::stringForRange, passing the original range.start to listMarkerTextForNodeAndPosition() is incorrect and it.range.start() should be passed instead. This solves the problem of list markers getting inserted in the middle of list item text when the <li> element contains children like in item3 of the layout test. The new layout test exercises setting and retrieving selection in list items, coverage that was missing in the test suite. In addition, listMarkerTextForNodeAndPosition(...) was optimized to avoid walking up the RenderObject hierarchy twice unnecessarily for list items. Some code cleanup. * LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items-expected.txt: Added. * LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items.html: Added. * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::renderListItemContainerForNode): (WebCore::AccessibilityObject::listMarkerTextForNodeAndPosition): (WebCore::AccessibilityObject::stringForRange const): (WebCore::listMarkerTextForNode): Deleted. Canonical link: https://commits.webkit.org/266383@main
WebKit · Jul 28, 2023 · 61c2f70 · 61c2f70
1 parent 522fb19
commit 61c2f70
Show file tree

Hide file tree

Showing 3 changed files with 119 additions and 23 deletions.
diff --git a/LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items-expected.txt b/LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items-expected.txt
@@ -0,0 +1,17 @@
+Tests setting and retrieving text selection in list items using TextMarkerRanges.
+
+Check that the selection range contains the text for the second item plus the list marker (bullet):
+PASS: selection === expectedSelection
+Check that the selection range contains the first item text plus the list marker (bullet):
+PASS: selection === expectedSelection
+Check that the selection range contains the static text plus the list marker (bullet):
+PASS: selection === expectedSelection
+Check that the selection range doesn't include the first character of the static text nor the list marker after moving the start of the range:
+PASS: selection === expectedSelection
+Check that the selection range contains the text for the item plus the list marker (bullet):
+PASS: selection === expectedSelection
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items.html b/LayoutTests/accessibility/mac/text-marker-range-selection-in-list-items.html
@@ -0,0 +1,92 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/accessibility-helper.js"></script>
+<script src="../../resources/js-test.js"></script>
+<script src="./resources/accessibility-helper.js"></script>
+</head>
+<body>
+
+<ul id="list1">
+    <li id="item1">First item.</li>
+    <li id="item2">Second item.</li>
+    <li id="item3"><em>Third</em> item.</li>
+</ul>
+
+<script>
+let output = "Tests setting and retrieving text selection in list items using TextMarkerRanges.\n\n";
+const bullet = String.fromCharCode(0x2022);
+window.jsTestIsAsync = true;
+
+let list = null;
+let selectedRange = null;
+let selection = "";
+let expectedSelection = "";
+
+async function checkSelection() {
+    await waitFor(() => {
+        selectedRange = list.selectedTextMarkerRange();
+        selection = list.stringForTextMarkerRange(selectedRange);
+        return selection == expectedSelection;
+    });
+    output += expect("selection", "expectedSelection");
+}
+
+if (window.accessibilityController) {
+    // Select the second list item using DOM API.
+    selectTextInNode("item2");
+
+    setTimeout(async () => {
+        // Get the selection range via accessibility API and compare against expectedSelection.
+        list = accessibilityController.accessibleElementById("list1");
+
+        output += "Check that the selection range contains the text for the second item plus the list marker (bullet):\n";
+        expectedSelection = `${bullet} Second item.`;
+        await checkSelection();
+
+        // Set the selection to the first item via accessibility API.
+        let item = accessibilityController.accessibleElementById("item1");
+        let range = list.textMarkerRangeForElement(item);
+        list.setSelectedTextMarkerRange(range);
+
+        output += "Check that the selection range contains the first item text plus the list marker (bullet):\n";
+        expectedSelection = `${bullet} First item.`;
+        await checkSelection();
+
+        // Go down one level and select the range of the static text contained in the list item.
+        let text = item.childAtIndex(1);
+        // Note: childAtIndex(0) is the list marker. The list marker is an AX object backed by an anonymous RenderObject, i.e., it doesn't have an associated Node.
+        // Therefore, the list marker or bullet has no TextMarkerRange. The list marker bullet is included in the range of the static text.
+        range = text.textMarkerRangeForElement(text);
+        list.setSelectedTextMarkerRange(range);
+
+        output += "Check that the selection range contains the static text plus the list marker (bullet):\n";
+        expectedSelection = `${bullet} First item.`;
+        await checkSelection();
+
+        // Move the start of the range and select it. Note that both the first character of the text and the list marker are removed from the range text.
+        let start = list.startTextMarkerForTextMarkerRange(range);
+        start = list.nextTextMarker(start);
+        let end = list.endTextMarkerForTextMarkerRange(range);
+        range = list.textMarkerRangeForMarkers(start, end);
+        list.setSelectedTextMarkerRange(range);
+
+        output += "Check that the selection range doesn't include the first character of the static text nor the list marker after moving the start of the range:\n";
+        expectedSelection = `irst item.`;
+        await checkSelection();
+
+        // Select the third list item using DOM API.
+        selectTextInNode("item3");
+
+        output += "Check that the selection range contains the text for the item plus the list marker (bullet):\n";
+        expectedSelection = `${bullet} Third item.`;
+        await checkSelection();
+
+        debug(output);
+        document.getElementById("list1").style.visibility = "hidden";
+        finishJSTest();
+    }, 0);
+}
+</script>
+</body>
+</html>
diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -1677,36 +1677,23 @@ Vector<RetainPtr<id>> AccessibilityObject::modelElementChildren()
 static RenderListItem* renderListItemContainerForNode(Node* node)
 {
     for (; node; node = node->parentNode()) {
-        RenderBoxModelObject* renderer = node->renderBoxModelObject();
-        if (is<RenderListItem>(renderer))
-            return downcast<RenderListItem>(renderer);
+        if (auto* listItem = dynamicDowncast<RenderListItem>(node->renderBoxModelObject()))
+            return listItem;
     }
     return nullptr;
 }
 
-static StringView listMarkerTextForNode(Node* node)
-{
-    RenderListItem* listItem = renderListItemContainerForNode(node);
-    if (!listItem)
-        return { };
-
-    // If this is in a list item, we need to manually add the text for the list marker
-    // because a RenderListMarker does not have a Node equivalent and thus does not appear
-    // when iterating text.
-    return listItem->markerTextWithSuffix();
-}
-
-// Returns the text associated with a list marker if this node is contained within a list item.
+// Returns the text representing a list marker if node is contained within a list item.
 StringView AccessibilityObject::listMarkerTextForNodeAndPosition(Node* node, const VisiblePosition& visiblePositionStart)
 {
     auto* listItem = renderListItemContainerForNode(node);
     if (!listItem)
         return { };
+
     // Only include the list marker if the range includes the line start (where the marker would be), and is in the same line as the marker.
     if (!isStartOfLine(visiblePositionStart) || !inSameLine(visiblePositionStart, firstPositionInNode(&listItem->element())))
         return { };
-
-    return listMarkerTextForNode(node);
+    return listItem->markerTextWithSuffix();
 }
 
 String AccessibilityObject::stringForRange(const SimpleRange& range) const
@@ -1719,12 +1706,12 @@ String AccessibilityObject::stringForRange(const SimpleRange& range) const
     for (; !it.atEnd(); it.advance()) {
         // non-zero length means textual node, zero length means replaced node (AKA "attachments" in AX)
         if (it.text().length()) {
-            // Add a textual representation for list marker text.
+            // If this is in a list item, we need to add the text for the list marker
+            // because a RenderListMarker does not have a Node equivalent and thus does not appear
+            // when iterating text.
             // Don't add list marker text for new line character.
-            if (it.text().length() != 1 || !deprecatedIsSpaceOrNewline(it.text()[0])) {
-                // FIXME: Seems like the position should be based on it.range(), not range.
-                builder.append(listMarkerTextForNodeAndPosition(it.node(), VisiblePosition(makeDeprecatedLegacyPosition(range.start))));
-            }
+            if (it.text().length() != 1 || !deprecatedIsSpaceOrNewline(it.text()[0]))
+                builder.append(listMarkerTextForNodeAndPosition(it.node(), makeDeprecatedLegacyPosition(it.range().start)));
             it.appendTextToStringBuilder(builder);
         } else {
             if (replacedNodeNeedsCharacter(it.node()))