LayoutTests:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Mar 2007 04:22:25 +0000 (04:22 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Mar 2007 04:22:25 +0000 (04:22 +0000)
        Reviewed by john

        <rdar://problem/5062376>
        REGRESSION: In Mail and Gmail, can't change alignment to text after it has been applied

        * editing/execCommand/5062376-expected.checksum: Added.
        * editing/execCommand/5062376-expected.png: Added.
        * editing/execCommand/5062376-expected.txt: Added.
        * editing/execCommand/5062376.html: Added.

WebCore:

        Reviewed by john

        <rdar://problem/5062376>
        REGRESSION: In Mail and Gmail, can't change alignment to text after it has been applied

        Bring back the remove step in applyBlockStyle.  It's
        necessary because addBlockStyleIfNeeded assumes that
        the properties it adds aren't already on the block that
        it adds them to.

        * editing/ApplyStyleCommand.cpp:
        (WebCore::ApplyStyleCommand::applyBlockStyle):
        Bring back the remove step (added a testcase).
        Don't do the add step if m_removeOnly is true (no testcase
        because there aren't any clients using removeOnly functionality
        to remove styles yet, only styled elemets).
        Moved the code for creating new blocks up one level
        to this function so that we can pass blocks to removeCSSStyle.
        When converting VisiblePositions to indices and vice versa,
        use the highest node in the shadow tree if we're in one as
        the scope (working on a testcase).
        (WebCore::ApplyStyleCommand::addBlockStyle): Moved code to
        applyBlockStyle.
        * editing/ApplyStyleCommand.h:

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/5062376-expected.checksum [new file with mode: 0644]
LayoutTests/editing/execCommand/5062376-expected.png [new file with mode: 0644]
LayoutTests/editing/execCommand/5062376-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5062376.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/ApplyStyleCommand.cpp
WebCore/editing/ApplyStyleCommand.h

index e22446a..36ac285 100644 (file)
@@ -1,3 +1,15 @@
+2007-03-15  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by john
+        
+        <rdar://problem/5062376> 
+        REGRESSION: In Mail and Gmail, can't change alignment to text after it has been applied
+
+        * editing/execCommand/5062376-expected.checksum: Added.
+        * editing/execCommand/5062376-expected.png: Added.
+        * editing/execCommand/5062376-expected.txt: Added.
+        * editing/execCommand/5062376.html: Added.
+
 2007-03-15  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Adele
diff --git a/LayoutTests/editing/execCommand/5062376-expected.checksum b/LayoutTests/editing/execCommand/5062376-expected.checksum
new file mode 100644 (file)
index 0000000..7f3cf74
--- /dev/null
@@ -0,0 +1 @@
+17e81a1109b61e34da5978dbcbe15cc9
\ No newline at end of file
diff --git a/LayoutTests/editing/execCommand/5062376-expected.png b/LayoutTests/editing/execCommand/5062376-expected.png
new file mode 100644 (file)
index 0000000..46ba1a7
Binary files /dev/null and b/LayoutTests/editing/execCommand/5062376-expected.png differ
diff --git a/LayoutTests/editing/execCommand/5062376-expected.txt b/LayoutTests/editing/execCommand/5062376-expected.txt
new file mode 100644 (file)
index 0000000..2f7ec61
--- /dev/null
@@ -0,0 +1,15 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {P} at (0,0) size 784x36
+        RenderText {#text} at (0,0) size 783x36
+          text run at (0,0) width 602: "This tests for a bug where centering text would prevent it from being right or left aligned again. "
+          text run at (602,0) width 181: "The paragraph below should"
+          text run at (0,18) width 102: "be right aligned."
+      RenderBlock {DIV} at (0,52) size 784x18
+        RenderText {#text} at (763,0) size 21x18
+          text run at (763,0) width 21: "foo"
+selection start: position 0 of child 0 {#text} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
+selection end:   position 3 of child 0 {#text} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/execCommand/5062376.html b/LayoutTests/editing/execCommand/5062376.html
new file mode 100644 (file)
index 0000000..a9a9fc8
--- /dev/null
@@ -0,0 +1,9 @@
+<p>This tests for a bug where centering text would prevent it from being right or left aligned again.  The paragraph below should be right aligned.</p>
+<div id="div" contenteditable="true">foo</div>
+
+<script>
+var div = document.getElementById("div");
+div.focus();
+document.execCommand("JustifyCenter");
+document.execCommand("JustifyRight");
+</script>
index 18b4e5a..9a65e9a 100644 (file)
@@ -1,3 +1,30 @@
+2007-03-15  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by john
+        
+        <rdar://problem/5062376> 
+        REGRESSION: In Mail and Gmail, can't change alignment to text after it has been applied
+        
+        Bring back the remove step in applyBlockStyle.  It's 
+        necessary because addBlockStyleIfNeeded assumes that 
+        the properties it adds aren't already on the block that 
+        it adds them to.
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::applyBlockStyle): 
+        Bring back the remove step (added a testcase).
+        Don't do the add step if m_removeOnly is true (no testcase
+        because there aren't any clients using removeOnly functionality
+        to remove styles yet, only styled elemets).
+        Moved the code for creating new blocks up one level
+        to this function so that we can pass blocks to removeCSSStyle.
+        When converting VisiblePositions to indices and vice versa,
+        use the highest node in the shadow tree if we're in one as
+        the scope (working on a testcase).
+        (WebCore::ApplyStyleCommand::addBlockStyle): Moved code to
+        applyBlockStyle.
+        * editing/ApplyStyleCommand.h: 
+
 2007-03-15  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Maciej
index aa1c7f3..9f55c1d 100644 (file)
@@ -377,8 +377,11 @@ void ApplyStyleCommand::applyBlockStyle(CSSMutableStyleDeclaration *style)
     VisiblePosition visibleEnd(end);
     // Save and restore the selection endpoints using their indices in the document, since
     // addBlockStyleIfNeeded may moveParagraphs, which can remove these endpoints.
-    RefPtr<Range> startRange = new Range(document(), Position(document(), 0), visibleStart.deepEquivalent());
-    RefPtr<Range> endRange = new Range(document(), Position(document(), 0), visibleEnd.deepEquivalent());
+    // Calculate start and end indices from the start of the tree that they're in.
+    Node* scope = highestAncestor(visibleStart.deepEquivalent().node());
+    Position rangeStart(scope, 0);
+    RefPtr<Range> startRange = new Range(document(), rangeStart, visibleStart.deepEquivalent());
+    RefPtr<Range> endRange = new Range(document(), rangeStart, visibleEnd.deepEquivalent());
     int startIndex = TextIterator::rangeLength(startRange.get());
     int endIndex = TextIterator::rangeLength(endRange.get());
     
@@ -386,14 +389,25 @@ void ApplyStyleCommand::applyBlockStyle(CSSMutableStyleDeclaration *style)
     VisiblePosition nextParagraphStart(endOfParagraph(paragraphStart).next());
     VisiblePosition beyondEnd(endOfParagraph(visibleEnd).next());
     while (paragraphStart.isNotNull() && paragraphStart != beyondEnd) {
-        addBlockStyleIfNeeded(style, paragraphStart);
+        StyleChange styleChange(style, paragraphStart.deepEquivalent(), StyleChange::styleModeForParseMode(document()->inCompatMode()));
+        if (styleChange.cssStyle().length() > 0 || m_removeOnly) {
+            Node* block = enclosingBlock(paragraphStart.deepEquivalent().node());
+            Node* newBlock = moveParagraphContentsToNewBlockIfNecessary(paragraphStart.deepEquivalent());
+            if (newBlock)
+                block = newBlock;
+            ASSERT(block->isHTMLElement());
+            if (block->isHTMLElement()) {
+                removeCSSStyle(style, static_cast<HTMLElement*>(block));
+                if (!m_removeOnly)
+                    addBlockStyle(styleChange, static_cast<HTMLElement*>(block));
+            }
+        }
         paragraphStart = nextParagraphStart;
         nextParagraphStart = endOfParagraph(paragraphStart).next();
     }
     
-    setEndingSelection(Selection(TextIterator::rangeFromLocationAndLength(document()->documentElement(), startIndex, 0)->startPosition(), 
-                                 TextIterator::rangeFromLocationAndLength(document()->documentElement(), endIndex, 0)->startPosition(), 
-                                 DOWNSTREAM));
+    updateStartEnd(TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), startIndex, 0)->startPosition(),
+                   TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), endIndex, 0)->startPosition());
 }
 
 #define NoFontDelta (0.0f)
