Bug 47233 – REGRESSION: Indenting pre duplicates content

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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>

[Tony Chang]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 70125 [details]
fixes the bug

[Eric Seidel (no email)]

Attachment 70125 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4154118

[Ryosuke Niwa]

Created attachment 70134 [details]
fixes the bug; fixed release build

[Tony Chang]

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.

[Ryosuke Niwa]

Created attachment 70163 [details]
fixed per tony's comment

[Ryosuke Niwa]

(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.

[Tony Chang]

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.

[Ryosuke Niwa]

Committed r69354: <http://trac.webkit.org/changeset/69354>

[Tony Chang]

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.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69354 might have broken GTK Linux 32-bit Release

[Ryosuke Niwa]

Thanks for the review, Tony.  I can finally fix the 19795!

[Ryosuke Niwa]

(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.