Attachment 83450 Details for Bug 51001 – Patch for landing

[patch] Patch for landing

bug-51001-20110223180015.patch (text/plain), 14.24 KB, created by Benjamin (Ben) Kalman on 2011-02-22 23:00:18 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin (Ben) Kalman

Created: 2011-02-22 23:00:18 PST

Size: 14.24 KB

patch

obsolete

>Subversion Revision: 76075
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f76d9a30581792ea84ff3af4514617e8c78b7940..72b3d143360adbf890a51d34585a02ec90c0c9ae 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2011-02-23  Benjamin Kalman  <kalman@chromium.org>
>+
>+        Reviewed by Ojan Vafai.
>+
>+        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-02-22  MORITA Hajime  <morrita@google.com>
> 
>         Unreviewed, added Leopard-specific expectations for Chromium.
>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..b52b0a9e799b23ea8d96a171fa59e247a983fe79
>--- /dev/null
>+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable-expected.txt
>@@ -0,0 +1,4 @@
>+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..6135db74fae40f62a34f59d39f29e7a665630d10
>--- /dev/null
>+++ b/LayoutTests/editing/selection/extend-backward-by-word-over-non-editable.html
>@@ -0,0 +1,39 @@
>+<!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;
>+}
>+
>+function extendBackwardByWord(container, bidiName) {
>+  noneditableChild = placeCursorAfterFirstNoneditableChild(container);
>+  getSelection().modify("extend", "backward", "word");
>+  if (getSelection().toString() === noneditableChild.textContent)
>+    log("PASS for " + bidiName);
>+  else
>+    log("FAIL for " + bidiName + ", selection is \"" + getSelection() + "\" but should be \"" + noneditableChild.textContent + "\"");
>+}
>+
>+extendBackwardByWord(document.getElementById("ltrTextContainer"), "LTR");
>+extendBackwardByWord(document.getElementById("rtlTextContainer"), "RTL");
>+
>+if (window.layoutTestController)
>+    layoutTestController.dumpAsText();
>+</script>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 34846fa99a7b4496e85e32248f2212ae9a0e39e4..97e2b29ef1cda6bd403ec9df0f2bf85353eb352c 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,36 @@
>+2011-02-23  Benjamin Kalman  <kalman@chromium.org>
>+
>+        Reviewed by Ojan Vafai.
>+
>+        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 fix SimplifiedBackwardsTextIterator's iteration range check as an alternative fix.
>+
>+        The original bug was that double-clicking on an inline editable span at the start of a paragraph would clear the
>+        selection (webkit.org/b/36360).  This was caused by upstream/downstream VisbiblePosition complications.  To fix,
>+        refuse to iterate beyond the start node (rather than refusing to iterate across editable boundaries, which
>+        causes this bug).
>+
>+        To see why this is correct, and to make it clearer that is indeed what is happening, the text iterator code has
>+        been slightly restructured to express the invariant that the iterator will never advance past the start node.
>+
>+        * editing/TextIterator.cpp:
>+        (WebCore::TextIterator::TextIterator): Remove references to TextIterationEndsAtEditing boundary.
>+        (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator): Remove m_pastStartNode as the
>+        mechanism for iteration range checking, and use a flag m_havePassedStartNode instead.
>+        (WebCore::SimplifiedBackwardsTextIterator::advance): Clean up, use advanceRespectingRange and
>+        m_havePassedStartNode for iteration range checking rather than m_pastStartNode.
>+        (WebCore::SimplifiedBackwardsTextIterator::advanceRespectingRange): The new way of modifying m_node
>+        which updates m_havePassedStartNode and refuses to continue when it becomes true.
>+        * editing/TextIterator.h: Remove TextIteratorEndsAtEditingBoundary, update for new/removed prototypes and
>+        member variables.
>+        * editing/visible_units.cpp:
>+        (WebCore::previousBoundary): Remove references to TextIteratorEndsAtEditingBoundary.
>+
> 2011-02-22  Andreas Kling  <kling@webkit.org>
> 
>         Reviewed by Dan Bernstein.
>diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp
>index d3a65964f7e07286ecb273764324d56f1a5c8125..3b18127552af33b6358e150dc660f3412f8b0dc2 100644
>--- a/Source/WebCore/editing/TextIterator.cpp
>+++ b/Source/WebCore/editing/TextIterator.cpp
>@@ -194,21 +194,6 @@ static Node* nextInPreOrderCrossingShadowBoundaries(Node* rangeEndContainer, int
>     return 0;
> }
> 
>-static Node* previousInPostOrderCrossingShadowBoundaries(Node* rangeStartContainer, int rangeStartOffset)
>-{
>-    if (!rangeStartContainer)
>-        return 0;
>-    if (rangeStartOffset > 0 && !rangeStartContainer->offsetInCharacters()) {
>-        if (Node* previous = rangeStartContainer->childNode(rangeStartOffset - 1))
>-            return previous;
>-    }
>-    for (Node* node = rangeStartContainer; node; node = node->parentOrHostNode()) {
>-        if (Node* previous = node->previousSibling())
>-            return previous;
>-    }
>-    return 0;
>-}
>-
> // --------
> 
> static inline bool fullyClipsContents(Node* node)
>@@ -290,9 +275,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;
> 
>@@ -1052,7 +1034,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;
>@@ -1077,7 +1059,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r,
>         }
>     }
> 
>-    setCurrentNode(endNode);
>+    m_node = endNode;
>     setUpFullyClippedStack(m_fullyClippedStack, m_node);    
>     m_offset = endOffset;
>     m_handledNode = false;
>@@ -1096,7 +1078,7 @@ SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator(const Range* r,
>     m_lastTextNode = 0;
>     m_lastCharacter = '\n';
> 
>-    m_pastStartNode = previousInPostOrderCrossingShadowBoundaries(startNode, startOffset);
>+    m_havePassedStartNode = false;
> 
>     advance();
> }
>@@ -1108,7 +1090,7 @@ void SimplifiedBackwardsTextIterator::advance()
>     m_positionNode = 0;
>     m_textLength = 0;
> 
>-    while (m_node && m_node != m_pastStartNode) {
>+    while (m_node && !m_havePassedStartNode) {
>         // Don't handle node if we start iterating at [node, 0].
>         if (!m_handledNode && !(m_node == m_endNode && m_endOffset == 0)) {
>             RenderObject* renderer = m_node->renderer();
>@@ -1125,8 +1107,10 @@ void SimplifiedBackwardsTextIterator::advance()
>                 return;
>         }
> 
>-        Node* next = m_handledChildren ? 0 : m_node->lastChild();
>-        if (!next) {
>+        if (!m_handledChildren && m_node->hasChildNodes()) {
>+            m_node = m_node->lastChild();
>+            pushFullyClippedState(m_fullyClippedStack, m_node);
>+        } else {
>             // Exit empty containers as we pass over them or containers
>             // where [container, 0] is where we started iterating.
>             if (!m_handledNode
>@@ -1138,11 +1122,12 @@ void SimplifiedBackwardsTextIterator::advance()
>                     m_handledNode = true;
>                     m_handledChildren = true;
>                     return;
>-                }            
>+                }
>             }
>+
>             // Exit all other containers.
>             while (!m_node->previousSibling()) {
>-                if (!setCurrentNode(m_node->parentOrHostNode()))
>+                if (!advanceRespectingRange(m_node->parentOrHostNode()))
>                     break;
>                 m_fullyClippedStack.pop();
>                 exitNode();
>@@ -1153,14 +1138,12 @@ void SimplifiedBackwardsTextIterator::advance()
>                 }
>             }
> 
>-            next = m_node->previousSibling();
>             m_fullyClippedStack.pop();
>+            if (advanceRespectingRange(m_node->previousSibling()))
>+                pushFullyClippedState(m_fullyClippedStack, m_node);
>+            else
>+                m_node = 0;
>         }
>-        
>-        if (m_node && setCurrentNode(next))
>-            pushFullyClippedState(m_fullyClippedStack, m_node);
>-        else
>-            clearCurrentNode();
> 
>         // For the purpose of word boundary detection,
>         // we should iterate all visible text and trailing (collapsed) whitespaces. 
>@@ -1240,26 +1223,17 @@ void SimplifiedBackwardsTextIterator::emitCharacter(UChar c, Node* node, int sta
>     m_lastCharacter = c;
> }
> 
>-bool SimplifiedBackwardsTextIterator::crossesEditingBoundary(Node* node) const
>+bool SimplifiedBackwardsTextIterator::advanceRespectingRange(Node* next)
> {
>-    return m_node && m_node->isContentEditable() != node->isContentEditable();
>-}
>-
>-bool SimplifiedBackwardsTextIterator::setCurrentNode(Node* node)
>-{
>-    if (!node)
>+    if (!next)
>         return false;
>-    if (m_behavior == TextIteratorEndsAtEditingBoundary && crossesEditingBoundary(node))
>+    m_havePassedStartNode |= m_node == m_startNode;
>+    if (m_havePassedStartNode)
>         return false;
>-    m_node = node;
>+    m_node = next;
>     return true;
> }
> 
>-void SimplifiedBackwardsTextIterator::clearCurrentNode()
>-{
>-    m_node = 0;
>-}
>-
> PassRefPtr<Range> SimplifiedBackwardsTextIterator::range() const
> {
>     if (m_positionNode)
>diff --git a/Source/WebCore/editing/TextIterator.h b/Source/WebCore/editing/TextIterator.h
>index 8b61afe211b4d37b74f9f4a58237e1d28f2dcfb4..b0c310a6fc7e8011b4a44c2afc1cda02dd4249d3 100644
>--- a/Source/WebCore/editing/TextIterator.h
>+++ b/Source/WebCore/editing/TextIterator.h
>@@ -41,8 +41,7 @@ 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.
>@@ -205,9 +204,7 @@ private:
>     bool handleReplacedElement();
>     bool handleNonTextNode();
>     void emitCharacter(UChar, Node*, int startOffset, int endOffset);
>-    bool crossesEditingBoundary(Node*) const;
>-    bool setCurrentNode(Node*);
>-    void clearCurrentNode();
>+    bool advanceRespectingRange(Node*);
> 
>     TextIteratorBehavior m_behavior;
>     // Current position, not necessarily of the text being returned, but position
>@@ -238,9 +235,9 @@ private:
>     
>     // Used for whitespace characters that aren't in the DOM, so we can point at them.
>     UChar m_singleCharacterBuffer;
>-    
>-    // The node after the last node this iterator should process.
>-    Node* m_pastStartNode;
>+
>+    // Whether m_node has advanced beyond the iteration range (i.e. m_startNode).
>+    bool m_havePassedStartNode;
> };
> 
> // Builds on the text iterator, adding a character position so we can walk one
>diff --git a/Source/WebCore/editing/visible_units.cpp b/Source/WebCore/editing/visible_units.cpp
>index 281bc41c4b7b6383261418a4ac9393bd10a42da5..9cb4d6d0516b4ee24d60e12b41560f7266a938e8 100644
>--- a/Source/WebCore/editing/visible_units.cpp
>+++ b/Source/WebCore/editing/visible_units.cpp
>@@ -89,7 +89,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.deprecatedNode() && start.deprecatedNode()->renderer() && start.deprecatedNode()->renderer()->style()->textSecurity() != TSNONE;
>     bool needMoreContext = false;
>@@ -124,7 +124,7 @@ static VisiblePosition previousBoundary(const VisiblePosition& c, BoundarySearch
>         return VisiblePosition(Position(node, next), DOWNSTREAM);
> 
>     // 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);
>     return VisiblePosition(charIt.range()->endPosition(), DOWNSTREAM);
> }

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