Bug 152728 – AX: [Mac] Implement next/previous text marker functions using TextIterator

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

[Nan Wang]

Try to use TextIterator class to refactor the next/previous text marker functions, so we might be able to avoid some VisiblePosition bugs.

[Radar WebKit Bug Importer]

<rdar://problem/24052432>

[Nan Wang]

Created attachment 268269 [details]
patch

[chris fleizach]

Comment on attachment 268269 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        Implemented a way to navigate to previous/next text marker using Range and TextIterator.

can you add a few sentences as to why you're doing this

> Source/WebCore/accessibility/AXObjectCache.cpp:1431
> +    Node* currNode = nullptr;

currNode -> currentNode

> Source/WebCore/accessibility/AXObjectCache.cpp:1448
> +                if (preNode)

if (Node* preNode = previousNode(currNode))

> Source/WebCore/accessibility/AXObjectCache.cpp:1457
> +        int currLen = iterator.text().length();

currLen = currentLength

> Source/WebCore/accessibility/AXObjectCache.cpp:1469
> +            

extra newline can be removed

> Source/WebCore/accessibility/AXObjectCache.cpp:1509
> +    RefPtr<Range> range =  Range::create(*document);

too many spaces 

"= Range"

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

return ec ? nullptr : range;

> Source/WebCore/accessibility/AXObjectCache.cpp:1626
> +    if (node->parentNode() && node->parentNode()->renderer() &&  node->parentNode()->renderer()->isBody() && !node->previousSibling())

too many spaces between the &&
"node->parentNode()->renderer() &&  node->parentNode()->renderer()->isBody()"

> Source/WebCore/accessibility/AXObjectCache.h:65
> +    CharacterOffset(Node* n, int offset, int remaining)

you might be able to put default values for these parameters and then remove the default constructor
CharacterOffset() { }

--> 
CharacterOffset(Node* n = nullptr, int offset = 0, int remaining = 0)

> Source/WebCore/accessibility/AccessibilityObject.cpp:724
> +    return AXObjectCache::rangeForNodeContents(node);

seems like you could probably just do

return AXObjectCache::rangeForNodeContents(node());

and then handle the nil case inside rangeForNodeContents

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:790
> +static AccessibilityObject* accessibilityObjectForTextMarker(AXObjectCache* cache, CFTypeRef textMarker)

these methods might be better off inside WebAXObjectWrapperBase so that they can be used by iOS and OSX

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:830
> +- (id)textMarkerForNode:(Node&)node Offset:(int)offset

Offset -> offset

> LayoutTests/ChangeLog:7
> +

we should add a test for navigation within a user-select:none area

[Nan Wang]

Created attachment 268345 [details]
patch

review comments

[Nan Wang]

Comment on attachment 268345 [details]
patch

Working on fixing the iOS build issue and adding support for iOS

[chris fleizach]

Comment on attachment 268345 [details]
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

[Nan Wang]

Created attachment 268903 [details]
patch

- Changed a lot of code to implement stringForTextMarkerRange call
- Fixed an issue where text node's start offset is not always 0

Since the patch is already very large, I'll put the iOS implementation in another bug.

[Nan Wang]

Created attachment 268922 [details]
patch

Fixed some issue in rangeForUnorderedCharacterOffsets.

[chris fleizach]

