Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Sep 2011 05:04:20 +0000 (05:04 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Sep 2011 05:04:20 +0000 (05:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=68939

Reviewed by Darin Adler.

Simplified ReplaceSelectionCommand::positionAtStartOfInsertedContent.

This change revealed a bug in removeUnrenderedTextNodesAtEnds that text nodes without any visible
text at ends are not removed when it has a render object. Fixed the bug by checking the length of
the rendered text. (Tested by editing/pasteboard/pasting-word-in-div-extra-line.html)

This further revealed that caretMaxRenderedOffset doesn't return an offset and caretMaxRenderedOffset
on InlineBox, InlineTextBox, RenderObject, RenderBR, RenderPlaced are never called. To address this
issue, renamed caretMaxRenderedOffset to renderedTextLength for RenderText and removed the rest.

* dom/Position.cpp:
(WebCore::Position::rendersInDifferentPosition):
* editing/ReplaceSelectionCommand.cpp:
(WebCore::nodeHasVisibleRenderText): Added.
(WebCore::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds): Calls nodeHasVisibleRenderText.
(WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent): Simplified.
* editing/visible_units.cpp:
(WebCore::startOfParagraph): Calls renderedTextLength.
(WebCore::endOfParagraph): Ditto.
* rendering/InlineBox.cpp: Removed caretMaxRenderedOffset.
* rendering/InlineBox.h: Ditto.
* rendering/InlineTextBox.cpp: Ditto.
* rendering/InlineTextBox.h: Ditto.
* rendering/RenderBR.cpp: Ditto.
* rendering/RenderBR.h: Ditto.
* rendering/RenderObject.cpp: Ditto.
* rendering/RenderObject.h: Ditto.
* rendering/RenderReplaced.cpp: Ditto.
* rendering/RenderReplaced.h: Ditto.
* rendering/RenderText.cpp:
(WebCore::RenderText::renderedTextLength): Renamed from caretMaxRenderedOffset.
* rendering/RenderText.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@96187 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/Position.cpp
Source/WebCore/editing/ReplaceSelectionCommand.cpp
Source/WebCore/editing/visible_units.cpp
Source/WebCore/rendering/InlineBox.cpp
Source/WebCore/rendering/InlineBox.h
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/InlineTextBox.h
Source/WebCore/rendering/RenderBR.cpp
Source/WebCore/rendering/RenderBR.h
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderObject.h
Source/WebCore/rendering/RenderReplaced.cpp
Source/WebCore/rendering/RenderReplaced.h
Source/WebCore/rendering/RenderText.cpp
Source/WebCore/rendering/RenderText.h

index a932148..654a55a 100644 (file)
@@ -1,3 +1,43 @@
+2011-09-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Simplify ReplaceSelectionCommand::positionAtStartOfInsertedContent
+        https://bugs.webkit.org/show_bug.cgi?id=68939
+
+        Reviewed by Darin Adler.
+
+        Simplified ReplaceSelectionCommand::positionAtStartOfInsertedContent.
+
+        This change revealed a bug in removeUnrenderedTextNodesAtEnds that text nodes without any visible
+        text at ends are not removed when it has a render object. Fixed the bug by checking the length of
+        the rendered text. (Tested by editing/pasteboard/pasting-word-in-div-extra-line.html)
+
+        This further revealed that caretMaxRenderedOffset doesn't return an offset and caretMaxRenderedOffset
+        on InlineBox, InlineTextBox, RenderObject, RenderBR, RenderPlaced are never called. To address this
+        issue, renamed caretMaxRenderedOffset to renderedTextLength for RenderText and removed the rest.
+
+        * dom/Position.cpp:
+        (WebCore::Position::rendersInDifferentPosition):
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::nodeHasVisibleRenderText): Added.
+        (WebCore::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds): Calls nodeHasVisibleRenderText.
+        (WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent): Simplified.
+        * editing/visible_units.cpp:
+        (WebCore::startOfParagraph): Calls renderedTextLength.
+        (WebCore::endOfParagraph): Ditto.
+        * rendering/InlineBox.cpp: Removed caretMaxRenderedOffset.
+        * rendering/InlineBox.h: Ditto.
+        * rendering/InlineTextBox.cpp: Ditto.
+        * rendering/InlineTextBox.h: Ditto.
+        * rendering/RenderBR.cpp: Ditto.
+        * rendering/RenderBR.h: Ditto.
+        * rendering/RenderObject.cpp: Ditto.
+        * rendering/RenderObject.h: Ditto.
+        * rendering/RenderReplaced.cpp: Ditto.
+        * rendering/RenderReplaced.h: Ditto.
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::renderedTextLength): Renamed from caretMaxRenderedOffset.
+        * rendering/RenderText.h:
+
 2011-09-27  James Robinson  <jamesr@chromium.org>
 
         [chromium] LayerRenderChromium asserts about leaking textures.
