Reproduction steps: 1. Go to http://www.mozilla.org/editor/midasdemo/ and type in the following in HTML source mode: <pre> hello world </pre> 2. Back to WYSIWYG mode and select all Expected result: <blockquote> <pre> hello world </pre> </blockquote> Actual result: <blockquote class="webkit-indent-blockquote" style="margin: 0 0 0 40px; border: none; padding: 0px;"><pre>hello world </pre><pre>world </pre></blockquote><pre>worl<br>d </pre>
This is probably a consequence of https://bugs.webkit.org/show_bug.cgi?id=38231, but I'm not sure it's a regression. Previously, we would crash. I'm impressed that we can put a <br> in the middle of a word.
(In reply to comment #1) > This is probably a consequence of https://bugs.webkit.org/show_bug.cgi?id=38231, but I'm not sure it's a regression. Previously, we would crash. > > I'm impressed that we can put a <br> in the middle of a word. I don't think your patch caused this regression. It's probably mine when I attempted to rewrite IndentOutdentCommand last year. Thanks for the info.
Created attachment 70125 [details] fixes the bug
Attachment 70125 [details] did not build on mac: Build output: http://queues.webkit.org/results/4154118
Created attachment 70134 [details] fixes the bug; fixed release build
Comment on attachment 70134 [details] fixes the bug; fixed release build View in context: https://bugs.webkit.org/attachment.cgi?id=70134&action=review Overall, this looks great. Using explicit start and end positions is a lot better than passing in the end of a paragraph and using prev/next paragraph. r- because of function naming. > WebCore/ChangeLog:22 > + (WebCore::ApplyBlockElementCommand::rangeForParagraphSplitingTextNodesIfNeeded): Added. Nit: Splitting is spelled wrong. > WebCore/editing/ApplyBlockElementCommand.cpp:151 > +static UChar charAtPosition(const Position& position) Do you plan on check for a char other than '\n'? If not, you could simplify the callers by just making this function isNewlineCharAtPosition. > WebCore/editing/ApplyBlockElementCommand.cpp:213 > + // If start is in the middle of a text node, split. Nit: If *end* is . . . > LayoutTests/editing/execCommand/indent-pre-expected.txt:4 > +CONSOLE MESSAGE: line 41: Wrong end node type: [object HTMLBRElement] > +CONSOLE MESSAGE: line 44: Wrong node selected. Is this change intentional? Would extra tests with <pre> and <table> be useful at this point? Maybe it should just be a follow up patch. > LayoutTests/editing/execCommand/indent-pre-expected.txt:49 > +| "list two" > +| <br> > +| "list three" > +| <br> The additional <br>s look weird, but I guess it's harmless.
Created attachment 70163 [details] fixed per tony's comment
(In reply to comment #6) > > WebCore/ChangeLog:22 > > + (WebCore::ApplyBlockElementCommand::rangeForParagraphSplitingTextNodesIfNeeded): Added. > Nit: Splitting is spelled wrong. Oops, fixed. > > WebCore/editing/ApplyBlockElementCommand.cpp:151 > > +static UChar charAtPosition(const Position& position) > > Do you plan on check for a char other than '\n'? If not, you could simplify the callers by just making this function isNewlineCharAtPosition. Good idea. I had the exact function sometime in the past but I had to generalize it to deal with some fix that ended up not being in the final patch. Fixed. > > WebCore/editing/ApplyBlockElementCommand.cpp:213 > > + // If start is in the middle of a text node, split. > > Nit: If *end* is . . . Thanks for noticing that. You can tell my concentration was really low when I added this comment... > > LayoutTests/editing/execCommand/indent-pre-expected.txt:4 > > +CONSOLE MESSAGE: line 41: Wrong end node type: [object HTMLBRElement] > > +CONSOLE MESSAGE: line 44: Wrong node selected. > > Is this change intentional? No but the test claims that we just need not to crash. So I think this is okay. And I think this is related to the following change: > > LayoutTests/editing/execCommand/indent-pre-expected.txt:49 > > +| "list two" > > +| <br> > > +| "list three" > > +| <br> > > The additional <br>s look weird, but I guess it's harmless. Yes. They are visually identical. I believe this is a bug in moveParagraphWithClones for which I know how to fix it. But I'd rather not to add any more fixes into one patch since this is purely cosmetic issue, and new behavior is clearly better than duplicating contents. > Would extra tests with <pre> and <table> be useful at this point? Maybe it should just be a follow up patch. I'm not sure if that'll be useful because we take the same code path. My fix was mainly splitting text nodes and adjusting nodes for moveParagraphWithClones. Table only changes the unsplittable element and I don't think it's relevant to this change. The only reason I added list was to make sure two code paths in IndentOutdentCommand each of which calls moveParagraphWithClones have the test coverage.
Comment on attachment 70163 [details] fixed per tony's comment View in context: https://bugs.webkit.org/attachment.cgi?id=70163&action=review > WebCore/WebCore.xcodeproj/project.pbxproj:-21153 > - developmentRegion = English; Nit: It's probably best to remove this change when committing.
Committed r69354: <http://trac.webkit.org/changeset/69354>
Comment on attachment 70163 [details] fixed per tony's comment View in context: https://bugs.webkit.org/attachment.cgi?id=70163&action=review > WebCore/editing/ApplyBlockElementCommand.cpp:117 > + Position start; > + Position end; Sorry, I just noticed this: You might want to declare these outside the loop so we don't have to run the constructor/destructor on each loop.
http://trac.webkit.org/changeset/69354 might have broken GTK Linux 32-bit Release
Thanks for the review, Tony. I can finally fix the 19795!
(In reply to comment #11) > (From update of attachment 70163 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70163&action=review > > > WebCore/editing/ApplyBlockElementCommand.cpp:117 > > + Position start; > > + Position end; > > Sorry, I just noticed this: > You might want to declare these outside the loop so we don't have to run the constructor/destructor on each loop. Oops, I missed this comment. I'll fix this in my patch for the bug 19795.