Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Nov 2004 17:49:56 +0000 (17:49 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Nov 2004 17:49:56 +0000 (17:49 +0000)
        Fix for these bugs:

        <rdar://problem/3875618> REGRESSION (Mail): Hitting down arrow with full line selected skips line (br case)
        <rdar://problem/3875641> REGRESSION (Mail): Hitting down arrow with full line selected skips line (div case)

        * khtml/editing/selection.cpp:
        (khtml::Selection::modifyMovingRightForward): Fixed by juggling the position as the starting point for
        the next line position when necessary.
        * layout-tests/editing/selection/move-3875618-fix-expected.txt: Added.
        * layout-tests/editing/selection/move-3875618-fix.html: Added.
        * layout-tests/editing/selection/move-3875641-fix-expected.txt: Added.
        * layout-tests/editing/selection/move-3875641-fix.html: Added.

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

LayoutTests/editing/selection/move-3875618-fix-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/move-3875618-fix.html [new file with mode: 0644]
LayoutTests/editing/selection/move-3875641-fix-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/move-3875641-fix.html [new file with mode: 0644]
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/SelectionController.cpp
WebCore/khtml/editing/selection.cpp

diff --git a/LayoutTests/editing/selection/move-3875618-fix-expected.txt b/LayoutTests/editing/selection/move-3875618-fix-expected.txt
new file mode 100644 (file)
index 0000000..ca6f96f
--- /dev/null
@@ -0,0 +1,18 @@
+layer at (0,0) size 800x600
+  RenderCanvas 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 [border: (2px solid #FF0000)]
+      RenderBlock {DIV} at (14,14) size 756x56
+        RenderText {TEXT} at (0,0) size 139x28
+          text run at (0,0) width 139: "There is a tide"
+        RenderBR {BR} at (0,0) size 0x0
+        RenderText {TEXT} at (0,28) size 197x28
+          text run at (0,28) width 197: "in the affairs of men"
+      RenderBlock {DIV} at (14,70) size 756x28
+        RenderText {TEXT} at (0,0) size 241x28
+          text run at (0,0) width 241: "Which taken at the flood"
+selection is CARET:
+start:      position 0 of child 3 {TEXT} of child 1 {DIV} of root {BODY}
+upstream:   position 1 of child 2 {BR} of child 1 {DIV} of root {BODY}
+downstream: position 0 of child 3 {TEXT} of child 1 {DIV} of root {BODY}
diff --git a/LayoutTests/editing/selection/move-3875618-fix.html b/LayoutTests/editing/selection/move-3875618-fix.html
new file mode 100644 (file)
index 0000000..0c76117
--- /dev/null
@@ -0,0 +1,37 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    padding: 12px; 
+    font-size: 24px; 
+}
+.cell { 
+    padding: 12px; 
+    font-size: 24px;
+    height: 48px; 
+}
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    extendSelectionForwardByLineCommand();
+    moveSelectionForwardByLineCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+</head> 
+<body contenteditable id="root" class="editing">
+<div id="test">There is a tide<br>in the affairs of men</div><div>Which taken at the flood</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/editing/selection/move-3875641-fix-expected.txt b/LayoutTests/editing/selection/move-3875641-fix-expected.txt
new file mode 100644 (file)
index 0000000..2c58e42
--- /dev/null
@@ -0,0 +1,18 @@
+layer at (0,0) size 800x600
+  RenderCanvas 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 [border: (2px solid #FF0000)]
+      RenderBlock {DIV} at (14,14) size 756x28
+        RenderText {TEXT} at (0,0) size 139x28
+          text run at (0,0) width 139: "There is a tide"
+      RenderBlock {DIV} at (14,42) size 756x28
+        RenderText {TEXT} at (0,0) size 197x28
+          text run at (0,0) width 197: "in the affairs of men"
+      RenderBlock {DIV} at (14,70) size 756x28
+        RenderText {TEXT} at (0,0) size 241x28
+          text run at (0,0) width 241: "Which taken at the flood"
+selection is CARET:
+start:      position 0 of child 1 {TEXT} of child 2 {DIV} of root {BODY}
+upstream:   position 0 of child 2 {DIV} of root {BODY}
+downstream: position 0 of child 1 {TEXT} of child 2 {DIV} of root {BODY}
diff --git a/LayoutTests/editing/selection/move-3875641-fix.html b/LayoutTests/editing/selection/move-3875641-fix.html
new file mode 100644 (file)
index 0000000..8a5f58c
--- /dev/null
@@ -0,0 +1,37 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    padding: 12px; 
+    font-size: 24px; 
+}
+.cell { 
+    padding: 12px; 
+    font-size: 24px;
+    height: 48px; 
+}
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    extendSelectionForwardByLineCommand();
+    moveSelectionForwardByLineCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+</head> 
+<body contenteditable id="root" class="editing">
+<div id="test">There is a tide</div><div>in the affairs of men</div><div>Which taken at the flood</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
index 6506c28..ee0cbd2 100644 (file)
@@ -2,6 +2,23 @@
 
         Reviewed by John
 
+        Fix for these bugs:
+
+        <rdar://problem/3875618> REGRESSION (Mail): Hitting down arrow with full line selected skips line (br case)
+        <rdar://problem/3875641> REGRESSION (Mail): Hitting down arrow with full line selected skips line (div case)
+
+        * khtml/editing/selection.cpp:
+        (khtml::Selection::modifyMovingRightForward): Fixed by juggling the position as the starting point for
+        the next line position when necessary.
+        * layout-tests/editing/selection/move-3875618-fix-expected.txt: Added.
+        * layout-tests/editing/selection/move-3875618-fix.html: Added.
+        * layout-tests/editing/selection/move-3875641-fix-expected.txt: Added.
+        * layout-tests/editing/selection/move-3875641-fix.html: Added.
+
+2004-11-11  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+
         Improved some function names, at John's urging. No changes to the
         functions themselves.
         
index a62bf61..bb66705 100644 (file)
@@ -306,9 +306,28 @@ VisiblePosition Selection::modifyMovingRightForward(ETextGranularity granularity
         case PARAGRAPH:
             pos = nextParagraphPosition(VisiblePosition(m_end), m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
             break;
-        case LINE:
-            pos = nextLinePosition(VisiblePosition(m_end), m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
+        case LINE: {
+            // This somewhat complicated code is needed to handle the case where there is a
+            // whole line selected (like when the user clicks at the start of a line and hits shift+down-arrow),
+            // and then hits an (unshifted) down arrow. Since the whole-line selection considers its
+            // ending point to be the start of the next line, it may be necessary to juggle the 
+            // position to use as the VisiblePosition to pass to nextLinePosition(). If this juggling
+            // is not done, you can wind up skipping a line. See these two bugs for more information:
+            // <rdar://problem/3875618> REGRESSION (Mail): Hitting down arrow with full line selected skips line (br case)
+            // <rdar://problem/3875641> REGRESSION (Mail): Hitting down arrow with full line selected skips line (div case)
+            if (isCaret()) {
+                pos = VisiblePosition(m_end);
+            }
+            else if (isRange()) {
+                Position p(m_end.upstream());
+                if (p.node()->id() == ID_BR)
+                    pos = VisiblePosition(Position(p.node(), 0));
+                else
+                    pos = VisiblePosition(p);
+            }
+            pos = nextLinePosition(pos, m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
             break;
+        }
         case LINE_BOUNDARY:
             pos = VisiblePosition(selectionForLine(m_end, m_affinity).end());
             break;
index a62bf61..bb66705 100644 (file)
@@ -306,9 +306,28 @@ VisiblePosition Selection::modifyMovingRightForward(ETextGranularity granularity
         case PARAGRAPH:
             pos = nextParagraphPosition(VisiblePosition(m_end), m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
             break;
-        case LINE:
-            pos = nextLinePosition(VisiblePosition(m_end), m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
+        case LINE: {
+            // This somewhat complicated code is needed to handle the case where there is a
+            // whole line selected (like when the user clicks at the start of a line and hits shift+down-arrow),
+            // and then hits an (unshifted) down arrow. Since the whole-line selection considers its
+            // ending point to be the start of the next line, it may be necessary to juggle the 
+            // position to use as the VisiblePosition to pass to nextLinePosition(). If this juggling
+            // is not done, you can wind up skipping a line. See these two bugs for more information:
+            // <rdar://problem/3875618> REGRESSION (Mail): Hitting down arrow with full line selected skips line (br case)
+            // <rdar://problem/3875641> REGRESSION (Mail): Hitting down arrow with full line selected skips line (div case)
+            if (isCaret()) {
+                pos = VisiblePosition(m_end);
+            }
+            else if (isRange()) {
+                Position p(m_end.upstream());
+                if (p.node()->id() == ID_BR)
+                    pos = VisiblePosition(Position(p.node(), 0));
+                else
+                    pos = VisiblePosition(p);
+            }
+            pos = nextLinePosition(pos, m_affinity, xPosForVerticalArrowNavigation(END, isRange()));
             break;
+        }
         case LINE_BOUNDARY:
             pos = VisiblePosition(selectionForLine(m_end, m_affinity).end());
             break;