Attachment 78624 Details for Bug 51001 – Patch with parentOrHostNode changed to parentNode

[patch] Patch with parentOrHostNode changed to parentNode

bug-51001-20110112114640.patch (text/plain), 11.66 KB, created by Benjamin (Ben) Kalman on 2011-01-11 16:46:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Benjamin (Ben) Kalman

Created: 2011-01-11 16:46:43 PST

Size: 11.66 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f98f46912e090dbb56013b74de68c4af790c27df..0dea09b5f43c7067a8aca5a03e48aa268ba15479 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2011-01-11  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-11  Justin Schuh  <jschuh@chromium.org>
> 
>         Unreviewed, test expectations update.
>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 803e5f12c171d75096c0a024c5c62cba94781237..c0b42578c9c46f7427d662fc8bc1905f5b0f2db8 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2011-01-11  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/TextIterator.cpp: Remove references to TextIteratorEndsAtEditingBoundary.
>+        (WebCore::TextIterator::TextIterator):
>+        (WebCore::SimplifiedBackwardsTextIterator::SimplifiedBackwardsTextIterator):
>+        (WebCore::SimplifiedBackwardsTextIterator::advance): Don't advance out of the shadow dom.
>+        * 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-11  James Simonsen  <simonjam@chromium.org>
> 
>         Reviewed by Eric Seidel.
>diff --git a/Source/WebCore/editing/TextIterator.cpp b/Source/WebCore/editing/TextIterator.cpp
>index 7e41420ddf4c65fb58e2ba48db50b647ef7a8c9d..96875301b62f895d85cd49867dcc1f73d1afcff0 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;
>@@ -1141,8 +1138,10 @@ void SimplifiedBackwardsTextIterator::advance()
>             }
>             // Exit all other containers.
>             while (!m_node->previousSibling()) {
>-                if (!setCurrentNode(m_node->parentOrHostNode()))
>+                Node* next = m_node->parentNode();
>+                if (!next)
>                     break;
>+                m_node = next;
>                 m_fullyClippedStack.pop();
>                 exitNode();
>                 if (m_positionNode) {
>@@ -1156,9 +1155,10 @@ void SimplifiedBackwardsTextIterator::advance()
>             m_fullyClippedStack.pop();
>         }
>         
>-        if (m_node && setCurrentNode(next))
>+        if (m_node && next) {
>+            m_node = next;
>             pushFullyClippedState(m_fullyClippedStack, m_node);
>-        else
>+        } else
>             clearCurrentNode();
> 
>         // For the purpose of word boundary detection,
>@@ -1239,21 +1239,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..b523a691be80530206b6ea32b5349f1f42b52735 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))
>             // The next variable contains a usable index into a text node
>             pos = Position(node, next);
>         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