Reviewed by Kevin.
authormjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 May 2005 04:03:34 +0000 (04:03 +0000)
committermjs <mjs@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 10 May 2005 04:03:34 +0000 (04:03 +0000)
- remove isFirstVisiblePositionInBlock and isLastVisiblePositionInBlock, in favor of isStartOfBlock and isEndOfBlock

It turned out that both isEndOfBlock and isLastVisiblePositionInBlock had (different) bugs,
and there was code relying on the bugs of each. So in addition I fixed isEndOfBlock and fixed
the parts of the code relying on buggy behavior.

I also removed the includeEndOfLine parameter to endOfBlock since no one used it and it's not
clear if it would ever be useful.

        * khtml/editing/htmlediting.cpp:
        (khtml::InsertLineBreakCommand::doApply): Use new calls.
        (khtml::InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion): Don't gratuitously make
an UPSTREAM VisiblePosition, as this will cause trouble comparing it to end of block.
        (khtml::InsertParagraphSeparatorCommand::doApply): Use new calls.
        (khtml::ReplaceSelectionCommand::doApply): Use new calls. Also, don't make a position <BR,0> and test
if it is the end of a block, that can never be true, although the buggy code in
isLastVisiblePositionInBlock would say it is. Make <BR,1> instead.
        * khtml/editing/markup.cpp:
        (khtml::createMarkup): Instead of checking isEndOfBlock on the start position, check if the start's
next is in a different block, to avoid relying on the buggy old isEndOfBlock behavior.
        * khtml/editing/visible_position.cpp:
        (khtml::isFirstVisiblePositionInParagraph): Use isStartOfBlock.
        (khtml::isLastVisiblePositionInParagraph): Use isEndOfBlock.
        * khtml/editing/visible_position.h:
        * khtml/editing/visible_units.cpp:
        (khtml::endOfBlock): Greatly simplify, and no longer consider the start of a descendant
block to be the end of the block. That's inconsistent with how startOfBlock works. Also
remove include end of line parameter.
        (khtml::isEndOfBlock): Don't pass unneeded parameter.
        * khtml/editing/visible_units.h:

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

WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/editing/markup.cpp
WebCore/khtml/editing/visible_position.cpp
WebCore/khtml/editing/visible_position.h
WebCore/khtml/editing/visible_units.cpp
WebCore/khtml/editing/visible_units.h

index 9cb61542b3ea97c102ff0c05bf60b9531f2e539f..b5a7ff527744a827f920c256d60e9b9eb8ffc7ab 100644 (file)
@@ -1,3 +1,38 @@
+2005-05-09  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Kevin.
+
+       - remove isFirstVisiblePositionInBlock and isLastVisiblePositionInBlock, in favor of isStartOfBlock and isEndOfBlock
+
+       It turned out that both isEndOfBlock and isLastVisiblePositionInBlock had (different) bugs,
+       and there was code relying on the bugs of each. So in addition I fixed isEndOfBlock and fixed 
+       the parts of the code relying on buggy behavior.
+
+       I also removed the includeEndOfLine parameter to endOfBlock since no one used it and it's not
+       clear if it would ever be useful.
+       
+        * khtml/editing/htmlediting.cpp:
+        (khtml::InsertLineBreakCommand::doApply): Use new calls.
+        (khtml::InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion): Don't gratuitously make
+       an UPSTREAM VisiblePosition, as this will cause trouble comparing it to end of block.
+        (khtml::InsertParagraphSeparatorCommand::doApply): Use new calls.
+        (khtml::ReplaceSelectionCommand::doApply): Use new calls. Also, don't make a position <BR,0> and test
+       if it is the end of a block, that can never be true, although the buggy code in 
+       isLastVisiblePositionInBlock would say it is. Make <BR,1> instead.
+        * khtml/editing/markup.cpp:
+        (khtml::createMarkup): Instead of checking isEndOfBlock on the start position, check if the start's
+       next is in a different block, to avoid relying on the buggy old isEndOfBlock behavior.
+        * khtml/editing/visible_position.cpp:
+        (khtml::isFirstVisiblePositionInParagraph): Use isStartOfBlock.
+        (khtml::isLastVisiblePositionInParagraph): Use isEndOfBlock.
+        * khtml/editing/visible_position.h:
+        * khtml/editing/visible_units.cpp:
+        (khtml::endOfBlock): Greatly simplify, and no longer consider the start of a descendant
+       block to be the end of the block. That's inconsistent with how startOfBlock works. Also
+       remove include end of line parameter.
+        (khtml::isEndOfBlock): Don't pass unneeded parameter.
+        * khtml/editing/visible_units.h:
+
 2005-05-09  Adele Peterson  <adele@apple.com>
 
         fix for <rdar://problem/4110775> Crash will occur when double-clicking outerHTML link on W3 DOM test
