Bug 122748 – [contenteditable] Content after non-editable element disappears when merging lines using backspace

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

[Piotrek Koszuliński (Reinmar)]

Steps to reproduce the problem:
1. Open test_backspace.html sample.
2. Place caret at the beginning of the second line.
3. Press backspace

What is the expected behavior?
Second paragraph is merged into the first one.

What went wrong?
Non-editable span (orange border) and content after it disappeared.

This is pretty critical issue for CKEditor (http://dev.ckeditor.com/ticket/10989), because it makes inline non-editable elements very unstable.

Crossposted: https://code.google.com/p/chromium/issues/detail?id=306970

[Santosh Mahto]

Created attachment 214767 [details]
Testcase

[Santosh Mahto]

Created attachment 214771 [details]
Patch

A one liner fix to kill this very important issue. I see Santosh here: :D
http://devopsreactions.tumblr.com/post/63360879672/fixing-a-huge-bug-with-a-single-line-of-code

Seriously, I really hope we gonna have this patch reviewed soon! Thanks!

[Ryosuke Niwa]

Comment on attachment 214771 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        contain contenteditable=false element then after merging

Nit: contain*s*. Also need a comma after "element"

> Source/WebCore/ChangeLog:10
> +        non-editable and afterwards content are lost and only content

I don't get what you mean here. Perhaps something along this line?
"then content after the non-editable element will be removed while the content before the element will be merged with the first paragraph"

> Source/WebCore/ChangeLog:12
> +        happens becasue endOfParagraphToMove calculation in merging function doesnot

Nit: either "doesn't" or "does not".

> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:2
> + Expected behavior is on hitting backspace all three word (First,second and third) should be visible and merged at the end of first line.

Why do we have a space at the beginning of this line?
Also, we need a space between "First," and "second" and "first" should not be capitalized.

> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:8
> +|   "You should not lose "second Third" from second line when both paragraphs will be merged."

Why is only Third capitalized?

[Santosh Mahto]

Created attachment 214971 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 214971 [details]
Patch

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

> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:10
> +| <p>
> +|   "You should not lose "Second Third" from second line when both paragraph will be merged."
> +| "
> +        "

Does this paragraph need to be here?
It's cluttering the results.

[Santosh Mahto]

Comment on attachment 214971 [details]
Patch

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

>> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:10
>> +        "
> 
> Does this paragraph need to be here?
> It's cluttering the results.

But we need to show both paragraph to clearly see how 2nd paragraph got merged with first paragraph.
I mean after merging we can see what part was of first line and what was of second line

[Ryosuke Niwa]

Comment on attachment 214971 [details]
Patch

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

>>> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:10
>>> +        "
>> 
>> Does this paragraph need to be here?
>> It's cluttering the results.
> 
> But we need to show both paragraph to clearly see how 2nd paragraph got merged with first paragraph.
> I mean after merging we can see what part was of first line and what was of second line

Can't we put "First paragraph" instead?  This text is really long.
We should make the test results look as simple as possible.

[Santosh Mahto]

(In reply to comment #8)
> (From update of attachment 214971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214971&action=review
> 
> >>> LayoutTests/editing/deleting/merge-paragraph-contatining-noneditable-expected.txt:10
> >>> +        "
> >> 
> >> Does this paragraph need to be here?
> >> It's cluttering the results.
> > 
> > But we need to show both paragraph to clearly see how 2nd paragraph got merged with first paragraph.
> > I mean after merging we can see what part was of first line and what was of second line
> 
> Can't we put "First paragraph" instead?  This text is really long.
> We should make the test results look as simple as possible.

Hmm... I thought you meant <p> tag is cluttering.
Sure I  I will change the text.Thanks

[Santosh Mahto]

Created attachment 215079 [details]
Patch for Landing

[Darin Adler]

Comment on attachment 215079 [details]
Patch for Landing

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

> LayoutTests/ChangeLog:13
> +        * editing/deleting/merge-paragraph-contatining-noneditable-expected.txt: Added.
> +        * editing/deleting/merge-paragraph-contatining-noneditable.html: Added.

Just noticed that this says contatining rather than containing.

[WebKit Commit Bot]

Comment on attachment 215079 [details]
Patch for Landing

Clearing flags on attachment: 215079

Committed r157944: <http://trac.webkit.org/changeset/157944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brynjar Gretarsson]

After this fix there is still an issue. Once the two paragraphs have been merged, the non-editable element becomes editable, i.e. it loses the contenteditable="false" attribute. Any chance this bug can be re-opened and fixed so that the non-editable element stays non-editable?