[Webkit-unassigned] [Bug 119173] AccessibilityObject::stringForVisiblePositionRange() incorrectly repeats list marker text

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119173





--- Comment #2 from chris fleizach <cfleizach at apple.com>  2013-07-26 20:57:43 PST ---
(In reply to comment #0)
> Working on a downstream bug, I am seeing the list marker text inserted multiple times:
> 
> * At the start of the list item (as expected)
> * At the start (and sometimes also at the end) of a wrapped line (not expected)
> * Before/after embedded images, spans, etc (not expected)
> 
> Looking at AccessibilityObject::stringForVisiblePositionRange() [1], I noticed that for each TextIterator, we're calling AccessibilityObject::listMarkerTextForNodeAndPosition() with visiblePositionRange.start.
> 
> AccessibilityObject::listMarkerTextForNodeAndPosition() looks at that VisiblePosition, and only returns an empty String if that VisiblePosition is not the start of the line or there is no RenderListItem container for the node.
> 
> Why are we passing in the same VisiblePosition for each TextIterator in the VisiblePositionRange? That seems to be the culprit for the bug in question.
> 
> The following non-patch is just something I tried as part of debugging. It solves the problem. But without knowing why the original implementation was done as it was, I have no idea what this change might break. Chris, if you have some time to enlighten me, I'd be most grateful. Thanks in advance!
> 
> [1] http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityObject.cpp#L859
> [2] http://trac.webkit.org/browser/trunk/Source/WebCore/accessibility/AccessibilityObject.cpp#L843
> 
> ------------------------------------------------------------------------
> 
> diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
> index 9d564d0..162f24e 100644
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
> @@ -857,11 +857,12 @@ String AccessibilityObject::stringForVisiblePositionRange(const VisiblePositionR
> 
>      StringBuilder builder;
>      RefPtr<Range> range = makeRange(visiblePositionRange.start, visiblePositionRange.end);
> +    VisiblePosition position = visiblePositionRange.start;
>      for (TextIterator it(range.get()); !it.atEnd(); it.advance()) {
>          // non-zero length means textual node, zero length means replaced node (AKA "attachments" in AX)
>          if (it.length()) {
>              // Add a textual representation for list marker text
> -            String listMarkerText = listMarkerTextForNodeAndPosition(it.node(), visiblePositionRange.start);
> +            String listMarkerText = listMarkerTextForNodeAndPosition(it.node(), position);
>              if (!listMarkerText.isEmpty())
>                  builder.append(listMarkerText);
> 
> @@ -876,6 +877,7 @@ String AccessibilityObject::stringForVisiblePositionRange(const VisiblePositionR
>              if (replacedNodeNeedsCharacter(node->childNode(offset)))
>                  builder.append(objectReplacementCharacter);
>          }
> +        position = position.next();
>      }
> 
>      return builder.toString();

I think this bug has never come up so I don't think there's a good reason this is one way or the other. However, it seems suspect to assume position.next() will stay in sync with what the TextIterator is returning when advancing.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.