Bug 238341 – AccessibilityObject::listMarkerTextForNodeAndPosition should check for presence of list item before anything else

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

This function is currently:

String AccessibilityObject::listMarkerTextForNodeAndPosition(Node* node, const VisiblePosition& visiblePositionStart)
{
    // If the range does not contain the start of the line, the list marker text should not be included.
    if (!isStartOfLine(visiblePositionStart))
        return String();

    // We should speak the list marker only for the first line.
    RenderListItem* listItem = renderListItemContainerForNode(node);
    if (!listItem)
        return String();
    if (!inSameLine(visiblePositionStart, firstPositionInNode(&listItem->element())))
        return String();
    
    return listMarkerTextForNode(node);
}

The !isStartOfLine(visiblePositionStart) check is redundant, covered entirely by the later !inSameLine(visiblePositionStart, firstPositionInNode(&listItem->element())) check.

[Radar WebKit Bug Importer]

<rdar://problem/90781742>

[Tyler Wilcock]

Created attachment 455660 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 455660 [details]
> Patch

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        In AccessibilityObject::listMarkerTextForNodeAndPosition, the !isStartOfLine(visiblePositionStart)
+        check is redundant, covered entirely by the later
+        !inSameLine(visiblePositionStart,
+        firstPositionInNode(&listItem->element())) check. This patch removes
+        the former check, since it can sometimes cause crashes.

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -1455,17 +1455,13 @@ static String listMarkerTextForNode(Node* node)
 // Returns the text associated with a list marker if this node is contained within a list item.
 String AccessibilityObject::listMarkerTextForNodeAndPosition(Node* node, const VisiblePosition& visiblePositionStart)
 {
-    // If the range does not contain the start of the line, the list marker text should not be included.
-    if (!isStartOfLine(visiblePositionStart))
-        return String();

I don't see how the inSameLine check includes the above check inSameLine checks whether the two positions have the same startOfLine, but the above check is whether the given position is exactly startOfLine.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 455660 [details]
> > Patch
> 
> --- a/Source/WebCore/ChangeLog
> +++ a/Source/WebCore/ChangeLog
> 
> +        In AccessibilityObject::listMarkerTextForNodeAndPosition, the
> !isStartOfLine(visiblePositionStart)
> +        check is redundant, covered entirely by the later
> +        !inSameLine(visiblePositionStart,
> +        firstPositionInNode(&listItem->element())) check. This patch removes
> +        the former check, since it can sometimes cause crashes.
> 
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> @@ -1455,17 +1455,13 @@ static String listMarkerTextForNode(Node* node)
>  // Returns the text associated with a list marker if this node is contained
> within a list item.
>  String AccessibilityObject::listMarkerTextForNodeAndPosition(Node* node,
> const VisiblePosition& visiblePositionStart)
>  {
> -    // If the range does not contain the start of the line, the list marker
> text should not be included.
> -    if (!isStartOfLine(visiblePositionStart))
> -        return String();
> 
> I don't see how the inSameLine check includes the above check inSameLine
> checks whether the two positions have the same startOfLine, but the above
> check is whether the given position is exactly startOfLine.

Perhaps we can try:

    auto* listItem = renderListItemContainerForNode(node);
    if (!listItem)
        return { };
    if (!isStartOfLine(visiblePositionStart) || !inSameLine(visiblePositionStart, firstPositionInNode(&listItem->element())))
        return { };

[Tyler Wilcock]

Created attachment 455764 [details]
Patch

[EWS]

Committed r291968 (248933@main): <https://commits.webkit.org/248933@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455764 [details].