Bug 122748 - [contenteditable] Content after non-editable element disappears when merging lines using backspace
Summary: [contenteditable] Content after non-editable element disappears when merging ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-14 05:12 PDT by Piotrek Koszuliński (Reinmar)
Modified: 2014-02-05 13:35 PST (History)
8 users (show)

See Also:


Attachments
Testcase (521 bytes, text/html)
2013-10-21 13:03 PDT, Santosh Mahto
no flags Details
Patch (5.61 KB, patch)
2013-10-21 13:29 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2013-10-23 09:31 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch for Landing (5.48 KB, patch)
2013-10-24 10:32 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotrek Koszuliński (Reinmar) 2013-10-14 05:12:54 PDT
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
Comment 1 Santosh Mahto 2013-10-21 13:03:35 PDT
Created attachment 214767 [details]
Testcase
Comment 2 Santosh Mahto 2013-10-21 13:29:00 PDT
Created attachment 214771 [details]
Patch
Comment 3 webkit 2013-10-22 00:24:03 PDT
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!
Comment 4 Ryosuke Niwa 2013-10-22 17:53:09 PDT
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?
Comment 5 Santosh Mahto 2013-10-23 09:31:55 PDT
Created attachment 214971 [details]
Patch
Comment 6 Ryosuke Niwa 2013-10-23 10:57:53 PDT
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.
Comment 7 Santosh Mahto 2013-10-23 12:20:42 PDT
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
Comment 8 Ryosuke Niwa 2013-10-23 13:00:05 PDT
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.
Comment 9 Santosh Mahto 2013-10-23 22:12:29 PDT
(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
Comment 10 Santosh Mahto 2013-10-24 10:32:13 PDT
Created attachment 215079 [details]
Patch for Landing
Comment 11 Darin Adler 2013-10-24 12:14:17 PDT
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.
Comment 12 WebKit Commit Bot 2013-10-24 12:16:49 PDT
Comment on attachment 215079 [details]
Patch for Landing

Clearing flags on attachment: 215079

Committed r157944: <http://trac.webkit.org/changeset/157944>
Comment 13 WebKit Commit Bot 2013-10-24 12:16:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Brynjar Gretarsson 2014-02-05 13:35:43 PST
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?