index 6d3d155..fed0494 100644 (file)
@@ -896,17 +896,6 @@ bool Position::inRenderedText() const
     return false;
 }
 
-static unsigned caretMaxRenderedOffset(const Node* n)
-{
-    RenderObject* r = n->renderer();
-    if (r)
-        return r->caretMaxRenderedOffset();
-    
-    if (n->isCharacterDataNode())
-        return static_cast<const CharacterData*>(n)->length();
-    return 1;
-}
-
 bool Position::isRenderedCharacter() const
 {
     if (isNull() || !deprecatedNode()->isTextNode())
@@ -992,8 +981,8 @@ bool Position::rendersInDifferentPosition(const Position &pos) const
     LOG(Editing, "thisRenderedOffset:         %d\n", thisRenderedOffset);
     LOG(Editing, "posRenderer:            %p [%p]\n", posRenderer, b2);
     LOG(Editing, "posRenderedOffset:      %d\n", posRenderedOffset);
-    LOG(Editing, "node min/max:           %d:%d\n", caretMinOffset(deprecatedNode()), caretMaxRenderedOffset(deprecatedNode()));
-    LOG(Editing, "pos node min/max:       %d:%d\n", caretMinOffset(pos.deprecatedNode()), caretMaxRenderedOffset(pos.deprecatedNode()));
+    LOG(Editing, "node min/max:           %d:%d\n", caretMinOffset(deprecatedNode()), caretMaxOffset(deprecatedNode()));
+    LOG(Editing, "pos node min/max:       %d:%d\n", caretMinOffset(pos.deprecatedNode()), caretMaxOffset(pos.deprecatedNode()));
     LOG(Editing, "----------------------------------------------------------------------\n");
 
     if (!b1 || !b2) {
@@ -1005,12 +994,12 @@ bool Position::rendersInDifferentPosition(const Position &pos) const
     }
 
     if (nextRenderedEditable(deprecatedNode()) == pos.deprecatedNode()
-        && thisRenderedOffset == (int)caretMaxRenderedOffset(deprecatedNode()) && !posRenderedOffset) {
+        && thisRenderedOffset == caretMaxOffset(deprecatedNode()) && !posRenderedOffset) {
         return false;
     }
     
     if (previousRenderedEditable(deprecatedNode()) == pos.deprecatedNode()
-        && !thisRenderedOffset && posRenderedOffset == (int)caretMaxRenderedOffset(pos.deprecatedNode())) {
+        && !thisRenderedOffset && posRenderedOffset == caretMaxOffset(pos.deprecatedNode())) {
         return false;
     }
 
index f4f5ec8..891d0fb 100644 (file)
@@ -46,6 +46,7 @@
 #include "HTMLNames.h"
 #include "NodeList.h"
 #include "RenderObject.h"
+#include "RenderText.h"
 #include "SmartReplace.h"
 #include "TextIterator.h"
 #include "htmlediting.h"
@@ -553,11 +554,15 @@ void ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline(Insert
     }
 }
 
