AX: CharacterOffset not working correctly with composed characters and collapsed...
authorn_wang@apple.com <n_wang@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Apr 2016 20:05:07 +0000 (20:05 +0000)
committern_wang@apple.com <n_wang@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Apr 2016 20:05:07 +0000 (20:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157190

Reviewed by Chris Fleizach.

Source/WebCore:

When navigating emoji, next/previous text marker call is only moving by one character. Fixed it by
using the helper function in Position to get the real character count for the composed character sequence.
Also there's another issue with collapsed white spaces, TextIterator emits only one space. So we have to
use the actual space length to create the CharacterOffset in order to generate valid Range object from it.

New test cases in accessibility/text-marker/text-marker-previous-next.html.

* accessibility/AXObjectCache.cpp:
(WebCore::AXObjectCache::traverseToOffsetInRange):
(WebCore::AXObjectCache::textMarkerDataForNextCharacterOffset):
(WebCore::AXObjectCache::textMarkerDataForPreviousCharacterOffset):
(WebCore::AXObjectCache::nextNode):
(WebCore::AXObjectCache::characterOffsetFromVisiblePosition):
(WebCore::AXObjectCache::nextCharacterOffset):
(WebCore::AXObjectCache::previousCharacterOffset):
(WebCore::AXObjectCache::startCharacterOffsetOfWord):

LayoutTests:

* accessibility/mac/text-marker-word-nav.html:
* accessibility/text-marker/text-marker-previous-next-expected.txt:
* accessibility/text-marker/text-marker-previous-next.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@200258 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/accessibility/mac/text-marker-word-nav.html
LayoutTests/accessibility/text-marker/text-marker-previous-next-expected.txt
LayoutTests/accessibility/text-marker/text-marker-previous-next.html
Source/WebCore/ChangeLog
Source/WebCore/accessibility/AXObjectCache.cpp

index 61f4d1a..024fba7 100644 (file)
@@ -1,3 +1,14 @@
+2016-04-29  Nan Wang  <n_wang@apple.com>
+
+        AX: CharacterOffset not working correctly with composed characters and collapsed white spaces
+        https://bugs.webkit.org/show_bug.cgi?id=157190
+
+        Reviewed by Chris Fleizach.
+
+        * accessibility/mac/text-marker-word-nav.html:
+        * accessibility/text-marker/text-marker-previous-next-expected.txt:
+        * accessibility/text-marker/text-marker-previous-next.html:
+
 2016-04-29  Ryan Haddad  <ryanhaddad@apple.com>
 
         Marking fast/ruby/ruby-expansion-cjk.html and fast/ruby/ruby-expansion-cjk-4.html as flaky on Mac
index 8bceb8c..6c5354a 100644 (file)
@@ -67,7 +67,7 @@ text
         
         // Check the case with span
         // At "T" in "Thisis", should return the word as "Thisislongword".
-        currentMarker = advanceAndVerify(currentMarker, 2, text);
+        currentMarker = advanceAndVerify(currentMarker, 1, text);
         // At " " before "I", the word should be "I'll".
         currentMarker = advanceAndVerify(currentMarker, 14, text);
         // At " " before "try", the word should excludes "."
index 38151cf..894555e 100644 (file)
@@ -5,6 +5,10 @@ c  d
 
 can't select
 abc de f
+😃😏
+
+a b
+
 This tests the next/previous text marker functions are implemented correctly.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -28,6 +32,15 @@ PASS text2.accessibilityElementForTextMarker(currentMarker).isEqual(text3) is tr
 PASS text2.accessibilityElementForTextMarker(currentMarker).isEqual(text2.childAtIndex(2)) is true
 PASS text.stringForTextMarkerRange(markerRange) is 'f'
 PASS text.stringForTextMarkerRange(markerRange) is 'a'
+PASS text.textMarkerRangeLength(emojiTextMarkerRange) is 4
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is '😏'
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is '😃'
+PASS text.textMarkerRangeLength(collapsedWhitespaceMarkerRange) is 3
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is 'a'
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is ' '
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is 'b'
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is ' '
+PASS text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current)) is 'a'
 PASS successfullyParsed is true
 
 TEST COMPLETE
