Diff
Modified: trunk/LayoutTests/ChangeLog (200257 => 200258)
--- trunk/LayoutTests/ChangeLog 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/LayoutTests/ChangeLog 2016-04-29 20:05:07 UTC (rev 200258)
@@ -1,3 +1,14 @@
+2016-04-29 Nan Wang <n_w...@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 <ryanhad...@apple.com>
Marking fast/ruby/ruby-expansion-cjk.html and fast/ruby/ruby-expansion-cjk-4.html as flaky on Mac
Modified: trunk/LayoutTests/accessibility/mac/text-marker-word-nav.html (200257 => 200258)
--- trunk/LayoutTests/accessibility/mac/text-marker-word-nav.html 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/LayoutTests/accessibility/mac/text-marker-word-nav.html 2016-04-29 20:05:07 UTC (rev 200258)
@@ -67,7 +67,7 @@
// 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 "."
Modified: trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next-expected.txt (200257 => 200258)
--- trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next-expected.txt 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next-expected.txt 2016-04-29 20:05:07 UTC (rev 200258)
@@ -5,6 +5,10 @@
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(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
Modified: trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next.html (200257 => 200258)
--- trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next.html 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/LayoutTests/accessibility/text-marker/text-marker-previous-next.html 2016-04-29 20:05:07 UTC (rev 200258)
@@ -2,6 +2,7 @@
<html>
<head>
<script src=""
+<meta charset="UTF-16">
</head>
<style>
.userselect { user-select: none; -webkit-user-select: none; }
@@ -26,6 +27,10 @@
f
</div>
+<p id="text5">😃😏<p>
+
+<p id="text6">a b</p>
+
<p id="description"></p>
<div id="console"></div>
@@ -71,8 +76,8 @@
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 @@
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 @@
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;
Modified: trunk/Source/WebCore/ChangeLog (200257 => 200258)
--- trunk/Source/WebCore/ChangeLog 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/Source/WebCore/ChangeLog 2016-04-29 20:05:07 UTC (rev 200258)
@@ -1,3 +1,27 @@
+2016-04-29 Nan Wang <n_w...@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.no...@apple.com>
WebPlaybackControlsManager should not be owned by the WebPlaybackSessionInterfaceMac.
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (200257 => 200258)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2016-04-29 20:01:42 UTC (rev 200257)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2016-04-29 20:05:07 UTC (rev 200258)
@@ -1479,7 +1479,7 @@
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 @@
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 @@
// 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 @@
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 @@
}
// 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 @@
if (!finished) {
offsetInCharacter = lastLength;
if (!toNodeEnd)
- remaining = offset - offsetSoFar;
+ remaining = offset - cumulativeOffset;
}
return CharacterOffset(currentNode, lastStartOffset, offsetInCharacter, remaining);
@@ -1850,22 +1880,36 @@
{
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 @@
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 @@
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 @@
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)
