[Webkit-unassigned] [Bug 152728] AX:Implement next/previous text marker functions using TextIterator

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 268345
  --> https://bugs.webkit.org/attachment.cgi?id=268345
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1445
> +            // Here when we don't have any character to move and we are going backwards, we traverse to previous node.

traverse to "the" previous

> Source/WebCore/accessibility/AXObjectCache.cpp:1462
> +            int suboffset = iterator.range()->startOffset();

suboffset -> subOffset

> Source/WebCore/accessibility/AXObjectCache.cpp:1470
> +            if (currentLength == 1 && (iterator.text()[0] == ' ' || iterator.text()[0] == '\n' || iterator.text()[0] == '\t'))

did this line come from existing code? My guess is that there's some whitespace detection mechanism that will account for other weird things that you don't have here like \r

> Source/WebCore/accessibility/AXObjectCache.cpp:1484
> +            offsetInCharacter =  offset - (offsetSoFar - currentLength);

bad spacing after =

> Source/WebCore/accessibility/AXObjectCache.cpp:1513
> +void AXObjectCache::startOrEndTextMarkerDataforRange(TextMarkerData& textMarkerData, RefPtr<Range> range, bool isStart)

Datafor -> DataFor

> Source/WebCore/accessibility/AXObjectCache.cpp:1528
> +    if (is<HTMLInputElement>(*domNode) && downcast<HTMLInputElement>(*domNode).isPasswordField())

we should add a layout test variation for the password field case

> Source/WebCore/accessibility/AXObjectCache.cpp:1531
> +    AXObjectCache* cache = domNode->document().axObjectCache();

do we think that this domNode will have a different AXObjectCache then this axObjectCache that we're in right now?

> Source/WebCore/accessibility/AXObjectCache.cpp:1583
> +    domNode = charOffset.node;

this bit of code is pretty identical to the method above this one. Can we make a helper routine to consolidate the code?

> Source/WebCore/accessibility/AXObjectCache.cpp:1669
> +    AXObjectCache* cache = domNode->document().axObjectCache();

seems like we should be using this AXObjectCache instead of asking for the cache through the document

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:503
> +    if (!textMarker)

we should check for !cache case too just to be safe

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3866
> +        return [self textMarkerForNode:*textMarkerData.node Offset:textMarkerData.characterOffset + 1];

Offset should be lowercase

> LayoutTests/accessibility/mac/previous-next-text-marker.html:96
> +        shouldBeTrue("text3.accessibilityElementForTextMarker(currentMarker).isEqual(text3)");

let's add a check here that does

1. iterate into the user-select: none text
2. we get two markers that represent positions within user-select:none (like from "s" -> "l" or something)
3. check the string that we get back from this range and verify text is correct
4. then let's iterate backwards from that 2nd node and get another range where we verify the string

thanks

-- 
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/20160106/fe0f5254/attachment-0001.html>