Reviewed by Harrison
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Mar 2005 22:32:15 +0000 (22:32 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Mar 2005 22:32:15 +0000 (22:32 +0000)
        Fix for this bug:

        <rdar://problem/4032543> REGRESSION (Mail): Mail hangs when quoted text is pasted twice

        This code change fixes the bug in a non-obvious way. The root cause of the problem was
        that a VisiblePosition created using an affinity originating in Mail code caused
        two VisiblePosition objects that should have been equal to differ only in their
        affinities, which in turn caused us to run a code path that should not have run.

        * khtml/editing/visible_position.cpp:
        (khtml::VisiblePosition::VisiblePosition): Added copy constructor.
        (khtml::VisiblePosition::next): Factored out inline code that used to be here into new
        setAffinityUsingLinePosition() function.
        (khtml::isEqualIgnoringAffinity): New helper to handle cases when affinity in equality check does
        not matter. However, we want to know about such cases where a VisiblePosition differs only by affinity,
        and the code will assert in development when this happens.
        (khtml::isNotEqualIgnoringAffinity): Ditto, but not. :)
        (khtml::setAffinityUsingLinePosition): New helper function mentioned above. This will "correct"
        upstream affinity to downstream if the affinity does not make a difference for the position.
        * khtml/editing/visible_position.h:
        * khtml/editing/visible_range.h: Wacky bug. The operator== for this class took VisiblePosition classes!
        * khtml/editing/visible_units.cpp:
        (khtml::isStartOfParagraph): Now performs equality check without regard to affinity.
        (khtml::isEndOfParagraph): Ditto.
        (khtml::isStartOfBlock): Ditto.
        (khtml::isEndOfBlock): Ditto.
        * kwq/WebCoreBridge.mm:
        (-[WebCoreBridge setSelectedDOMRange:affinity:]): Adjusts the affinity using setAffinityUsingLinePosition()
        if necessary.

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

WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/visible_position.cpp
WebCore/khtml/editing/visible_position.h
WebCore/khtml/editing/visible_range.h
WebCore/khtml/editing/visible_units.cpp
WebCore/kwq/WebCoreBridge.mm

index b5c966ecdf32ea06b2d3034c2e079ebe5c7b4f81..511d2e0ce7039b2c37196c827e66dfbc33865eab 100644 (file)
@@ -1,3 +1,37 @@
+2005-03-04  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by Harrison
+
+        Fix for this bug:
+        
+        <rdar://problem/4032543> REGRESSION (Mail): Mail hangs when quoted text is pasted twice
+        
+        This code change fixes the bug in a non-obvious way. The root cause of the problem was
+        that a VisiblePosition created using an affinity originating in Mail code caused 
+        two VisiblePosition objects that should have been equal to differ only in their 
+        affinities, which in turn caused us to run a code path that should not have run.
+
+        * khtml/editing/visible_position.cpp:
+        (khtml::VisiblePosition::VisiblePosition): Added copy constructor.
+        (khtml::VisiblePosition::next): Factored out inline code that used to be here into new
+        setAffinityUsingLinePosition() function.
+        (khtml::isEqualIgnoringAffinity): New helper to handle cases when affinity in equality check does
+        not matter. However, we want to know about such cases where a VisiblePosition differs only by affinity, 
+        and the code will assert in development when this happens.
+        (khtml::isNotEqualIgnoringAffinity): Ditto, but not. :)
+        (khtml::setAffinityUsingLinePosition): New helper function mentioned above. This will "correct"
+        upstream affinity to downstream if the affinity does not make a difference for the position.
+        * khtml/editing/visible_position.h:
+        * khtml/editing/visible_range.h: Wacky bug. The operator== for this class took VisiblePosition classes!
+        * khtml/editing/visible_units.cpp:
+        (khtml::isStartOfParagraph): Now performs equality check without regard to affinity.
+        (khtml::isEndOfParagraph): Ditto.
+        (khtml::isStartOfBlock): Ditto.
+        (khtml::isEndOfBlock): Ditto.
+        * kwq/WebCoreBridge.mm:
+        (-[WebCoreBridge setSelectedDOMRange:affinity:]): Adjusts the affinity using setAffinityUsingLinePosition()
+        if necessary.
+
 2005-03-04  Darin Adler  <darin@apple.com>
 
         Reviewed by John.
index d281c1e63c1fe8bb2c6f9d488a8682cf50090772..41e7b946665bb61d4bc93816b73f8f9c98c24d4b 100644 (file)
@@ -62,6 +62,12 @@ VisiblePosition::VisiblePosition(NodeImpl *node, long offset, EAffinity affinity
     init(Position(node, offset), initHint, affinity);
 }
 