index 41845be21ac5c7d1e22e598082325e6f6ada08c8..1fa5b2ef6eb09128e43c5b7710b63c7a3b256307 100644 (file)
@@ -3286,7 +3286,7 @@ void InsertLineBreakCommand::doApply()
 
     bool atStart = pos.offset() <= pos.node()->caretMinOffset();
     bool atEnd = pos.offset() >= pos.node()->caretMaxOffset();
-    bool atEndOfBlock = isLastVisiblePositionInBlock(VisiblePosition(pos, selection.startAffinity()));
+    bool atEndOfBlock = isEndOfBlock(VisiblePosition(pos, selection.startAffinity()));
     
     if (atEndOfBlock) {
         LOG(Editing, "input newline case 1");
@@ -3454,7 +3454,7 @@ void InsertParagraphSeparatorCommand::calculateStyleBeforeInsertion(const Positi
     // It is only important to set a style to apply later if we're at the boundaries of
     // a paragraph. Otherwise, content that is moved as part of the work of the command
     // will lend their styles to the new paragraph without any extra work needed.
-    VisiblePosition visiblePos(pos, UPSTREAM);
+    VisiblePosition visiblePos(pos, VP_DEFAULT_AFFINITY);
     if (!isFirstVisiblePositionInParagraph(visiblePos) && !isLastVisiblePositionInParagraph(visiblePos))
         return;
     
@@ -3507,8 +3507,8 @@ void InsertParagraphSeparatorCommand::doApply()
         return;
 
     VisiblePosition visiblePos(pos, affinity);
-    bool isFirstInBlock = isFirstVisiblePositionInBlock(visiblePos);
-    bool isLastInBlock = isLastVisiblePositionInBlock(visiblePos);
+    bool isFirstInBlock = isStartOfBlock(visiblePos);
+    bool isLastInBlock = isEndOfBlock(visiblePos);
     bool startBlockIsRoot = startBlock == startBlock->rootEditableElement();
 
     // This is the block that is going to be inserted.
@@ -4791,8 +4791,8 @@ void ReplaceSelectionCommand::doApply()
 
     VisiblePosition visibleStart(selection.start(), selection.startAffinity());
     VisiblePosition visibleEnd(selection.end(), selection.endAffinity());
-    bool startAtStartOfBlock = isFirstVisiblePositionInBlock(visibleStart);
-    bool startAtEndOfBlock = isLastVisiblePositionInBlock(visibleStart);
+    bool startAtStartOfBlock = isStartOfBlock(visibleStart);
+    bool startAtEndOfBlock = isEndOfBlock(visibleStart);
     bool startAtBlockBoundary = startAtStartOfBlock || startAtEndOfBlock;
     NodeImpl *startBlock = selection.start().node()->enclosingBlockFlowElement();
     NodeImpl *endBlock = selection.end().node()->enclosingBlockFlowElement();
@@ -4962,9 +4962,9 @@ void ReplaceSelectionCommand::doApply()
         NodeImpl *insertionBlock = insertionPos.node()->enclosingBlockFlowElement();
         bool insertionBlockIsRoot = insertionBlock == insertionBlock->rootEditableElement();
         VisiblePosition visiblePos(insertionPos, DOWNSTREAM);
-        if (!insertionBlockIsRoot && isProbablyBlock(refNode) && isFirstVisiblePositionInBlock(visiblePos))
+        if (!insertionBlockIsRoot && isProbablyBlock(refNode) && isStartOfBlock(visiblePos))
             insertNodeBeforeAndUpdateNodesInserted(refNode, insertionBlock);
-        else if (!insertionBlockIsRoot && isProbablyBlock(refNode) && isLastVisiblePositionInBlock(visiblePos)) {
+        else if (!insertionBlockIsRoot && isProbablyBlock(refNode) && isEndOfBlock(visiblePos)) {
             insertNodeAfterAndUpdateNodesInserted(refNode, insertionBlock);
         } else if (mergeStart && !isProbablyBlock(refNode)) {
             Position pos = visiblePos.next().deepEquivalent().downstream();
@@ -5056,8 +5056,8 @@ void ReplaceSelectionCommand::doApply()
     else {
         if (m_lastNodeInserted && m_lastNodeInserted->id() == ID_BR && !document()->inStrictMode()) {
             document()->updateLayout();
-            VisiblePosition pos(Position(m_lastNodeInserted, 0), DOWNSTREAM);
-            if (isLastVisiblePositionInBlock(pos)) {
+            VisiblePosition pos(Position(m_lastNodeInserted, 1), DOWNSTREAM);
+            if (isEndOfBlock(pos)) {
                 NodeImpl *next = m_lastNodeInserted->traverseNextNode();
                 bool hasTrailingBR = next && next->id() == ID_BR && m_lastNodeInserted->enclosingBlockFlowElement() == next->enclosingBlockFlowElement();
                 if (!hasTrailingBR) {
index 9553d23b54aab8e046f6cf86eb77c3655c900579..7e6b248573d4ffd93e203357fbfb733f2b3c3349 100644 (file)
@@ -333,7 +333,7 @@ QString createMarkup(const RangeImpl *range, QPtrList<NodeImpl> *nodes, EAnnotat
     NodeImpl *startNode = range->startNode();
     VisiblePosition visibleStart(range->startPosition(), VP_DEFAULT_AFFINITY);
     VisiblePosition visibleEnd(range->endPosition(), VP_DEFAULT_AFFINITY);
-    if (isEndOfBlock(visibleStart)) {
+    if (!inSameBlock(visibleStart, visibleStart.next())) {
         if (visibleStart == visibleEnd.previous())
             return interchangeNewlineString;
         markups.append(interchangeNewlineString);
index 2ab359164940bfb9e5ff4d2f612d3711d09b9845..68635da563cea707b2dbdaf2d1b93797ef9f4b6b 100644 (file)
@@ -24,6 +24,7 @@
  */
 
 #include "visible_position.h"
+#include "visible_units.h"
 
 #include "misc/htmltags.h"
 #include "rendering/render_line.h"
@@ -537,16 +538,7 @@ bool isFirstVisiblePositionInParagraph(const VisiblePosition &pos)
     if (pos.isNull())
         return false;
 
-    return pos.deepEquivalent().upstream().node()->id() == ID_BR || isFirstVisiblePositionInBlock(pos);
-}
-
-bool isFirstVisiblePositionInBlock(const VisiblePosition &pos)
-{
-    if (pos.isNull())
-        return false;
-
-    Position upstream = pos.deepEquivalent().upstream();
-    return upstream.node()->isBlockFlow() && upstream.offset() == 0;
+    return pos.deepEquivalent().upstream().node()->id() == ID_BR || isStartOfBlock(pos);
 }
 
 bool isFirstVisiblePositionInNode(const VisiblePosition &pos, const NodeImpl *node)
@@ -572,30 +564,7 @@ bool isLastVisiblePositionInParagraph(const VisiblePosition &pos)
     if (pos.isNull())
         return false;
 
-    return pos.deepEquivalent().downstream().node()->id() == ID_BR || isLastVisiblePositionInBlock(pos);
-}
-
-bool isLastVisiblePositionInBlock(const VisiblePosition &pos)
-{
-    if (pos.isNull())
-        return false;
-        
-    VisiblePosition next = pos.next();
-    if (next.isNull())
-        return true;
-    
-    switch (blockRelationship(pos, next)) {
-        case NoBlockRelationship:
-        case SameBlockRelationship:
-        case AncestorBlockRelationship:
-            return false;
-        case OtherBlockRelationship:
-        case PeerBlockRelationship:
-        case DescendantBlockRelationship:
-            return true;
-    }
-    ASSERT_NOT_REACHED();
-    return false;
+    return pos.deepEquivalent().downstream().node()->id() == ID_BR || isEndOfBlock(pos);
 }
 
 bool isLastVisiblePositionInNode(const VisiblePosition &pos, const NodeImpl *node)
index 3abfebb7bef0e4ac7bb40de18be7ff75bcbe435f..d6ba961481557ded0937753a31f5dd2c6c2e7312 100644 (file)
@@ -137,11 +137,9 @@ bool visiblePositionsOnDifferentLines(const VisiblePosition &, const VisiblePosi
 bool visiblePositionsInDifferentBlocks(const VisiblePosition &, const VisiblePosition &);
 bool isFirstVisiblePositionOnLine(const VisiblePosition &);
 bool isFirstVisiblePositionInParagraph(const VisiblePosition &);
-bool isFirstVisiblePositionInBlock(const VisiblePosition &);
 bool isFirstVisiblePositionInNode(const VisiblePosition &, const DOM::NodeImpl *);
 bool isLastVisiblePositionOnLine(const VisiblePosition &);
 bool isLastVisiblePositionInParagraph(const VisiblePosition &);
-bool isLastVisiblePositionInBlock(const VisiblePosition &);
 bool isLastVisiblePositionInNode(const VisiblePosition &, const DOM::NodeImpl *);
 
 } // namespace khtml
index da29fc6630849f79c1969b735d40bc8d894d6b55..dffa5499c375e24bf0f7433ceb01d05a5ae21148 100644 (file)
@@ -697,7 +697,7 @@ VisiblePosition startOfBlock(const VisiblePosition &c)
 }
 
 // written, but not yet tested
-VisiblePosition endOfBlock(const VisiblePosition &c, EIncludeLineBreak includeLineBreak)
+VisiblePosition endOfBlock(const VisiblePosition &c)
 {
     Position p = c.deepEquivalent();
 
@@ -706,37 +706,8 @@ VisiblePosition endOfBlock(const VisiblePosition &c, EIncludeLineBreak includeLi
         return VisiblePosition();
 
     NodeImpl *startBlock = startNode->enclosingBlockFlowElement();
-    NodeImpl *stayInsideBlock = includeLineBreak ? 0 : startBlock;
     
-    NodeImpl *node = startNode;
-    long offset = p.offset();
-
-    for (NodeImpl *n = startNode; n; n = n->traverseNextNode(stayInsideBlock)) {
-        RenderObject *r = n->renderer();
-        if (!r)
-            continue;
-        RenderStyle *style = r->style();
-        if (style->visibility() != VISIBLE)
-            continue;
-        if (r->isBlockFlow()) {
-            if (includeLineBreak)
-                return VisiblePosition(n, 0, DOWNSTREAM);
-            break;
-        }
-        if (r->isText()) {
-            if (includeLineBreak && !n->isAncestor(startBlock))
-                return VisiblePosition(n, 0, DOWNSTREAM);
-            node = n;
-            offset = static_cast<RenderText *>(r)->length();
-        } else if (r->isReplaced()) {
-            node = n;
-            offset = 1;
-            if (includeLineBreak && !n->isAncestor(startBlock))
-                break;
-        }
-    }
-
-    return VisiblePosition(node, offset, DOWNSTREAM);
+    return VisiblePosition(startBlock, startBlock->childNodeCount(), VP_DEFAULT_AFFINITY);   
 }
 
 bool inSameBlock(const VisiblePosition &a, const VisiblePosition &b)
@@ -751,7 +722,7 @@ bool isStartOfBlock(const VisiblePosition &pos)
 
 bool isEndOfBlock(const VisiblePosition &pos)
 {
-    return pos.isNotNull() && isEqualIgnoringAffinity(pos, endOfBlock(pos, DoNotIncludeLineBreak));
+    return pos.isNotNull() && isEqualIgnoringAffinity(pos, endOfBlock(pos));
 }
 
 // ---------
index d3161ae7e505367a864fff0a039ae38877654776..9f133d50518d50368477da0855542eff2ddaaebb 100644 (file)
@@ -67,7 +67,7 @@ bool isEndOfParagraph(const VisiblePosition &);
 
 // blocks (true paragraphs; line break elements don't break blocks)
 VisiblePosition startOfBlock(const VisiblePosition &);
-VisiblePosition endOfBlock(const VisiblePosition &, EIncludeLineBreak = DoNotIncludeLineBreak);
+VisiblePosition endOfBlock(const VisiblePosition &);
 bool inSameBlock(const VisiblePosition &, const VisiblePosition &);
 bool isStartOfBlock(const VisiblePosition &);
 bool isEndOfBlock(const VisiblePosition &);