Diff
Modified: trunk/LayoutTests/ChangeLog (93346 => 93347)
--- trunk/LayoutTests/ChangeLog 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/LayoutTests/ChangeLog 2011-08-18 20:41:51 UTC (rev 93347)
@@ -1,3 +1,19 @@
+2011-08-18 Ryosuke Niwa <[email protected]>
+
+ SimplifiedBackwardsTextIterator returns incorrect offset with first-letter rule
+ https://bugs.webkit.org/show_bug.cgi?id=66086
+
+ Reviewed by Darin Adler.
+
+ Added a test to ensure WebKit does not hit assertions in SimplifiedBackwardsTextIterator.
+ Also fixed a bug in first-letter-word-boundary.html and updated expected offsets for move backward by word
+ from 0 to 1 because there is unrendered space before "hello".
+
+ * editing/text-iterator/backward-textiterator-first-letter-crash-expected.txt: Added.
+ * editing/text-iterator/backward-textiterator-first-letter-crash.html: Added.
+ * editing/text-iterator/first-letter-word-boundary-expected.txt:
+ * editing/text-iterator/first-letter-word-boundary.html:
+
2011-08-18 Alexey Proskuryakov <[email protected]>
Regional indicator symbols that are combined should behave as a single character when editing
Added: trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash-expected.txt (0 => 93347)
--- trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash-expected.txt 2011-08-18 20:41:51 UTC (rev 93347)
@@ -0,0 +1 @@
+PASS if WebKit did not hit assertions
Added: trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash.html (0 => 93347)
--- trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash.html (rev 0)
+++ trunk/LayoutTests/editing/text-iterator/backward-textiterator-first-letter-crash.html 2011-08-18 20:41:51 UTC (rev 93347)
@@ -0,0 +1,15 @@
+<style>
+div:first-letter { margin-top: 0em; }
+</style>
+<script>
+if (window.layoutTestController) {
+ layoutTestController.dumpAsText();
+ layoutTestController.waitUntilDone();
+}
+function done() {
+ document.body.innerHTML = 'PASS if WebKit did not hit assertions';
+ if (window.layoutTestController)
+ layoutTestController.notifyDone();
+}
+</script>
+<body _onload_="setTimeout(done, 100);"><div>AB<select autofocus contenteditable>
Modified: trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary-expected.txt (93346 => 93347)
--- trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary-expected.txt 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary-expected.txt 2011-08-18 20:41:51 UTC (rev 93347)
@@ -2,9 +2,9 @@
hello world'
white-space: normal;
-FAIL: moving forward by word put caret at offset 4 but expected 6
-PASS: moving backward by word put caret at offset 0
+FAIL: moving forward by word from offset 4 put caret at offset 10 but expected 6
+PASS: moving backward by word from offset 4 put caret at offset 1
white-space: pre;
-FAIL: moving forward by word put caret at offset 4 but expected 6
-PASS: moving backward by word put caret at offset 0
+FAIL: moving forward by word from offset 4 put caret at offset 10 but expected 6
+PASS: moving backward by word from offset 4 put caret at offset 1
Modified: trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary.html (93346 => 93347)
--- trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary.html 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/LayoutTests/editing/text-iterator/first-letter-word-boundary.html 2011-08-18 20:41:51 UTC (rev 93347)
@@ -20,7 +20,8 @@
layoutTestController.dumpAsText();
function runTest(actor, expectedOffset) {
- var action = "" + ' put caret at offset ';
+ window.getSelection().setPosition(test.firstChild, 4);
+ var action = "" + ' from offset ' + 4 + ' put caret at offset ';
var startOffset = window.getSelection().getRangeAt(0).startOffset;
action += startOffset;
if (startOffset == expectedOffset)
@@ -31,16 +32,15 @@
var test = document.getElementById('test');
var console = document.getElementById('console');
-window.getSelection().setPosition(test, 0);
console.innerHTML += 'white-space: normal;\n';
runTest(function () {window.getSelection().modify('move', 'forward', 'word'); return 'moving forward by word';}, 6);
-runTest(function () {window.getSelection().modify('move', 'backward', 'word'); return 'moving backward by word';}, 0);
+runTest(function () {window.getSelection().modify('move', 'backward', 'word'); return 'moving backward by word';}, 1);
console.innerHTML += 'white-space: pre;\n';
test.style.whiteSpace = 'pre';
runTest(function () {window.getSelection().modify('move', 'forward', 'word'); return 'moving forward by word';}, 6);
-runTest(function () {window.getSelection().modify('move', 'backward', 'word'); return 'moving backward by word';}, 0);
+runTest(function () {window.getSelection().modify('move', 'backward', 'word'); return 'moving backward by word';}, 1);
</script>
</body>
Modified: trunk/Source/WebCore/ChangeLog (93346 => 93347)
--- trunk/Source/WebCore/ChangeLog 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/Source/WebCore/ChangeLog 2011-08-18 20:41:51 UTC (rev 93347)
@@ -1,3 +1,26 @@
+2011-08-18 Ryosuke Niwa <[email protected]>
+
+ SimplifiedBackwardsTextIterator returns incorrect offset with first-letter rule
+ https://bugs.webkit.org/show_bug.cgi?id=66086
+
+ Reviewed by Darin Adler.
+
+ The bug was caused by SimplifiedBackwardsTextIterator's not taking care of first-letter at all.
+ Fixing the bug by detecting RenderTextFragment in handleTextNode.
+
+ Also added m_shouldHandleFirstLetter to SimplifiedBackwardsTextIterator to keep track of whether or not
+ the next call to handleTextNode needs to process the first-letter part of the text fragment.
+
+ Test: editing/text-iterator/backward-textiterator-first-letter-crash.html
+
+ * editing/TextIterator.cpp:
+ (WebCore::firstRenderTextInFirstLetter): Extracted from handleTextNodeFirstLetter.
+ (WebCore::TextIterator::handleTextNodeFirstLetter): Calls firstRenderTextInFirstLetter.
+ (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator):
+ (WebCore::SimplifiedBackwardsTextIterator::handleTextNode):
+ (WebCore::SimplifiedBackwardsTextIterator::handleFirstLetter): Added.
+ * editing/TextIterator.h:
+
2011-08-18 Iain Merrick <[email protected]>
[chromium] Assert that main thread and compositor thread are used safely
Modified: trunk/Source/WebCore/editing/TextIterator.cpp (93346 => 93347)
--- trunk/Source/WebCore/editing/TextIterator.cpp 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/Source/WebCore/editing/TextIterator.cpp 2011-08-18 20:41:51 UTC (rev 93347)
@@ -603,21 +603,30 @@
}
}
+static inline RenderText* firstRenderTextInFirstLetter(RenderObject* firstLetter)
+{
+ if (!firstLetter)
+ return 0;
+
+ // FIXME: Should this check descendent objects?
+ for (RenderObject* current = firstLetter->firstChild(); current; current = current->nextSibling()) {
+ if (current->isText())
+ return toRenderText(current);
+ }
+ return 0;
+}
+
void TextIterator::handleTextNodeFirstLetter(RenderTextFragment* renderer)
{
if (renderer->firstLetter()) {
RenderObject* r = renderer->firstLetter();
if (r->style()->visibility() != VISIBLE && !m_ignoresStyleVisibility)
return;
- for (RenderObject *currChild = r->firstChild(); currChild; currChild->nextSibling()) {
- if (currChild->isText()) {
- RenderText* firstLetter = toRenderText(currChild);
- m_handledFirstLetter = true;
- m_remainingTextBox = m_textBox;
- m_textBox = firstLetter->firstTextBox();
- m_firstLetterText = firstLetter;
- return;
- }
+ if (RenderText* firstLetter = firstRenderTextInFirstLetter(r)) {
+ m_handledFirstLetter = true;
+ m_remainingTextBox = m_textBox;
+ m_textBox = firstLetter->firstTextBox();
+ m_firstLetterText = firstLetter;
}
}
m_handledFirstLetter = true;
@@ -1043,14 +1052,46 @@
SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator()
: m_behavior(TextIteratorDefaultBehavior)
, m_node(0)
+ , m_offset(0)
+ , m_handledNode(false)
+ , m_handledChildren(false)
+ , m_startNode(0)
+ , m_startOffset(0)
+ , m_endNode(0)
+ , m_endOffset(0)
, m_positionNode(0)
+ , m_positionStartOffset(0)
+ , m_positionEndOffset(0)
+ , m_textCharacters(0)
+ , m_textLength(0)
+ , m_lastTextNode(0)
+ , m_lastCharacter(0)
+ , m_singleCharacterBuffer(0)
+ , m_havePassedStartNode(false)
+ , m_shouldHandleFirstLetter(false)
{
}
SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r, TextIteratorBehavior behavior)
: m_behavior(behavior)
, m_node(0)
+ , m_offset(0)
+ , m_handledNode(false)
+ , m_handledChildren(false)
+ , m_startNode(0)
+ , m_startOffset(0)
+ , m_endNode(0)
+ , m_endOffset(0)
, m_positionNode(0)
+ , m_positionStartOffset(0)
+ , m_positionEndOffset(0)
+ , m_textCharacters(0)
+ , m_textLength(0)
+ , m_lastTextNode(0)
+ , m_lastCharacter(0)
+ , m_singleCharacterBuffer(0)
+ , m_havePassedStartNode(false)
+ , m_shouldHandleFirstLetter(false)
{
ASSERT(m_behavior == TextIteratorDefaultBehavior);
@@ -1178,25 +1219,64 @@
{
m_lastTextNode = m_node;
- RenderText* renderer = toRenderText(m_node->renderer());
- String str = renderer->text();
+ int startOffset;
+ int offsetInNode;
+ RenderText* renderer = handleFirstLetter(startOffset, offsetInNode);
+ if (!renderer)
+ return true;
- if (!renderer->firstTextBox() && str.length() > 0)
+ String text = renderer->text();
+ if (!renderer->firstTextBox() && text.length() > 0)
return true;
m_positionEndOffset = m_offset;
-
- m_offset = (m_node == m_startNode) ? m_startOffset : 0;
+ m_offset = startOffset + offsetInNode;
m_positionNode = m_node;
m_positionStartOffset = m_offset;
+
+ ASSERT(0 <= m_positionStartOffset - offsetInNode && m_positionStartOffset - offsetInNode <= static_cast<int>(text.length()));
+ ASSERT(1 <= m_positionEndOffset - offsetInNode && m_positionEndOffset - offsetInNode <= static_cast<int>(text.length()));
+ ASSERT(m_positionStartOffset <= m_positionEndOffset);
+
m_textLength = m_positionEndOffset - m_positionStartOffset;
- m_textCharacters = str.characters() + m_positionStartOffset;
+ m_textCharacters = text.characters() + (m_positionStartOffset - offsetInNode);
+ ASSERT(m_textCharacters >= text.characters());
+ ASSERT(m_textCharacters + m_textLength <= text.characters() + static_cast<int>(text.length()));
- m_lastCharacter = str[m_positionEndOffset - 1];
+ m_lastCharacter = text[m_positionEndOffset - 1];
- return true;
+ return !m_shouldHandleFirstLetter;
}
+RenderText* SimplifiedBackwardsTextIterator::handleFirstLetter(int& startOffset, int& offsetInNode)
+{
+ RenderText* renderer = toRenderText(m_node->renderer());
+ startOffset = (m_node == m_startNode) ? m_startOffset : 0;
+
+ if (!renderer->isTextFragment()) {
+ offsetInNode = 0;
+ return renderer;
+ }
+
+ RenderTextFragment* fragment = toRenderTextFragment(renderer);
+ int offsetAfterFirstLetter = fragment->start();
+ if (startOffset >= offsetAfterFirstLetter) {
+ ASSERT(!m_shouldHandleFirstLetter);
+ offsetInNode = offsetAfterFirstLetter;
+ return renderer;
+ }
+
+ if (!m_shouldHandleFirstLetter && offsetAfterFirstLetter < m_offset) {
+ m_shouldHandleFirstLetter = true;
+ offsetInNode = offsetAfterFirstLetter;
+ return renderer;
+ }
+
+ m_shouldHandleFirstLetter = false;
+ offsetInNode = 0;
+ return firstRenderTextInFirstLetter(fragment->firstLetter());
+}
+
bool SimplifiedBackwardsTextIterator::handleReplacedElement()
{
unsigned index = m_node->nodeIndex();
Modified: trunk/Source/WebCore/editing/TextIterator.h (93346 => 93347)
--- trunk/Source/WebCore/editing/TextIterator.h 2011-08-18 20:37:25 UTC (rev 93346)
+++ trunk/Source/WebCore/editing/TextIterator.h 2011-08-18 20:41:51 UTC (rev 93347)
@@ -203,6 +203,7 @@
private:
void exitNode();
bool handleTextNode();
+ RenderText* handleFirstLetter(int& startOffset, int& offsetInNode);
bool handleReplacedElement();
bool handleNonTextNode();
void emitCharacter(UChar, Node*, int startOffset, int endOffset);
@@ -240,6 +241,9 @@
// Whether m_node has advanced beyond the iteration range (i.e. m_startNode).
bool m_havePassedStartNode;
+
+ // Should handle first-letter renderer in the next call to handleTextNode.
+ bool m_shouldHandleFirstLetter;
};
// Builds on the text iterator, adding a character position so we can walk one