index 33d2201..f27808b 100644 (file)
@@ -2,6 +2,7 @@
 <html>
 <head>
 <script src="../../resources/js-test-pre.js"></script>
+<meta charset="UTF-16">
 </head>
 <style>
 .userselect { user-select: none; -webkit-user-select: none; }
@@ -26,6 +27,10 @@ de
 f
 </div>
 
+<p id="text5">😃😏<p>
+
+<p id="text6">a     b</p>
+
 <p id="description"></p>
 <div id="console"></div>
 
@@ -71,8 +76,8 @@ f
         markerRange = text.textMarkerRangeForMarkers(previousMarker, currentMarker);
         shouldBe("text.stringForTextMarkerRange(markerRange)", "newline");
         
-        // Traverse backwards two more character, it will land at the last character of "text".
-        result = backwards(2, previousMarker, currentMarker, text);
+        // Traverse backwards one more character, it will land at the last character of "text".
+        result = backwards(1, previousMarker, currentMarker, text);
         previousMarker = result.previous;
         currentMarker = result.current;
         markerRange = text.textMarkerRangeForMarkers(previousMarker, currentMarker);
@@ -93,8 +98,8 @@ f
         markerRange = text2.textMarkerRangeForMarkers(previousMarker, currentMarker);
         shouldBe("text2.stringForTextMarkerRange(markerRange)", "'d'");
         
-        // Traverse backwards 8 characters, it will land at the last character of "text1".
-        result = backwards(8, previousMarker, currentMarker, text2);
+        // Traverse backwards 6 characters, it will land at the last character of "text1".
+        result = backwards(6, previousMarker, currentMarker, text2);
         previousMarker = result.previous;
         currentMarker = result.current;
         markerRange = text2.textMarkerRangeForMarkers(previousMarker, currentMarker);
@@ -163,6 +168,33 @@ f
         markerRange = text.textMarkerRangeForMarkers(startMarker, currentMarker)
         shouldBe("text.stringForTextMarkerRange(markerRange)", "'a'");
         
