AX: Incorrect behavior for word related text marker functions when there's collapsed whitespace

[Nan Wang]

When there's collapsed whitespace, we are getting incorrect text marker ranges for words.

[Radar WebKit Bug Importer]

<rdar://problem/27414506>

[Nan Wang]

<rdar://problem/26898984>

[Nan Wang]

Created attachment 283964 [details]
Initial patch

[chris fleizach]

Comment on attachment 283964 [details]
Initial patch

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

> Source/WebCore/ChangeLog:8
> +        We are getting bad CharacterOffset when there's collapsed whitespace. Added a TraverseOptionValidateOffset

getting "a" bad

> Source/WebCore/ChangeLog:10
> +        fixed word navigation issue based on that.

fixed "a" word

> Source/WebCore/accessibility/AXObjectCache.cpp:1645
> +    // For replaced node without children, we should inluce itself in the range.

inluce -> include

> Source/WebCore/accessibility/AXObjectCache.cpp:1650
> +    if (ec)

it looks like the ec case is handled directly below

[Nan Wang]

Created attachment 284009 [details]
patch

update from review.

[Nan Wang]

Created attachment 284010 [details]
patch

fixed typo

[chris fleizach]

Comment on attachment 284009 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1590
> +        bool cumulateEnough = validateOffset ? cumulativeOffset + lastStartOffset >= offset : cumulativeOffset >= offset;

the name of this variable is strange: cumulateEnough
what purpose do you want it to serve?

it seems like it could be "bool validOffset"

> Source/WebCore/accessibility/AXObjectCache.cpp:2307
> +    // SimplifiedBackwardsTextIterator ignores replaced element

period after this sentence

[Nan Wang]

Comment on attachment 284009 [details]
patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:1590
>> +        bool cumulateEnough = validateOffset ? cumulativeOffset + lastStartOffset >= offset : cumulativeOffset >= offset;
> 
> the name of this variable is strange: cumulateEnough
> what purpose do you want it to serve?
> 
> it seems like it could be "bool validOffset"

This is to check if we have cumulated enough characters so that we can break early.

[Nan Wang]

Created attachment 284012 [details]
patch

more review comments

[chris fleizach]

Comment on attachment 284009 [details]
patch

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

>>> Source/WebCore/accessibility/AXObjectCache.cpp:1590
>>> +        bool cumulateEnough = validateOffset ? cumulativeOffset + lastStartOffset >= offset : cumulativeOffset >= offset;
>> 
>> the name of this variable is strange: cumulateEnough
>> what purpose do you want it to serve?
>> 
>> it seems like it could be "bool validOffset"
> 
> This is to check if we have cumulated enough characters so that we can break early.

bool offsetLimitReached

[Nan Wang]

Created attachment 284018 [details]
patch

Changed variable name.

[WebKit Commit Bot]

Comment on attachment 284018 [details]
patch

Clearing flags on attachment: 284018

Committed r203412: <http://trac.webkit.org/changeset/203412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.