Bug 118908 – [ATK] Issues with edge cases when getting offsets for a text range in AtkText

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

I noticed that there are some issues working with edge cases when working with implementations of AtkText through the atk_text_get_text_*_offset() functions (e.g. trying to get a word before/after the first/last position), that can be avoided if we change the way getSelectionOffsetsForObject() is implemented.

This patch will block bugs 114871, 114872 and 114873 since I will be using it as a base for those ones.

[Mario Sanchez Prada]

Created attachment 207114 [details]
Patch proposal

Here comes the patch

[Martin Robinson]

Comment on attachment 207114 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=207114&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:521
> +    // Default values, unless the contrary is proved.

This comment seems superfluous.

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:538
> +        startOffset = comparePositions(selection.start(), firstValidPosition) < 0 ? 0 : accessibilityObjectLength(coreObject);

So if the selection is after the node then you return accessibilityObjectLength as the start and end offsets?

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:560
> +    int rangeLength = TextIterator::rangeLength(nodeRange.get(), true);

Looks like you can avoid this temporary.

[Mario Sanchez Prada]

(In reply to comment #2)
> (From update of attachment 207114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207114&action=review
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:521
> > +    // Default values, unless the contrary is proved.
> 
> This comment seems superfluous.
> 
Ok

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:538
> > +        startOffset = comparePositions(selection.start(), firstValidPosition) < 0 ? 0 : accessibilityObjectLength(coreObject);
> 
> So if the selection is after the node then you return accessibilityObjectLength 
> as the start and end offsets?

Yes. If we reached that point it means that the selection falls *entirely* out of the range associated to the accessibility object, so we need to return a zero-length offsets range.

Now the reason why we return accesibilityObjectLength in that case is because in that situation the selection happens to be after the accessibility object, and so we must return a zero-length range coherent with that, since that's what other ATK implementations do.

Just think of it as normalizing the start and end offsets to the correct side of the text range associated to the object, so you either will have [0, 0] when the selection is before the object or [n, n] when it's after it.

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:560
> > +    int rangeLength = TextIterator::rangeLength(nodeRange.get(), true);
> 
> Looks like you can avoid this temporary.

Ok

[Mario Sanchez Prada]

(In reply to comment #3)
> [...]
> > > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceText.cpp:538
> > > +        startOffset = comparePositions(selection.start(), firstValidPosition) < 0 ? 0 : accessibilityObjectLength(coreObject);
> > 
> > So if the selection is after the node then you return 
> > accessibilityObjectLength 
> > as the start and end offsets?
> 
> Yes. If we reached that point it means that the selection falls *entirely* 
> out of the range associated to the accessibility object, so we need to return 
> a zero-length offsets range.
> 
> Now the reason why we return accesibilityObjectLength in that case is because 
> in that situation the selection happens to be after the accessibility object, 
> and so we must return a zero-length range coherent with that, since that's 
> what other ATK implementations do.
> 
> Just think of it as normalizing the start and end offsets to the correct side 
> of the text range associated to the object, so you either will have [0, 0] 
> when the selection is before the object or [n, n] when it's after it.
> 
Chris, could you take an additional quick look to this patch before landing it, just to make sure that the a11y specific bits make sense?

I forgot to mention it in my previous comment, but both Martin and me think that it would be very valuable if you could double check the patch before committing it.

Thanks!

[Mario Sanchez Prada]

> (In reply to comment #3)
> [...]
> Chris, could you take an additional quick look to this patch before landing it,
> just to make sure that the a11y specific bits make sense?
> 
> I forgot to mention it in my previous comment, but both Martin and me think
> that it would be very valuable if you could double check the patch before
> committing it.

I think I'm going to push it today in the end, so we can move forward asap with other patches that depend on this one too.

We will be able to see in the bots if it causes any regression and Chris will be always able to comment in the future if needed, so we should be safe anyway without having to request right now for time from more people.

In any case, I will wait a few hours from now just to give some additional margin for last-minute comments :)

[chris fleizach]

Sorry for the delay. This patch appears to be ok from an accessibility standpoint

(In reply to comment #5)
> > (In reply to comment #3)
> > [...]
> > Chris, could you take an additional quick look to this patch before landing it,
> > just to make sure that the a11y specific bits make sense?
> > 
> > I forgot to mention it in my previous comment, but both Martin and me think
> > that it would be very valuable if you could double check the patch before
> > committing it.
> 
> I think I'm going to push it today in the end, so we can move forward asap with other patches that depend on this one too.
> 
> We will be able to see in the bots if it causes any regression and Chris will be always able to comment in the future if needed, so we should be safe anyway without having to request right now for time from more people.
> 
> In any case, I will wait a few hours from now just to give some additional margin for last-minute comments :)

[Mario Sanchez Prada]

(In reply to comment #6)
> Sorry for the delay. This patch appears to be ok from an accessibility 
> standpoint

No problem. Thanks for reviewing it, Chris. It was just in time(tm)!

[Mario Sanchez Prada]

Committed r153427: <http://trac.webkit.org/changeset/153427>