Bug 113007 – Unable to insert a paragraph in between some text whose previous sibling is a non-editable block.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Arpita Bahuguna]

Created attachment 194449 [details]
Testpage

Fetch the attached testpage and try to insert a line break in between the second line of text, i.e. in between "Trytoinsertlinebreakinmiddleofthisline".

The text is not broken as expected and a new paragraph is also not inserted.

This causes an assert in comparePositions() function in htmlediting.cpp
(ASSERT(commonScope);)

[Arpita Bahuguna]

Created attachment 194464 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 194464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194464&action=review

> Source/WebCore/ChangeLog:15
> +        improper position computation.

What kind of improper position?

> Source/WebCore/ChangeLog:26
> +        If the current node's renderer doesn't exist, it's next
> +        sibling should be sought.

Why?

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:6
> +<div contenteditable="false">1</div>Trytoinsertlinebreakinmiddleofthisline</div>

Can we use CamelCase in "Trytoinsertlinebreakinmiddleofthisline"?

[Arpita Bahuguna]

(In reply to comment #2)
> (From update of attachment 194464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194464&action=review
> 
Hi Ryosuke, thanks for the review!

> > Source/WebCore/ChangeLog:15
> > +        improper position computation.
> 
> What kind of improper position?
> 
Got it! Apologize for my careless interpretation. Uploading another patch.

> > Source/WebCore/ChangeLog:26
> > +        If the current node's renderer doesn't exist, it's next
> > +        sibling should be sought.
> 
> Why?
> 
> > LayoutTests/editing/inserting/insert-paragraph-between-text.html:6
> > +<div contenteditable="false">1</div>Trytoinsertlinebreakinmiddleofthisline</div>
> 
> Can we use CamelCase in "Trytoinsertlinebreakinmiddleofthisline"?
Will do.

[Arpita Bahuguna]

Created attachment 194489 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 194489 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194489&action=review

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:375
> +                visiblePos = positionBeforeNode(n);

I would have preferred to declare another local variable rather than re-using visiblePos.
As confusingly named as visiblePos is, visiblePos is supposed to be VisiblePosition of the insertion point.

> LayoutTests/editing/inserting/insert-paragraph-between-text-expected.txt:1
> +Testcase for bug <https://bugs.webkit.org/show_bug.cgi?id=113007> 113007: Unable to insert a paragraph in between some text whose previous sibling is a non-editable block.

We can use shorthand URL https://webkit.org/b/113007. Also, We probably don't have both bug URL and the bug id.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:12
> +var sel = window.getSelection();

Please don't use abbreviations like sel. Spell out the whole word.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:13
> +sel.setPosition(test, 3);

More standardized way to do this is to use collapse instead. setPosition is a WebKit extension.
Instead of saying 3, maybe we should just use test.childNodes.length?

[Arpita Bahuguna]

Created attachment 194800 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #5)
> (From update of attachment 194489 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194489&action=review
> 
Hi rniwa! Many thanks for the r+ !

> > Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:375
> > +                visiblePos = positionBeforeNode(n);
> 
> I would have preferred to declare another local variable rather than re-using visiblePos.
> As confusingly named as visiblePos is, visiblePos is supposed to be VisiblePosition of the insertion point.
> 
Have used another local variable: posBeforeNode (for want of a better and more appropriate alternative).

> > LayoutTests/editing/inserting/insert-paragraph-between-text-expected.txt:1
> > +Testcase for bug <https://bugs.webkit.org/show_bug.cgi?id=113007> 113007: Unable to insert a paragraph in between some text whose previous sibling is a non-editable block.
> 
> We can use shorthand URL https://webkit.org/b/113007. Also, We probably don't have both bug URL and the bug id.
> 
Done

> > LayoutTests/editing/inserting/insert-paragraph-between-text.html:12
> > +var sel = window.getSelection();
> 
> Please don't use abbreviations like sel. Spell out the whole word.
> 
> > LayoutTests/editing/inserting/insert-paragraph-between-text.html:13
> > +sel.setPosition(test, 3);
> 
> More standardized way to do this is to use collapse instead. setPosition is a WebKit extension.
> Instead of saying 3, maybe we should just use test.childNodes.length?
Done

[Ryosuke Niwa]

Comment on attachment 194800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194800&action=review

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:374
> +            VisiblePosition posBeforeNode;

Please spell out position instead of using abbreviations like "pos". I know the existing code uses it but we're trying to remove that usage.

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:376
> +                posBeforeNode = positionBeforeNode(n);

Can't we declare VisiblePosition here?

> Source/WebCore/editing/InsertParagraphSeparatorCommand.cpp:378
> +                if (!posBeforeNode.isNull()) {
> +                    if (comparePositions(VisiblePosition(insertionPosition), posBeforeNode) <= 0)

These two if-statements could be combined.

> LayoutTests/editing/inserting/insert-paragraph-between-text.html:18
> +Markup.description('Testcase for bug https://webkit.org/b/113007: Unable to insert a paragraph in between some text whose previous sibling is a non-editable block.\n'+
> +'The test has passed if three lines are displayed instead of two, with the last line consisting of the letter "e".');

I would prefer seeing this description before the test code so that I can read it before reading the test code.

[Arpita Bahuguna]

Created attachment 195027 [details]
Patch

[Build Bot]

Comment on attachment 195027 [details]
Patch

Attachment 195027 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17209936

[WebKit Review Bot]

Comment on attachment 195027 [details]
Patch

Clearing flags on attachment: 195027

Committed r146867: <http://trac.webkit.org/changeset/146867>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Arpita Bahuguna]

The win bot failure is due to: fast/text/custom-font-data-crash.html -> crashed

It seems to be unrelated to this fix (which is specifically for inserting a paragraph in some editable content).

Also, win-bot failure results for other patches such as for #113233 [http://webkit-commit-queue.appspot.com/results/17310196] and #113275 [http://webkit-commit-queue.appspot.com/results/17246524] show the same crash failure.