@@ -1198,37 +1212,18 @@ void ApplyStyleCommand::surroundNodeRangeWithElement(Node *startNode, Node *endN
     }
 }
 
-void ApplyStyleCommand::addBlockStyleIfNeeded(CSSMutableStyleDeclaration* style, const VisiblePosition& paragraphStart)
+void ApplyStyleCommand::addBlockStyle(const StyleChange& styleChange, HTMLElement* block)
 {
     // Do not check for legacy styles here. Those styles, like <B> and <I>, only apply for
     // inline content.
-    if (paragraphStart.isNull())
-        return;
-    
-    Node* block = enclosingBlock(paragraphStart.deepEquivalent().node());
     if (!block)
         return;
         
-    StyleChange styleChange(style, Position(block, 0), StyleChange::styleModeForParseMode(document()->inCompatMode()));
-    if (styleChange.cssStyle().length() == 0)
-        return;
-
-    // If a new block was necessary, this function uses moveParagraphs to move content into
-    // the new block, which invalidates block.
-    Node* newBlock = moveParagraphContentsToNewBlockIfNecessary(paragraphStart.deepEquivalent());
-    
-    if (newBlock)
-        block = newBlock;
-        
-    ASSERT(block->isHTMLElement());
-    if (!block->isHTMLElement())
-        return;
-        
     String cssText = styleChange.cssStyle();
-    CSSMutableStyleDeclaration* decl = static_cast<HTMLElement*>(block)->inlineStyleDecl();
+    CSSMutableStyleDeclaration* decl = block->inlineStyleDecl();
     if (decl)
         cssText += decl->cssText();
-    setNodeAttribute(static_cast<Element*>(block), styleAttr, cssText);
+    setNodeAttribute(block, styleAttr, cssText);
 }
 
 void ApplyStyleCommand::addInlineStyleIfNeeded(CSSMutableStyleDeclaration *style, Node *startNode, Node *endNode)
index 4b83645..eafec59 100644 (file)
@@ -31,6 +31,7 @@
 namespace WebCore {
 
 class HTMLElement;
+class StyleChange;
 
 class ApplyStyleCommand : public CompositeEditCommand {
 public:
@@ -65,7 +66,7 @@ private:
     void applyBlockStyle(CSSMutableStyleDeclaration*);
     void applyRelativeFontStyleChange(CSSMutableStyleDeclaration*);
     void applyInlineStyle(CSSMutableStyleDeclaration*);
-    void addBlockStyleIfNeeded(CSSMutableStyleDeclaration*, const VisiblePosition&);
+    void addBlockStyle(const StyleChange&, HTMLElement*);
     void addInlineStyleIfNeeded(CSSMutableStyleDeclaration*, Node* start, Node* end);
     bool splitTextAtStartIfNeeded(const Position& start, const Position& end);
     bool splitTextAtEndIfNeeded(const Position& start, const Position& end);