Reproduction steps: 1. Open ayoutTests/editing/deleting/delete-4038408-fix.html 2. Insert a paragraph between quoted lines "Here is some reply text" and "It should have the reply text style" New line is inserted below the blockquote :( Is this a regression? It also reproduces on Safari 5.1
I'm not sure if I understand the question. Do expected results show pass or fail? If it's fail, did someone change them lately? I see that you converted it to a dumpAsMarkup test this fall. The test is complicated, so I don't immediately see if it's supposed to work in browser.
(In reply to comment #1) > I'm not sure if I understand the question. Do expected results show pass or fail? If it's fail, did someone change them lately? I see that you converted it to a dumpAsMarkup test this fall. The test still passes. I was working on some other bug and happened to try inserting line breaks between quoted lines after the test had ran. (i.e. it's not the test itself that's broken).
Created attachment 134857 [details] First try
Comment on attachment 134857 [details] First try View in context: https://bugs.webkit.org/attachment.cgi?id=134857&action=review > Source/WebCore/ChangeLog:4 > + Inserting a paragraph between quoted lines in editing/deleting/delete-4038408-fix.html doesn't work Bug title should appear before the bug url. > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:232 > // We can get here if we pasted a copied portion of a blockquote with a newline at the end and are trying to paste it > // into an unquoted area. We then don't want the newline within the blockquote or else it will also be quoted. > - if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > - startBlock = static_cast<Element*>(highestBlockquote); > + if (m_pasteBlockqutoeIntoUnquotedArea) > + if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > + startBlock = static_cast<Element*>(highestBlockquote); This looks like a hack, and we're adding more hack on top of it. Can we adjust position in ReplaceSelectionCommand to avoid this hack altogether?
Thanks for the review. I was thinking to adjust position in ReplaceSectionCommand also. Will try it on my next patch. (In reply to comment #4) > (From update of attachment 134857 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134857&action=review > > > Source/WebCore/ChangeLog:4 > > + Inserting a paragraph between quoted lines in editing/deleting/delete-4038408-fix.html doesn't work > > Bug title should appear before the bug url. > > > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:232 > > // We can get here if we pasted a copied portion of a blockquote with a newline at the end and are trying to paste it > > // into an unquoted area. We then don't want the newline within the blockquote or else it will also be quoted. > > - if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > > - startBlock = static_cast<Element*>(highestBlockquote); > > + if (m_pasteBlockqutoeIntoUnquotedArea) > > + if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > > + startBlock = static_cast<Element*>(highestBlockquote); > > This looks like a hack, and we're adding more hack on top of it. Can we adjust position in ReplaceSelectionCommand to avoid this hack altogether?
Comment on attachment 134857 [details] First try Attachment 134857 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12262448 New failing tests: editing/inserting/insert-paragraph-separator-in-blockquote.html
Created attachment 134881 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 135138 [details] fix changelog & test failure on chromium In order to adjust position in ReplaceSelectionCommand and make it accessible by InsertParagraphSeparatorCommand, we have to call setEndingSelection() in ReplaceSelectionCommand, then InsertParagraphSeparatorCommand can retrieve the new position by calling the endingSelection(). However, the position we pass to setEndingSelection() would be canonicalize, so the insertion position that can be seen by InsertParagraphSeparatorCommand may NOT be the desired one. For example, we try to ask InsertParagraphSeparatorCommand to insert a paragraph separator in [Div, 1], <div><blockquote>abc</blockquote>^<div>, after calling the setEndingSelection(), the insertion position changed to [Text, 3], <div><blockquote>abc^</blockquote><div>. AS a result, the paragraph separator is insert into a unexpected place. As a workaround, the new patch still use an extra parameter to provide the missing information for the InsertParagraphSeparatorCommand. Let me know if you have better idea to do it. Thanks!
Comment on attachment 135138 [details] fix changelog & test failure on chromium View in context: https://bugs.webkit.org/attachment.cgi?id=135138&action=review Okay :( Sounds like we need this hack. > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:232 > + if (m_pasteBlockqutoeIntoUnquotedArea) > + if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > + startBlock = static_cast<Element*>(highestBlockquote); You need the curly brackets around the outer if. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1131 > } else > // Use a default paragraph element (a plain div) for the empty paragraph, using the last paragraph > // block's style seems to annoy users. > - insertParagraphSeparator(true); > + insertParagraphSeparator(true, (!startIsInsideMailBlockquote && insertMailBlockquote)); > We need curly brackets around this statement and comments since they span more than one line. > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:30 > + var ev = document.createEvent("KeyboardEvent"); Please don't use abbreviations like ev. > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:56 > +if ((nodesOfTopDiv == topDiv.childNodes.length) && (blockquoteElement.childNodes.length == (nodesOfBlockquote + 1))) > + document.body.innerHTML = "SUCCESS"; > +else > + document.body.innerHTML = "FAIL"; I don't think this is a good check. We may, in the future, generate slightly different markup and this test will break. I think it's better to use dump-as-markup.js.
Thanks for the review. Will update the patch with your comments. (In reply to comment #9) > (From update of attachment 135138 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135138&action=review > > Okay :( Sounds like we need this hack. > > > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:232 > > + if (m_pasteBlockqutoeIntoUnquotedArea) > > + if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > > + startBlock = static_cast<Element*>(highestBlockquote); > > You need the curly brackets around the outer if. > > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1131 > > } else > > // Use a default paragraph element (a plain div) for the empty paragraph, using the last paragraph > > // block's style seems to annoy users. > > - insertParagraphSeparator(true); > > + insertParagraphSeparator(true, (!startIsInsideMailBlockquote && insertMailBlockquote)); > > > > We need curly brackets around this statement and comments since they span more than one line. > > > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:30 > > + var ev = document.createEvent("KeyboardEvent"); > > Please don't use abbreviations like ev. > > > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:56 > > +if ((nodesOfTopDiv == topDiv.childNodes.length) && (blockquoteElement.childNodes.length == (nodesOfBlockquote + 1))) > > + document.body.innerHTML = "SUCCESS"; > > +else > > + document.body.innerHTML = "FAIL"; > > I don't think this is a good check. We may, in the future, generate slightly different markup and this test will break. I think it's better to use dump-as-markup.js.
Created attachment 135161 [details] updated patch based on Niwa's review
Comment on attachment 135161 [details] updated patch based on Niwa's review View in context: https://bugs.webkit.org/attachment.cgi?id=135161&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1007 > + bool insertMailBlockquote = isMailBlockquote(refNode.get()); What if blockquote had a wrapping element (e.g. div)? > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1130 > + insertParagraphSeparator(true, (!startIsInsideMailBlockquote && insertMailBlockquote)); The outer parenthesis is unnecessary.
Comment on attachment 135161 [details] updated patch based on Niwa's review View in context: https://bugs.webkit.org/attachment.cgi?id=135161&action=review > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote-expected.txt:2 > +You should see a <br> tag is between 'First Line' and 'Second Line'. Nit: s/ is//.
I think someone familiar with Apple's Mail should verify that this patch doesn't break its editor. In particular, I'm concerned about the logic in Source/WebCore/editing/ReplaceSelectionCommand.cpp:1007. Adele, Enrica, Justin, could you do that for us?
Created attachment 135364 [details] updated patch with a new test
(In reply to comment #12) > (From update of attachment 135161 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135161&action=review > > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1007 > > + bool insertMailBlockquote = isMailBlockquote(refNode.get()); > > What if blockquote had a wrapping element (e.g. div)? Good catch! Fix it and add a new test for it. > > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1130 > > + insertParagraphSeparator(true, (!startIsInsideMailBlockquote && insertMailBlockquote)); > > The outer parenthesis is unnecessary.
Comment on attachment 135364 [details] updated patch with a new test View in context: https://bugs.webkit.org/attachment.cgi?id=135364&action=review > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:230 > // We can get here if we pasted a copied portion of a blockquote with a newline at the end and are trying to paste it > // into an unquoted area. We then don't want the newline within the blockquote or else it will also be quoted. > - if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > - startBlock = static_cast<Element*>(highestBlockquote); > + if (m_pasteBlockqutoeIntoUnquotedArea) { Alternatively, can we check is we're at the end of blockquote or not? This trick should only be used when we're inserting a new paragraph at the end of a blockquote, right? Can we check whether there's a content between canonicalPos and the end of the mail blockblock? And adjust startBlock only if we don't have any contents there? > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:16 > +<title></title> No need for title. > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:48 > +pressKey("\n"); Can't we just do document.execCommand('InsertParagraph', false, null) ? > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote-expected.txt:1 > +This test ensures the copied the newline is NOT inside the blockquote. We should describe what we should expect to see below. > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:1 > +<html> No DOCTYPE? > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:10 > + <style> > + blockquote { > + color: blue; > + border-left: 2px solid blue; > + margin: 0px; > + padding: 0 0 0 20px; > + } > + </style> No need to indent style element and rules like this. > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:19 > + var range = document.createRange(); No need to indent script contents.
Thanks for reviewing :) Please see my comments below, (In reply to comment #17) > (From update of attachment 135364 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135364&action=review > > > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:230 > > // We can get here if we pasted a copied portion of a blockquote with a newline at the end and are trying to paste it > > // into an unquoted area. We then don't want the newline within the blockquote or else it will also be quoted. > > - if (Node* highestBlockquote = highestEnclosingNodeOfType(canonicalPos, &isMailBlockquote)) > > - startBlock = static_cast<Element*>(highestBlockquote); > > + if (m_pasteBlockqutoeIntoUnquotedArea) { > > Alternatively, can we check is we're at the end of blockquote or not? > This trick should only be used when we're inserting a new paragraph at the end of a blockquote, right? > Can we check whether there's a content between canonicalPos and the end of the mail blockblock? > And adjust startBlock only if we don't have any contents there? > it may cause a regression I think. Assume the caret is at the end of the blockquote, then user hits the enter key to insert a new paragraph. The new paragraph would be inserted out of the blockquote since there is no contents between the insertion position and the end of the blockquote, right? This behavior seems incorrect to me -- user can't insert a new paragraph in blockquote by using the enter key. I believe this trick should only be used when we try to paste a copied portion of a blockquote with a newline at the end into an unquoted area. > > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:16 > > +<title></title> > > No need for title. Will remove it > > > LayoutTests/editing/inserting/insert-paragraph-separator-in-blockquote.html:48 > > +pressKey("\n"); > > Can't we just do document.execCommand('InsertParagraph', false, null) ? Will fix it. > > > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote-expected.txt:1 > > +This test ensures the copied the newline is NOT inside the blockquote. > > We should describe what we should expect to see below. > > > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:1 > > +<html> > > No DOCTYPE? > Will fix > > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:10 > > + <style> > > + blockquote { > > + color: blue; > > + border-left: 2px solid blue; > > + margin: 0px; > > + padding: 0 0 0 20px; > > + } > > + </style> > > No need to indent style element and rules like this. > Will fix > > LayoutTests/editing/pasteboard/paste-wrapped-blockquote-into-nonblockquote.html:19 > > + var range = document.createRange(); > > No need to indent script contents. Will fix
Created attachment 138566 [details] updated patch based on Niwa's review
Comment on attachment 138566 [details] updated patch based on Niwa's review Okay, r=me. I'm still sad that we have to add a new flag for this but the patch seems landable for the lack of better options.
Thanks for the review :] (In reply to comment #20) > (From update of attachment 138566 [details]) > Okay, r=me. I'm still sad that we have to add a new flag for this but the patch seems landable for the lack of better options.
Comment on attachment 138566 [details] updated patch based on Niwa's review Clearing flags on attachment: 138566 Committed r115628: <http://trac.webkit.org/changeset/115628>
All reviewed patches have been landed. Closing bug.