[Webkit-unassigned] [Bug 154098] AX: Implement paragraph related text marker functions using TextIterator

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Nan Wang <n_wang at apple.com> ---
Comment on attachment 271042
  --> https://bugs.webkit.org/attachment.cgi?id=271042
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160211/fcab7b03/attachment-0001.html>