Bug 154098 – AX: Implement paragraph related text marker functions using TextIterator

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

[Nan Wang]

Improve accessibility text navigation.
Change the paragraph related text marker calls to use TextIterator.

[Nan Wang]

<rdar://problem/24269675>

[Nan Wang]

Created attachment 271040 [details]
patch

[Nan Wang]

Created attachment 271042 [details]
patch

Implemented the FIXME issue from the last word related patch.

[chris fleizach]

Comment on attachment 271042 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1882
> +    // Sometimes a text node's character count doesn't include characters before a line break,

do you have a test for this case?

> Source/WebCore/accessibility/AXObjectCache.cpp:2177
> +        return characterOffsetForNodeAndOffset(*node, offset, false, false);

we should add enums for these two bool parameters so its clear in the code why you're passing what you're passing

> Source/WebCore/accessibility/AXObjectCache.cpp:2201
> +    return endCharacterOffsetOfParagraph(next);

can nextCharacterOffset and endCharacterOffsetOfParagraph hand the isNull() case so that this method can be collapsed to

return endCharacterOffsetOfParagraph(nextCharacterOffset(characterOffset))

> Source/WebCore/accessibility/AXObjectCache.cpp:2214
> +    return startCharacterOffsetOfParagraph(previous);

ditto

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2434
> +        textMarker = [self nextMarkerForCharacterOffset:next];

i feel like this recursive formulation wastes cycles, because at the minimum it has to pull the axObjectCache() each time

can we write this like

while (textMarker && textMarker.isIgnored)
   characterOffset = cache->nextCharacterOffset(characterOffset)
   textMarker = [self nextMarkerForCharacterOffset: characterOffset];

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2446
> +        textMarker = [self previousMarkerForCharacterOffset:previous];

ditto

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:851
> +        textMarker = nextTextMarkerForCharacterOffset(cache, next);

looks like we have basically the same code in iOS and Mac. Can you put this in the Base object

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:863
> +        textMarker = previousTextMarkerForCharacterOffset(cache, previous);

ditto

> LayoutTests/accessibility/mac/text-marker-paragraph-nav.html:12
> +<div id="text1" tabindex="0">

can you also add a test that does something like

<div>this is my first paragraph. Of text. it has some text.<br><br>
this is my second paragraph. Of text. it has some text.<br>
this is my second paragraph. Of text. it has some text.<br><br>
</div>

and verify you get the right start for each one

[Nan Wang]

Comment on attachment 271042 [details]
patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:1882
>> +    // Sometimes a text node's character count doesn't include characters before a line break,
> 
> do you have a test for this case?

Yes, it's covered in the test.

>> Source/WebCore/accessibility/AXObjectCache.cpp:2177
>> +        return characterOffsetForNodeAndOffset(*node, offset, false, false);
> 
> we should add enums for these two bool parameters so its clear in the code why you're passing what you're passing

Ok

>> Source/WebCore/accessibility/AXObjectCache.cpp:2201
>> +    return endCharacterOffsetOfParagraph(next);
> 
> can nextCharacterOffset and endCharacterOffsetOfParagraph hand the isNull() case so that this method can be collapsed to
> 
> return endCharacterOffsetOfParagraph(nextCharacterOffset(characterOffset))

Ok

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2434
>> +        textMarker = [self nextMarkerForCharacterOffset:next];
> 
> i feel like this recursive formulation wastes cycles, because at the minimum it has to pull the axObjectCache() each time
> 
> can we write this like
> 
> while (textMarker && textMarker.isIgnored)
>    characterOffset = cache->nextCharacterOffset(characterOffset)
>    textMarker = [self nextMarkerForCharacterOffset: characterOffset];

Ok

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:851
>> +        textMarker = nextTextMarkerForCharacterOffset(cache, next);
> 
> looks like we have basically the same code in iOS and Mac. Can you put this in the Base object

The problem is that iOS and Mac don't use the same TextMarker wrapper, I'll try to figure something out.

>> LayoutTests/accessibility/mac/text-marker-paragraph-nav.html:12
>> +<div id="text1" tabindex="0">
> 
> can you also add a test that does something like
> 
> <div>this is my first paragraph. Of text. it has some text.<br><br>
> this is my second paragraph. Of text. it has some text.<br>
> this is my second paragraph. Of text. it has some text.<br><br>
> </div>
> 
> and verify you get the right start for each one

Ok

[Nan Wang]

Created attachment 271236 [details]
patch

- review comments.
- found and fixed an issue where we could't walk through a text node by character if there's line breaks in it.

[WebKit Commit Bot]

Comment on attachment 271236 [details]
patch

Clearing flags on attachment: 271236

Committed r196546: <http://trac.webkit.org/changeset/196546>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.