+static inline bool nodeHasVisibleRenderText(Text* text)
+{
+    return text->renderer() && toRenderText(text->renderer())->renderedTextLength() > 0;
+}
+
 void ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds()
 {
     document()->updateLayoutIgnorePendingStylesheets();
-    if (!m_lastLeafInserted->renderer()
-        && m_lastLeafInserted->isTextNode()
+    if (m_lastLeafInserted->isTextNode() && !nodeHasVisibleRenderText(static_cast<Text*>(m_lastLeafInserted.get()))
         && !enclosingNodeWithTag(firstPositionInOrBeforeNode(m_lastLeafInserted.get()), selectTag)
         && !enclosingNodeWithTag(firstPositionInOrBeforeNode(m_lastLeafInserted.get()), scriptTag)) {
         if (m_firstNodeInserted == m_lastLeafInserted) {
@@ -573,8 +578,7 @@ void ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds()
     
     // We don't have to make sure that m_firstNodeInserted isn't inside a select or script element, because
     // it is a top level node in the fragment and the user can't insert into those elements.
-    if (!m_firstNodeInserted->renderer() &&
-        m_firstNodeInserted->isTextNode()) {
+    if (m_firstNodeInserted->isTextNode() && !nodeHasVisibleRenderText(static_cast<Text*>(m_firstNodeInserted.get()))) {
         if (m_firstNodeInserted == m_lastLeafInserted) {
             removeNode(m_firstNodeInserted.get());
             m_firstNodeInserted = 0;
@@ -606,8 +610,7 @@ VisiblePosition ReplaceSelectionCommand::positionAtEndOfInsertedContent()
 
 VisiblePosition ReplaceSelectionCommand::positionAtStartOfInsertedContent()
 {
-    // Return the inserted content's first VisiblePosition.
-    return VisiblePosition(nextCandidate(positionInParentBeforeNode(m_firstNodeInserted.get())));
+    return firstPositionInOrBeforeNode(m_firstNodeInserted.get());
 }
 
 static void removeHeadContents(ReplacementFragment& fragment)
index a6d2dda..9cf7925 100644 (file)
@@ -764,7 +764,7 @@ VisiblePosition startOfParagraph(const VisiblePosition& c, EditingBoundaryCrossi
         if (r->isBR() || isBlock(n))
             break;
 
-        if (r->isText() && r->caretMaxRenderedOffset() > 0) {
+        if (r->isText() && toRenderText(r)->renderedTextLength()) {
             ASSERT(n->isTextNode());
             type = Position::PositionIsOffsetInAnchor;
             if (style->preserveNewline()) {
@@ -842,7 +842,7 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing
             break;
 
         // FIXME: We avoid returning a position where the renderer can't accept the caret.
-        if (r->isText() && r->caretMaxRenderedOffset() > 0) {
+        if (r->isText() && toRenderText(r)->renderedTextLength()) {
             ASSERT(n->isTextNode());
             int length = toRenderText(r)->textLength();
             type = Position::PositionIsOffsetInAnchor;
index 8a804b9..2eda402 100644 (file)
@@ -155,11 +155,6 @@ int InlineBox::caretMaxOffset() const
     return m_renderer->caretMaxOffset(); 
 }
 
-unsigned InlineBox::caretMaxRenderedOffset() const 
-{ 
-    return 1; 
-}
-
 void InlineBox::dirtyLineBoxes()
 {
     markDirty();
index e0a6bf6..cf6da56 100644 (file)
@@ -278,7 +278,6 @@ public:
     
     virtual int caretMinOffset() const;
     virtual int caretMaxOffset() const;
-    virtual unsigned caretMaxRenderedOffset() const;
 
     unsigned char bidiLevel() const { return m_bidiEmbeddingLevel; }
     void setBidiLevel(unsigned char level) { m_bidiEmbeddingLevel = level; }
index a22e8d0..73f2b7f 100644 (file)
@@ -1209,11 +1209,6 @@ int InlineTextBox::caretMaxOffset() const
     return m_start + m_len;
 }
 
-unsigned InlineTextBox::caretMaxRenderedOffset() const
-{
-    return m_start + m_len;
-}
-
 float InlineTextBox::textPos() const
 {
     // When computing the width of a text run, RenderBlock::computeInlineDirectionPositionsForLine() doesn't include the actual offset
index 84eb02d..4e87479 100644 (file)
@@ -146,8 +146,6 @@ public:
     virtual int caretMaxOffset() const;
 
 private:
-    virtual unsigned caretMaxRenderedOffset() const;
-
     float textPos() const; // returns the x position relative to the left start of the text line.
 
 public:
index 64e1f72..ca14e45 100644 (file)
@@ -68,11 +68,6 @@ int RenderBR::caretMaxOffset() const
     return 1;
 }
 
-unsigned RenderBR::caretMaxRenderedOffset() const
-{
-    return 1;
-}
-
 VisiblePosition RenderBR::positionForPoint(const LayoutPoint&)
 {
     return createVisiblePosition(0, DOWNSTREAM);
index 0589eee..49f9b5e 100644 (file)
@@ -50,7 +50,6 @@ public:
 
     virtual int caretMinOffset() const;
     virtual int caretMaxOffset() const;
-    virtual unsigned caretMaxRenderedOffset() const;
 
     virtual VisiblePosition positionForPoint(const LayoutPoint&);
 
index 237da51..a45e60d 100644 (file)
@@ -2550,11 +2550,6 @@ int RenderObject::caretMaxOffset() const
     return 0;
 }
 
-unsigned RenderObject::caretMaxRenderedOffset() const
-{
-    return 0;
-}
-
 int RenderObject::previousOffset(int current) const
 {
     return current - 1;
index 6553c40..ecf5227 100644 (file)
@@ -791,7 +791,6 @@ public:
 
     virtual int caretMinOffset() const;
     virtual int caretMaxOffset() const;
-    virtual unsigned caretMaxRenderedOffset() const;
 
     virtual int previousOffset(int current) const;
     virtual int previousOffsetForBackwardDeletion(int current) const;
index e559ee7..87084d1 100644 (file)
@@ -410,11 +410,6 @@ void RenderReplaced::computePreferredLogicalWidths()
     setPreferredLogicalWidthsDirty(false);
 }
 
-unsigned RenderReplaced::caretMaxRenderedOffset() const
-{
-    return 1; 
-}
-
 VisiblePosition RenderReplaced::positionForPoint(const LayoutPoint& point)
 {
     // FIXME: This code is buggy if the replaced element is relative positioned.
index e6dfbc9..3cecb37 100644 (file)
@@ -77,7 +77,6 @@ private:
 
     virtual IntRect clippedOverflowRectForRepaint(RenderBoxModelObject* repaintContainer) const;
 
-    virtual unsigned caretMaxRenderedOffset() const;
     virtual VisiblePosition positionForPoint(const LayoutPoint&);
     
     virtual bool canBeSelectionLeaf() const { return true; }
index 6ecc346..e88861f 100644 (file)
@@ -1595,15 +1595,16 @@ int RenderText::caretMinOffset() const
 int RenderText::caretMaxOffset() const
 {
     InlineTextBox* box = lastTextBox();
-    if (!box)
+    if (!lastTextBox())
         return textLength();
+
     int maxOffset = box->start() + box->len();
     for (box = box->prevTextBox(); box; box = box->prevTextBox())
         maxOffset = max<int>(maxOffset, box->start() + box->len());
     return maxOffset;
 }
 
-unsigned RenderText::caretMaxRenderedOffset() const
+unsigned RenderText::renderedTextLength() const
 {
     int l = 0;
     for (InlineTextBox* box = firstTextBox(); box; box = box->nextTextBox())
index 665bb7c..4020d8a 100644 (file)
@@ -107,7 +107,7 @@ public:
 
     virtual int caretMinOffset() const;
     virtual int caretMaxOffset() const;
-    virtual unsigned caretMaxRenderedOffset() const;
+    virtual unsigned renderedTextLength() const;
 
     virtual int previousOffset(int current) const;
     virtual int previousOffsetForBackwardDeletion(int current) const;