+VisiblePosition::VisiblePosition(const VisiblePosition &other)
+{
+    m_deepPosition = other.m_deepPosition;
+    m_affinity = other.m_affinity;
+}
+
 void VisiblePosition::init(const Position &pos, EInitHint initHint, EAffinity affinity)
 {
     m_affinity = affinity;
@@ -137,17 +143,7 @@ void VisiblePosition::initDownstream(const Position &pos)
 VisiblePosition VisiblePosition::next() const
 {
     VisiblePosition result = VisiblePosition(nextVisiblePosition(m_deepPosition), m_affinity);
-
-    // When moving across line wrap, make sure to end up with DOWNSTREAM affinity (i.e. at first
-    // character on next line).  Correct behavior regardless of whether the current position is
-    // at or after the last character on the current line.
-    if (result.isNotNull() && result.affinity() == UPSTREAM) {
-        VisiblePosition temp = result;
-        temp.setAffinity(DOWNSTREAM);
-        if (!visiblePositionsOnDifferentLines(temp, result))
-            result.setAffinity(DOWNSTREAM);
-    }
-
+    setAffinityUsingLinePosition(result);
     return result;
 }
 
@@ -412,6 +408,24 @@ QChar VisiblePosition::character() const
     return textNode->data()[offset];
 }
 
+bool isEqualIgnoringAffinity(const VisiblePosition &a, const VisiblePosition &b)
+{
+    bool result = a.m_deepPosition == b.m_deepPosition;
+    if (result) {
+        // We want to catch cases where positions are equal, but affinities are not, since
+        // this is very likely a bug, given the places where this call is used. The difference
+        // is very likely due to code that set the affinity on a VisiblePosition "by hand" and 
+        // did so incorrectly.
+        ASSERT(a.m_affinity == b.m_affinity);
+    }
+    return result;
+}
+
+bool isNotEqualIgnoringAffinity(const VisiblePosition &a, const VisiblePosition &b)
+{
+    return !isEqualIgnoringAffinity(a, b);
+}
+
 void VisiblePosition::debugPosition(const char *msg) const
 {
     if (isNull())
@@ -466,6 +480,17 @@ bool setEnd(Range &r, const VisiblePosition &c)
     return code == 0;
 }
 
+void setAffinityUsingLinePosition(VisiblePosition &pos)
+{
+    // When not moving across line wrap, make sure to end up with DOWNSTREAM affinity.  
+    if (pos.isNotNull() && pos.affinity() == UPSTREAM) {
+        VisiblePosition temp(pos);
+        temp.setAffinity(DOWNSTREAM);
+        if (!visiblePositionsOnDifferentLines(temp, pos))
+            pos.setAffinity(DOWNSTREAM);
+    }
+}
+
 bool visiblePositionsOnDifferentLines(const VisiblePosition &pos1, const VisiblePosition &pos2)
 {
     if (pos1.isNull() || pos2.isNull())
index ea52578d7f729b04d04dd6eaf8dc421ceaf9db54..63b65ab80d4e23a5c5be4f36d25d3b66b37f3fca 100644 (file)
@@ -54,7 +54,8 @@ public:
 
     VisiblePosition() { m_affinity = VP_DEFAULT_AFFINITY; };
     VisiblePosition(NodeImpl *, long offset, EAffinity, EInitHint initHint=INIT_DOWN);
-    explicit VisiblePosition(const Position &, EAffinity, EInitHint initHint=INIT_DOWN);
+    VisiblePosition(const Position &, EAffinity, EInitHint initHint=INIT_DOWN);
+    VisiblePosition(const VisiblePosition &);
 
     void clear() { m_deepPosition.clear(); }
 
@@ -69,6 +70,10 @@ public:
     Position downstreamDeepEquivalent() const;
 
     friend bool operator==(const VisiblePosition &a, const VisiblePosition &b);
+    friend bool operator!=(const VisiblePosition &a, const VisiblePosition &b);
+
+    friend bool isEqualIgnoringAffinity(const VisiblePosition &a, const VisiblePosition &b);
+    friend bool isNotEqualIgnoringAffinity(const VisiblePosition &a, const VisiblePosition &b);
 
     // next() and previous() will increment/decrement by a character cluster.
     VisiblePosition next() const;
@@ -105,7 +110,7 @@ private:
     static bool atEnd(const Position &);
 
     static bool isCandidate(const Position &);
-    
+        
     Position m_deepPosition;
     EAffinity m_affinity;
 };
@@ -114,7 +119,7 @@ inline bool operator==(const VisiblePosition &a, const VisiblePosition &b)
 {
     return a.m_deepPosition == b.m_deepPosition && a.m_affinity == b.m_affinity;
 }