+        // Test case with emoji.
+        text = accessibilityController.accessibleElementById("text5");
+        var emojiTextMarkerRange = text.textMarkerRangeForElement(text);
+        shouldBe("text.textMarkerRangeLength(emojiTextMarkerRange)", "4");
+        // Make sure navigating next/previous text marker is by emoji.
+        startMarker = text.startTextMarkerForTextMarkerRange(emojiTextMarkerRange);
+        result = forward(2, previousMarker, startMarker, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "'😏'");
+        result = backwards(1, result.previous, result.current, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "'😃'");
+        
+        // Test case with collapsed whitespace.
+        text = accessibilityController.accessibleElementById("text6");
+        var collapsedWhitespaceMarkerRange = text.textMarkerRangeForElement(text);
+        shouldBe("text.textMarkerRangeLength(collapsedWhitespaceMarkerRange)", "3");
+        startMarker = text.startTextMarkerForTextMarkerRange(collapsedWhitespaceMarkerRange);
+        result = forward(1, previousMarker, startMarker, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "'a'");
+        result = forward(1, result.previous, result.current, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "' '");
+        result = forward(1, result.previous, result.current, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "'b'");
+        result = backwards(1, result.previous, result.current, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "' '");
+        result = backwards(1, result.previous, result.current, text);
+        shouldBe("text.stringForTextMarkerRange(text.textMarkerRangeForMarkers(result.previous, result.current))", "'a'");
+        
         function forward(count, previousMarker, currentMarker, obj) {
             for (var i = 0; i < count; i++) {
                 previousMarker = currentMarker;
index b30729f..1586d66 100644 (file)
@@ -1,3 +1,27 @@
+2016-04-29  Nan Wang  <n_wang@apple.com>
+
+        AX: CharacterOffset not working correctly with composed characters and collapsed white spaces
+        https://bugs.webkit.org/show_bug.cgi?id=157190
+
+        Reviewed by Chris Fleizach.
+
+        When navigating emoji, next/previous text marker call is only moving by one character. Fixed it by
+        using the helper function in Position to get the real character count for the composed character sequence.
+        Also there's another issue with collapsed white spaces, TextIterator emits only one space. So we have to 
+        use the actual space length to create the CharacterOffset in order to generate valid Range object from it.
+
+        New test cases in accessibility/text-marker/text-marker-previous-next.html.
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::traverseToOffsetInRange):
+        (WebCore::AXObjectCache::textMarkerDataForNextCharacterOffset):
+        (WebCore::AXObjectCache::textMarkerDataForPreviousCharacterOffset):
+        (WebCore::AXObjectCache::nextNode):
+        (WebCore::AXObjectCache::characterOffsetFromVisiblePosition):
+        (WebCore::AXObjectCache::nextCharacterOffset):
+        (WebCore::AXObjectCache::previousCharacterOffset):
+        (WebCore::AXObjectCache::startCharacterOffsetOfWord):
+
 2016-04-28  Jer Noble  <jer.noble@apple.com>
 
         WebPlaybackControlsManager should not be owned by the WebPlaybackSessionInterfaceMac.
index 595a387..7a8db34 100644 (file)
@@ -1479,7 +1479,7 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
     bool toNodeEnd = option & TraverseOptionToNodeEnd;
     
     int offsetInCharacter = 0;
-    int offsetSoFar = 0;
+    int cumulativeOffset = 0;
     int remaining = 0;
     int lastLength = 0;
     Node* currentNode = nullptr;
@@ -1494,8 +1494,8 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
         lastStartOffset = range->startOffset();
         if (offset > 0 || toNodeEnd) {
             if (AccessibilityObject::replacedNodeNeedsCharacter(currentNode) || (currentNode->renderer() && currentNode->renderer()->isBR()))
-                offsetSoFar++;
-            lastLength = offsetSoFar;
+                cumulativeOffset++;
+            lastLength = cumulativeOffset;
             
             // When going backwards, stayWithinRange is false.
             // Here when we don't have any character to move and we are going backwards, we traverse to the previous node.
@@ -1510,19 +1510,27 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
     // Sometimes text contents in a node are splitted into several iterations, so that iterator.range()->startOffset()
     // might not be the correct character count. Here we use a previousNode object to keep track of that.
     Node* previousNode = nullptr;
+    // When text node has collapsed whitespaces, we need to treat it differently since text iterator
+    // will omit the collapsed spaces and make the offset inaccurate.
+    Node* collapsedWhitespaceNode = nullptr;
     for (; !iterator.atEnd(); iterator.advance()) {
         int currentLength = iterator.text().length();
         bool hasReplacedNodeOrBR = false;
         
         Node& node = iterator.range()->startContainer();
         currentNode = &node;
+        
+        // The offset of node with collapsed whitespaces has been calcualted in the first iteration.
+        if (currentNode == collapsedWhitespaceNode)
+            continue;
+        
         // When currentLength == 0, we check if there's any replaced node.
         // If not, we skip the node with no length.
         if (!currentLength) {
             int subOffset = iterator.range()->startOffset();
             Node* childNode = node.traverseToChildAt(subOffset);
             if (AccessibilityObject::replacedNodeNeedsCharacter(childNode)) {
-                offsetSoFar++;
+                cumulativeOffset++;
                 currentLength++;
                 currentNode = childNode;
                 hasReplacedNodeOrBR = true;
@@ -1545,8 +1553,30 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
                         continue;
                     }
                 }
+            } else if (currentNode->isTextNode() && currentNode->renderer()) {
+                // When there's collapsed whitespace, the text iterator will only count those spaces as one single space.
+                // Here we use the RenderText to get the actual length.
+                RenderText* renderedText = downcast<RenderText>(currentNode->renderer());
+                int currentStartOffset = iterator.range()->startOffset();
+                if (renderedText->style().isCollapsibleWhiteSpace(iterator.text()[currentLength - 1])  && currentLength + currentStartOffset != renderedText->caretMaxOffset()) {
+                    int appendLength = (&range->endContainer() == currentNode ? range->endOffset() : (int)renderedText->text()->length()) - currentStartOffset;
+                    lastStartOffset = currentStartOffset;
+                    cumulativeOffset += appendLength;
+                    lastLength = appendLength;
+                    
+                    // Break early if we have advanced enough characters.
+                    if (!toNodeEnd && cumulativeOffset >= offset) {
+                        offsetInCharacter = offset - (cumulativeOffset - lastLength);
+                        finished = true;
+                        break;
+                    }
+                    
+                    collapsedWhitespaceNode = currentNode;
+                    continue;
+                }
+                
             }
-            offsetSoFar += currentLength;
+            cumulativeOffset += currentLength;
         }
 
         if (currentNode == previousNode)
@@ -1557,8 +1587,8 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
         }
         
         // Break early if we have advanced enough characters.