Comment on attachment 268922 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1457
> +    for (; !iterator.atEnd(); iterator.advance()) {

seems like int count = 1 and count++ can be part of the for loop

> Source/WebCore/accessibility/AXObjectCache.cpp:1466
> +            if (AccessibilityObject::replacedNodeNeedsCharacter(node.traverseToChildAt(subOffset))) {

you can probably cache node.traverseToChildAt(subOffset) in a local variable so you don't have to call it twice

> Source/WebCore/accessibility/AXObjectCache.cpp:1479
> +        if (!currentLength)

this check could be put up as the else for the if block at 1466 right?

> Source/WebCore/accessibility/AXObjectCache.cpp:1562
> +    // endOffset can be out of bound sometimes if the node is a replaced node or has brTag.

out of "bounds"

> Source/WebCore/accessibility/AXObjectCache.cpp:1565
> +        if (endOffset > TextIterator::rangeLength(nodeRange.get()))

you can store TextIterator::rangeLength(nodeRange.get()) so you don't have to call it twice

> Source/WebCore/accessibility/AXObjectCache.h:55
> +    int characterOffset;

what is startIndex and what is ignored used for?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:805
> +        return nil;

nullptr since it's a C++ object

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:847
> +    if (isTextMarkerIgnored(textMarker))

doesn't this need to be in a while loop

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:856
> +    if (isTextMarkerIgnored(textMarker))

doesn't this need to be in a while loop

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877
> +    if (!textMarkerData.axID && !textMarkerData.ignored)

can we use axID = 0 to mean ignored?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:886
> +        return nil;

nullptr since its C++

> LayoutTests/accessibility/mac/previous-next-text-marker.html:117
> +        shouldBeTrue("!psw.accessibilityElementForTextMarker(start)");

shouldBeFalse

[Nan Wang]

Comment on attachment 268922 [details]
patch

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

I'll address other comments.

>> Source/WebCore/accessibility/AXObjectCache.h:55
>> +    int characterOffset;
> 
> what is startIndex and what is ignored used for?

In the range of a text node's content, the start offset is not always 0. So I used (startIndex + character count) to set the correct start offset when creating a range from text marker range.
Ignored is used here for password field, so we can skip that when calling next/previous text marker call.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:847
>> +    if (isTextMarkerIgnored(textMarker))
> 
> doesn't this need to be in a while loop

I'm calling nextTextMarkerForNode:, not textMarkerForNode:. So it's recursive.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:856
>> +    if (isTextMarkerIgnored(textMarker))
> 
> doesn't this need to be in a while loop

ditto.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877
>> +    if (!textMarkerData.axID && !textMarkerData.ignored)
> 
> can we use axID = 0 to mean ignored?

We also return an empty TextMarkerData when node is nullptr. In that case we want to return nil for the text marker. For example we get to the end of document that we can't ignore it and keep going forward. But here for password filed, we want to treat it differently, so that we know the node exists and there might be nodes after it.

[Nan Wang]

Comment on attachment 268922 [details]
patch

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

>> LayoutTests/accessibility/mac/previous-next-text-marker.html:117
>> +        shouldBeTrue("!psw.accessibilityElementForTextMarker(start)");
> 
> shouldBeFalse

I tried to use shouldBeNull, but DumpRenderTree is returning a undefined object which will cause the check fail. So I kept it this way.

[chris fleizach]

(In reply to comment #11)
> Comment on attachment 268922 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268922&action=review
> 
> >> LayoutTests/accessibility/mac/previous-next-text-marker.html:117
> >> +        shouldBeTrue("!psw.accessibilityElementForTextMarker(start)");
> > 
> > shouldBeFalse
> 
> I tried to use shouldBeNull, but DumpRenderTree is returning a undefined
> object which will cause the check fail. So I kept it this way.

ok no worries

[Nan Wang]

Created attachment 269022 [details]
patch

Update from review comments.

[chris fleizach]

Comment on attachment 269022 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877
> +    if (!textMarkerData.axID && !textMarkerData.ignored)

should this be if (!textMarkerData.axID || !textMarkerData.ignored)

[chris fleizach]

i also wrote a test for this a million years ago in
https://bugs.webkit.org/attachment.cgi?id=209142&action=review

can you include that test in this patch as well for more coverage?

[Nan Wang]

(In reply to comment #14)
> Comment on attachment 269022 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269022&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:877
> > +    if (!textMarkerData.axID && !textMarkerData.ignored)
> 
> should this be if (!textMarkerData.axID || !textMarkerData.ignored)

For the ignored element, I only set the ignored property, its axID is still 0. So || will return a nil which is not what we want. For a zeroed TextMarketData, the ignored property is false, so the empty data is not ignored by default.

[Nan Wang]

(In reply to comment #15)
> i also wrote a test for this a million years ago in
> https://bugs.webkit.org/attachment.cgi?id=209142&action=review
> 
> can you include that test in this patch as well for more coverage?

Ok, will do.

[Nan Wang]

Created attachment 269234 [details]
patch

Added text-marker-with-user-select-none.html test.
Since the indexForTextMarker function is using VisiblePosition code and it's not working fine with user-select:none, I removed that portion from the test.

[WebKit Commit Bot]

Comment on attachment 269234 [details]
patch

Clearing flags on attachment: 269234

Committed r195240: <http://trac.webkit.org/changeset/195240>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.