Attachment 79238 Details for Bug 51001 – Ugh. check-webkit-style.

[patch] Ugh. check-webkit-style.

bug-51001-20110118142416.patch (text/plain), 14.58 KB, created by Benjamin (Ben) Kalman on 2011-01-17 19:24:19 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin (Ben) Kalman

Created: 2011-01-17 19:24:19 PST

Size: 14.58 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 7ac1e2e2ddd328a93372af164e9bc8fedd03d57f..201c469664dcc3b5511c94b1025685d19b08c701 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2011-01-17  Benjamin Kalman  <kalman@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Moving or selecting backwards by words jumps to start of contenteditable region if contenteditable=false span is encountered
>+        https://bugs.webkit.org/show_bug.cgi?id=51001
>+
>+        Add regression test.
>+
>+        * editing/selection/extend-backward-by-word-over-non-editable-expected.txt: Added.
>+        * editing/selection/extend-backward-by-word-over-non-editable.html: Added.
>+
> 2011-01-17  Dan Bernstein  <mitz@apple.com>
> 
>         Reviewed by Darin Adler.
>diff --git a/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..fc4fb0379d370be98da711bd2beb5e807b4c60e3
>--- /dev/null
>+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt
>@@ -0,0 +1,5 @@
>+PASS for LTR
>+PASS for RTL
>+
>+foo bar baz qux quux
>+×©×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨× ×©×Ö¼×¨×
>diff --git a/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..19249319fb04ee983a7b1e1c80e3bba0ba9274a0
>--- /dev/null
>+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html
>@@ -0,0 +1,47 @@
>+<!DOCTYPE html>
>+<pre id="log"></pre>
>+
>+<div id="ltrTextContainer" contenteditable>foo bar <span contenteditable=false>baz</span> qux quux</div>
>+<div id="rtlTextContainer" contenteditable dir="rtl">&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4; &#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4;
>+<span contenteditable=false>&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4;</span>
>+&#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4; &#x05E9;&#x05D5;&#x05BC;&#x05E8;&#x05D4;</div>
>+
>+<script>
>+function log(s) {
>+  document.getElementById("log").appendChild(document.createTextNode(s + "\n"));
>+}
>+
>+function placeCursorAfterFirstNoneditableChild(container) {
>+  for (var i = 0; i < container.childNodes.length; i++) {
>+    var node = container.childNodes[i];
>+    if (node.isContentEditable !== undefined && !node.isContentEditable) {
>+      getSelection().setPosition(node.nextSibling, 0);
>+      return node;
>+    }
>+  }
>+  throw "Couldn't find noneditable child of " + container.textContent;
>+}
>+
>+ltrTextContainer = document.getElementById("ltrTextContainer");
>+noneditableChild = placeCursorAfterFirstNoneditableChild(ltrTextContainer);
>+getSelection().modify("extend", "backward", "word");
>+
>+if (getSelection().toString() === noneditableChild.textContent)
>+  log("PASS for LTR");
>+else
>+  log("FAIL for LTR, selection is \"" + getSelection() + "\" but should be \"" + noneditableChild.textContent + "\"");
>+
>+rtlTextContainer = document.getElementById("rtlTextContainer");
>+noneditableChild = placeCursorAfterFirstNoneditableChild(rtlTextContainer);
>+getSelection().modify("extend", "backward", "word");
>+
>+if (getSelection().toString() === noneditableChild.textContent)
>+  log("PASS for RTL");
>+else
>+  log("FAIL for RTL, selection is \"" + getSelection() + "\" but should be \"" + noneditableChild.textContent + "\"");
>+
>+log("");
>+
>+if (window.layoutTestController)
>+    layoutTestController.dumpAsText();
>+</script>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3302905d77e9a576d051bff101818fb8dc90de1b..aebfb2d1e6f0ce3d9273a465a236618ec531eae6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,33 @@
>+2011-01-17  Benjamin Kalman  <kalman@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Moving or selecting backwards by words jumps to start of contenteditable region if contenteditable=false span is encountered
>+        https://bugs.webkit.org/show_bug.cgi?id=51001
>+
>+        Test: editing/selection/extend-backward-by-word-over-non-editable.html
>+
>+        Revert some previous changes (the TextIteratorEndsAtEditingBoundary text iteration
>+        behaviour) which caused this bug in the first place, and add alternative fix for the problem
>+        that TextIterationEndsAtEditingBoundary solved.
>+
>+        * editing/Editor.cpp:
>+        (WebCore::Editor::markMisspellingsAfterTypingToWord): Use wordStart argument as-is, don't
>+        redundantly call startOfWord on it again.
>+        * editing/TextIterator.cpp: Remove references to TextIteratorEndsAtEditingBoundary, add
>+        alternative fix for the problem which it solved.
>+        (WebCore::TextIterator::TextIterator):
>+        (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator):
>+        (WebCore::SimplifiedBackwardsTextIterator::advance): Call exitNode() until all containers
>+        have been exited, rather than stopping after the first.  This fixes the problem where
>+        multiple \n characters were being emitted from the iterator in cases where there should only
>+        have been one, such as nested divs.
>+        * editing/TextIterator.h: Remove RemoveTextIteratorEndsAtEditingBoundary.
>+        * editing/visible_units.cpp:
>+        (WebCore::rightmostEquivalent):
>+        (WebCore::previousBoundary): Use the rightmost equivalent position when returning the
>+        endPosition of the range.
>+
> 2011-01-17  Naoki Takano  <takano.naoki@gmail.com>
> 
>         Reviewed by Kent Tamura.
>@@ -2629,7 +2659,6 @@
> 
>         * html/canvas/WebGLRenderingContext.cpp:
> 
>->>>>>>> .r75740
> 2011-01-13  Adrienne Walker  <enne@google.com>
> 
>         Reviewed by Kenneth Russell.
>diff --git a/Source/WebCore/editing/Editor.cpp b/Source/WebCore/editing/Editor.cpp
>index bea74d96120d1a38fd6884a62271fccaeea6dce2..52531a58e9ed6d5abd5e46cfb0c2cf7eb9a39744 100644
>--- a/Source/WebCore/editing/Editor.cpp
>+++ b/Source/WebCore/editing/Editor.cpp
>@@ -2088,7 +2088,7 @@ void Editor::markMisspellingsAfterTypingToWord(const VisiblePosition &wordStart,
>     if (isGrammarCheckingEnabled())
>         textCheckingOptions |= MarkGrammar;
> 
>-    VisibleSelection adjacentWords = VisibleSelection(startOfWord(wordStart, LeftWordIfOnBoundary), endOfWord(wordStart, RightWordIfOnBoundary));
>+    VisibleSelection adjacentWords = VisibleSelection(wordStart, endOfWord(wordStart, RightWordIfOnBoundary));
>     if (textCheckingOptions & MarkGrammar) {
>         VisibleSelection selectedSentence = VisibleSelection(startOfSentence(wordStart), endOfSentence(wordStart));
>         markAllMisspellingsAndBadGrammarInRanges(textCheckingOptions, adjacentWords.toNormalizedRange().get(), selectedSentence.toNormalizedRange().get());
>diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp
>index 7e41420ddf4c65fb58e2ba48db50b647ef7a8c9d..2200652f11abf999eca07b3dc4d24fa0a5b4661c 100644
>--- a/Source/WebCore/editing/TextIterator.cpp
>+++ b/Source/WebCore/editing/TextIterator.cpp
>@@ -289,9 +289,6 @@ TextIterator::TextIterator(const Range* r, TextIteratorBehavior behavior)
>     , m_handledFirstLetter(false)
>     , m_ignoresStyleVisibility(behavior & TextIteratorIgnoresStyleVisibility)
> {
>-    // FIXME: should support TextIteratorEndsAtEditingBoundary http://webkit.org/b/43609
>-    ASSERT(behavior != TextIteratorEndsAtEditingBoundary);
>-
>     if (!r)
>         return;
> 
>@@ -1051,7 +1048,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r,
>     , m_node(0)
>     , m_positionNode(0)
> {
>-    ASSERT(m_behavior == TextIteratorDefaultBehavior || m_behavior == TextIteratorEndsAtEditingBoundary);
>+    ASSERT(m_behavior == TextIteratorDefaultBehavior);
> 
>     if (!r)
>         return;
>@@ -1076,7 +1073,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r,
>         }
>     }
> 
>-    setCurrentNode(endNode);
>+    m_node = endNode;
>     setUpFullyClippedStack(m_fullyClippedStack, m_node);    
>     m_offset = endOffset;
>     m_handledNode = false;
>@@ -1128,37 +1125,41 @@ void SimplifiedBackwardsTextIterator::advance()
>         if (!next) {
>             // Exit empty containers as we pass over them or containers
>             // where [container, 0] is where we started iterating.
>-            if (!m_handledNode &&
>-                canHaveChildrenForEditing(m_node) && 
>-                m_node->parentNode() && 
>-                (!m_node->lastChild() || (m_node == m_endNode && m_endOffset == 0))) {
>+            if (!m_handledNode
>+                    && canHaveChildrenForEditing(m_node)
>+                    && m_node->parentNode()
>+                    && (!m_node->lastChild() || (m_node == m_endNode && !m_endOffset))) {
>                 exitNode();
>                 if (m_positionNode) {
>                     m_handledNode = true;
>                     m_handledChildren = true;
>                     return;
>-                }            
>+                }
>             }
>+
>             // Exit all other containers.
>             while (!m_node->previousSibling()) {
>-                if (!setCurrentNode(m_node->parentOrHostNode()))
>+                Node* parent = m_node->parentOrHostNode();
>+                if (!parent)
>                     break;
>+                m_node = parent;
>                 m_fullyClippedStack.pop();
>                 exitNode();
>-                if (m_positionNode) {
>-                    m_handledNode = true;
>-                    m_handledChildren = true;
>-                    return;
>-                }
>+            }
>+            if (m_positionNode) {
>+                m_handledNode = true;
>+                m_handledChildren = true;
>+                return;
>             }
> 
>             next = m_node->previousSibling();
>             m_fullyClippedStack.pop();
>         }
>         
>-        if (m_node && setCurrentNode(next))
>+        if (next) {
>+            m_node = next;
>             pushFullyClippedState(m_fullyClippedStack, m_node);
>-        else
>+        } else
>             clearCurrentNode();
> 
>         // For the purpose of word boundary detection,
>@@ -1239,21 +1240,6 @@ void SimplifiedBackwardsTextIterator::emitCharacter(UChar c, Node* node, int sta
>     m_lastCharacter = c;
> }
> 
>-bool SimplifiedBackwardsTextIterator::crossesEditingBoundary(Node* node) const
>-{
>-    return m_node && m_node->isContentEditable() != node->isContentEditable();
>-}
>-
>-bool SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
>-{
>-    if (!node)
>-        return false;
>-    if (m_behavior == TextIteratorEndsAtEditingBoundary && crossesEditingBoundary(node))
>-        return false;
>-    m_node = node;
>-    return true;
>-}
>-
> void SimplifiedBackwardsTextIterator::clearCurrentNode()
> {
>     m_node = 0;
>diff --git a/Source/WebCore/editing/TextIterator.h b/Source/WebCore/editing/TextIterator.h
>index 8b61afe211b4d37b74f9f4a58237e1d28f2dcfb4..59fe2ad81dfff1961c61839e9cfe927280735abc 100644
>--- a/Source/WebCore/editing/TextIterator.h
>+++ b/Source/WebCore/editing/TextIterator.h
>@@ -41,10 +41,9 @@ enum TextIteratorBehavior {
>     TextIteratorEmitsCharactersBetweenAllVisiblePositions = 1 << 0,
>     TextIteratorEntersTextControls = 1 << 1,
>     TextIteratorEmitsTextsWithoutTranscoding = 1 << 2,
>-    TextIteratorEndsAtEditingBoundary = 1 << 3,
>-    TextIteratorIgnoresStyleVisibility = 1 << 4
>+    TextIteratorIgnoresStyleVisibility = 1 << 3
> };
>-    
>+
> // FIXME: Can't really answer this question correctly without knowing the white-space mode.
> // FIXME: Move this somewhere else in the editing directory. It doesn't belong here.
> inline bool isCollapsibleWhitespace(UChar c)
>@@ -205,8 +204,6 @@ private:
>     bool handleReplacedElement();
>     bool handleNonTextNode();
>     void emitCharacter(UChar, Node*, int startOffset, int endOffset);
>-    bool crossesEditingBoundary(Node*) const;
>-    bool setCurrentNode(Node*);
>     void clearCurrentNode();
> 
>     TextIteratorBehavior m_behavior;
>diff --git a/Source/WebCore/editing/visible_units.cpp b/Source/WebCore/editing/visible_units.cpp
>index 7bb1515736fdcf3e1e3050e7bf7b3f346bd7372b..736e6404d59daf88d5df253e99382b2d89ea4b95 100644
>--- a/Source/WebCore/editing/visible_units.cpp
>+++ b/Source/WebCore/editing/visible_units.cpp
>@@ -49,6 +49,16 @@ enum BoundarySearchContextAvailability { DontHaveMoreContext, MayHaveMoreContext
> 
> typedef unsigned (*BoundarySearchFunction)(const UChar*, unsigned length, unsigned offset, BoundarySearchContextAvailability, bool& needMoreContext);
> 
>+static Position rightmostEquivalent(Position position)
>+{
>+    Position next = position.next();
>+    while (position.node() != next.node()) {
>+        position = next;
>+        next = position.next();
>+    }
>+    return position;
>+}
>+
> static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearchFunction searchFunction)
> {
>     Position pos = c.deepEquivalent();
>@@ -89,7 +99,7 @@ static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearch
>     if (ec)
>         return VisiblePosition();
> 
>-    SimplifiedBackwardsTextIterator it(searchRange.get(), TextIteratorEndsAtEditingBoundary);
>+    SimplifiedBackwardsTextIterator it(searchRange.get());
>     unsigned next = 0;
>     bool inTextSecurityMode = start.node() && start.node()->renderer() && start.node()->renderer()->style()->textSecurity() != TSNONE;
>     bool needMoreContext = false;
>@@ -115,18 +125,19 @@ static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearch
>         ASSERT(!needMoreContext);
>     }
> 
>-    if (it.atEnd() && next == 0) {
>+    if (it.atEnd() && !next)
>         pos = it.range()->startPosition();
>-    } else if (next != 0) {
>+    else if (next) {
>         Node *node = it.range()->startContainer(ec);
>-        if ((node->isTextNode() && static_cast<int>(next) <= node->maxCharacterOffset()) || (node->renderer() && node->renderer()->isBR() && !next))
>+        if ((node->isTextNode() && static_cast<int>(next) <= node->maxCharacterOffset()) || (node->renderer() && node->renderer()->isBR() && !next)) {
>             // The next variable contains a usable index into a text node
>             pos = Position(node, next);
>-        else {
>+        } else {    
>             // Use the character iterator to translate the next value into a DOM position.
>-            BackwardsCharacterIterator charIt(searchRange.get(), TextIteratorEndsAtEditingBoundary);
>+            BackwardsCharacterIterator charIt(searchRange.get());
>             charIt.advance(string.size() - suffixLength - next);
>-            pos = charIt.range()->endPosition();
>+            // Take the rightmost equivalent position so that the correct containing editable region is used if a boundary is reached.
>+            pos = rightmostEquivalent(charIt.range()->endPosition());
>         }
>     }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 51001: 76483 | 76506 | 76850 | 77653 | 77655 | 78624 | 79237 | 79238 | 79349 | 79699 | 79884 | 80023 | 80024 | 80273 | 83122 | 83444 | 83447 | 83450