-        if (!toNodeEnd && offsetSoFar >= offset) {
-            offsetInCharacter = offset - (offsetSoFar - lastLength);
+        if (!toNodeEnd && cumulativeOffset >= offset) {
+            offsetInCharacter = offset - (cumulativeOffset - lastLength);
             finished = true;
             break;
         }
@@ -1568,7 +1598,7 @@ CharacterOffset AXObjectCache::traverseToOffsetInRange(RefPtr<Range>range, int o
     if (!finished) {
         offsetInCharacter = lastLength;
         if (!toNodeEnd)
-            remaining = offset - offsetSoFar;
+            remaining = offset - cumulativeOffset;
     }
     
     return CharacterOffset(currentNode, lastStartOffset, offsetInCharacter, remaining);
@@ -1850,22 +1880,36 @@ void AXObjectCache::textMarkerDataForNextCharacterOffset(TextMarkerData& textMar
 {
     CharacterOffset next = characterOffset;
     CharacterOffset previous = characterOffset;
+    bool shouldContinue;
     do {
+        shouldContinue = false;
         next = nextCharacterOffset(next, false);
         if (shouldSkipBoundary(previous, next))
             next = nextCharacterOffset(next, false);
         textMarkerDataForCharacterOffset(textMarkerData, next);
+        
+        // We should skip next CharactetOffset if it's visually the same.
+        if (!lengthForRange(rangeForUnorderedCharacterOffsets(previous, next).get()))
+            shouldContinue = true;
         previous = next;
-    } while (textMarkerData.ignored);
+    } while (textMarkerData.ignored || shouldContinue);
 }
 
 void AXObjectCache::textMarkerDataForPreviousCharacterOffset(TextMarkerData& textMarkerData, const CharacterOffset& characterOffset)
 {
     CharacterOffset previous = characterOffset;
+    CharacterOffset next = characterOffset;
+    bool shouldContinue;
     do {
+        shouldContinue = false;
         previous = previousCharacterOffset(previous, false);
         textMarkerDataForCharacterOffset(textMarkerData, previous);
-    } while (textMarkerData.ignored);
+        
+        // We should skip previous CharactetOffset if it's visually the same.
+        if (!lengthForRange(rangeForUnorderedCharacterOffsets(previous, next).get()))
+            shouldContinue = true;
+        next = previous;
+    } while (textMarkerData.ignored || shouldContinue);
 }
 
 Node* AXObjectCache::nextNode(Node* node) const
@@ -1924,27 +1968,32 @@ CharacterOffset AXObjectCache::characterOffsetFromVisiblePosition(const VisibleP
         return CharacterOffset();
     
     // Use nextVisiblePosition to calculate how many characters we need to traverse to the current position.
-    VisiblePositionRange vpRange = obj->visiblePositionRange();
-    VisiblePosition vp = vpRange.start;
+    VisiblePositionRange visiblePositionRange = obj->visiblePositionRange();
+    VisiblePosition visiblePosition = visiblePositionRange.start;
     int characterOffset = 0;
-    Position vpDeepPos = vp.deepEquivalent();
+    Position currentPosition = visiblePosition.deepEquivalent();
     
     VisiblePosition previousVisiblePos;
-    while (!vpDeepPos.isNull() && !deepPos.equals(vpDeepPos)) {
-        previousVisiblePos = vp;
-        vp = obj->nextVisiblePosition(vp);
-        vpDeepPos = vp.deepEquivalent();
+    while (!currentPosition.isNull() && !deepPos.equals(currentPosition)) {
+        previousVisiblePos = visiblePosition;
+        visiblePosition = obj->nextVisiblePosition(visiblePosition);
+        currentPosition = visiblePosition.deepEquivalent();
+        Position previousPosition = previousVisiblePos.deepEquivalent();
         // Sometimes nextVisiblePosition will give the same VisiblePostion,
         // we break here to avoid infinite loop.
-        if (vpDeepPos.equals(previousVisiblePos.deepEquivalent()))
+        if (currentPosition.equals(previousPosition))
             break;
         characterOffset++;
         
         // When VisiblePostion moves to next node, it will count the leading line break as
         // 1 offset, which we shouldn't include in CharacterOffset.
-        if (vpDeepPos.deprecatedNode() != previousVisiblePos.deepEquivalent().deprecatedNode()) {
-            if (vp.characterBefore() == '\n')
+        if (currentPosition.deprecatedNode() != previousPosition.deprecatedNode()) {
+            if (visiblePosition.characterBefore() == '\n')
                 characterOffset--;
+        } else {
+            // Sometimes VisiblePosition will move multiple characters, like emoji.
+            if (currentPosition.deprecatedNode()->offsetInCharacters())
+                characterOffset += currentPosition.offsetInContainerNode() - previousPosition.offsetInContainerNode() - 1;
         }
     }
     
@@ -2006,7 +2055,9 @@ CharacterOffset AXObjectCache::nextCharacterOffset(const CharacterOffset& charac
     if (characterOffset.isNull())
         return CharacterOffset();
     
-    CharacterOffset next = characterOffsetForNodeAndOffset(*characterOffset.node, characterOffset.offset + 1);
+    // We don't always move one 'character' at a time since there might be composed characters.
+    int nextOffset = Position::uncheckedNextOffset(characterOffset.node, characterOffset.offset);
+    CharacterOffset next = characterOffsetForNodeAndOffset(*characterOffset.node, nextOffset);
     
     // To be consistent with VisiblePosition, we should consider the case that current node end to next node start counts 1 offset.
     bool isReplacedOrBR = isReplacedNodeOrBR(characterOffset.node) || isReplacedNodeOrBR(next.node);
@@ -2025,7 +2076,9 @@ CharacterOffset AXObjectCache::previousCharacterOffset(const CharacterOffset& ch
     if (!ignorePreviousNodeEnd && !characterOffset.offset)
         return characterOffsetForNodeAndOffset(*characterOffset.node, 0);
     
-    return characterOffsetForNodeAndOffset(*characterOffset.node, characterOffset.offset - 1, TraverseOptionIncludeStart);
+    // We don't always move one 'character' a time since there might be composed characters.
+    int previousOffset = Position::uncheckedPreviousOffset(characterOffset.node, characterOffset.offset);
+    return characterOffsetForNodeAndOffset(*characterOffset.node, previousOffset, TraverseOptionIncludeStart);
 }
 
 CharacterOffset AXObjectCache::startCharacterOffsetOfWord(const CharacterOffset& characterOffset, EWordSide side)