-
 inline bool operator!=(const VisiblePosition &a, const VisiblePosition &b)
 {
     return !(a == b);
@@ -130,6 +135,8 @@ VisiblePosition startVisiblePosition(const DOM::RangeImpl *, EAffinity);
 VisiblePosition endVisiblePosition(const DOM::Range &, EAffinity);
 VisiblePosition endVisiblePosition(const DOM::RangeImpl *, EAffinity);
 
+void setAffinityUsingLinePosition(VisiblePosition &);
+
 bool visiblePositionsOnDifferentLines(const VisiblePosition &, const VisiblePosition &);
 bool visiblePositionsInDifferentBlocks(const VisiblePosition &, const VisiblePosition &);
 bool isFirstVisiblePositionOnLine(const VisiblePosition &);
index 51dcd4a1a959e13a5ae317368e79d756a1649581..f5956678384b56413dc3fd83649399de4d02bab6 100644 (file)
@@ -59,7 +59,7 @@ public:
     bool isNull() const { return m_start.isNull(); }
     bool isCollapsed() const { return m_start == m_end; }
 
-    friend inline bool operator==(const VisiblePosition &a, const VisiblePosition &b);
+    friend inline bool operator==(const VisibleRange &a, const VisibleRange &b);
 
 private:
     VisiblePosition m_start;
index f55d408a410dd7e76539dacef20a2b70f0717453..286370efecf803e425aff60af8517b87df8bf85f 100644 (file)
@@ -641,12 +641,12 @@ bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b)
 
 bool isStartOfParagraph(const VisiblePosition &pos)
 {
-    return pos.isNotNull() && pos == startOfParagraph(pos);
+    return pos.isNotNull() && isEqualIgnoringAffinity(pos, startOfParagraph(pos));
 }
 
 bool isEndOfParagraph(const VisiblePosition &pos)
 {
-    return pos.isNotNull() && pos == endOfParagraph(pos, DoNotIncludeLineBreak);
+    return pos.isNotNull() && isEqualIgnoringAffinity(pos, endOfParagraph(pos, DoNotIncludeLineBreak));
 }
 
 VisiblePosition previousParagraphPosition(const VisiblePosition &p, int x)
@@ -736,12 +736,12 @@ bool inSameBlock(const VisiblePosition &a, const VisiblePosition &b)
 
 bool isStartOfBlock(const VisiblePosition &pos)
 {
-    return pos.isNotNull() && pos == startOfBlock(pos);
+    return pos.isNotNull() && isEqualIgnoringAffinity(pos, startOfBlock(pos));
 }
 
 bool isEndOfBlock(const VisiblePosition &pos)
 {
-    return pos.isNotNull() && pos == endOfBlock(pos, DoNotIncludeLineBreak);
+    return pos.isNotNull() && isEqualIgnoringAffinity(pos, endOfBlock(pos, DoNotIncludeLineBreak));
 }
 
 // ---------
index b6b87bf0261830f18fddcd8eaa5fdaa26b1f99c3..cd6c5dce5dcfe01a790aec1a7b1c80d425bd919d 100644 (file)
@@ -109,6 +109,7 @@ using khtml::ChildrenOnly;
 using khtml::createMarkup;
 using khtml::Decoder;
 using khtml::DeleteSelectionCommand;
+using khtml::DOWNSTREAM;
 using khtml::EAffinity;
 using khtml::EditAction;
 using khtml::EditCommandPtr;
@@ -124,6 +125,7 @@ using khtml::RenderStyle;
 using khtml::RenderWidget;
 using khtml::ReplaceSelectionCommand;
 using khtml::Selection;
+using khtml::setAffinityUsingLinePosition;
 using khtml::Tokenizer;
 using khtml::TypingCommand;
 using khtml::UPSTREAM;
@@ -1570,11 +1572,21 @@ static HTMLFormElementImpl *formElementFromDOMElement(DOMElement *element)
 
     EAffinity affinity = static_cast<EAffinity>(selectionAffinity);
 
+    bool rangeCollapsed = [range collapsed];
+    if (!rangeCollapsed)
+        affinity = DOWNSTREAM;
+    
     // Work around bug where isRenderedContent returns false for <br> elements at the ends of lines.
     // If that bug wasn't an issue, we could just make the position from the range directly.
     Position start(startContainer, [range startOffset]);
     Position end(endContainer, [range endOffset]);
-    start = VisiblePosition(start, affinity, khtml::VisiblePosition::INIT_UP).deepEquivalent();
+    VisiblePosition visibleStart(start, affinity, khtml::VisiblePosition::INIT_UP);
+    start = visibleStart.deepEquivalent();
+
+    if (rangeCollapsed) {
+        setAffinityUsingLinePosition(visibleStart);
+        affinity = visibleStart.affinity();
+    }
 
     // FIXME: Can we provide extentAffinity?
     Selection selection(start, affinity, end, khtml::SEL_DEFAULT_AFFINITY);