Bug 33950 - contentEditable with "position:relative" paragraphs is buggy
Summary: contentEditable with "position:relative" paragraphs is buggy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-21 06:47 PST by Luke Plant
Modified: 2013-10-10 23:17 PDT (History)
6 users (show)

See Also:


Attachments
HTML page showing the problem (334 bytes, text/html)
2011-03-17 17:15 PDT, Luke Plant
no flags Details
work in progress (1.19 KB, patch)
2013-10-05 10:00 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2013-10-07 00:49 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (474.85 KB, application/zip)
2013-10-07 01:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (481.44 KB, application/zip)
2013-10-07 01:55 PDT, Build Bot
no flags Details
Patch (4.38 KB, patch)
2013-10-07 02:18 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2013-10-08 07:44 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (482.93 KB, application/zip)
2013-10-08 08:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (505.94 KB, application/zip)
2013-10-08 08:56 PDT, Build Bot
no flags Details
Patch (21.19 KB, patch)
2013-10-08 09:22 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (591.38 KB, application/zip)
2013-10-08 10:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (570.08 KB, application/zip)
2013-10-08 11:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (588.54 KB, application/zip)
2013-10-08 11:32 PDT, Build Bot
no flags Details
Patch (21.19 KB, patch)
2013-10-09 09:00 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (577.85 KB, application/zip)
2013-10-09 09:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (867.79 KB, application/zip)
2013-10-09 10:30 PDT, Build Bot
no flags Details
Patch (15.11 KB, patch)
2013-10-09 11:00 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (25.73 KB, patch)
2013-10-10 10:47 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2013-10-10 13:05 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 Luke Plant 2010-01-21 06:47:51 PST
With "position:relative" set for paragraphs, editing content using contentEditable=true produces weird behaviour and bugs, and even crashes sometimes.

The simplest test case is something like this: In a paragraph, type [Enter] 1 [Enter] 2 [Enter] 3 [Enter] 4 [Up] [Up] [Backspace]

The result is that the two paragraphs after the caret are switched and merged, with a stray <br> appearing.

A test page is here: http://lukeplant.me.uk/webkitbug.html

I found this bug in a very recent Chrome 4 on Linux, and Safari 4 on Windows, which is why I'm filing here, since I presume this must be a WebKit bug.  I'm still building the most recent WebKit to test it out.  The bug was not present in Safari 3.1, I think.  Opera and Firefox handle this fine.

By the way, the reason for wanting to put "position:relative" on paragraphs like this is to develop a text editor that uses ':before' to be able to indicate things like any class that is applied to the text.  See http://bitbucket.org/spookylukey/semanticeditor/wiki/Home for screen shot.  position:relative is needed to establish a containing box for the ":before" element, so that it can be relatively positioned.  Without this, it is difficult to put additional information inside the editing box.
Comment 1 Luke Plant 2011-03-17 17:15:52 PDT
Created attachment 86121 [details]
HTML page showing the problem

Adding page as attachment, in case the hosted version disappears.
Comment 2 Aaron Boushley 2012-10-22 07:10:41 PDT
I'm still seeing this bug in Chrome 23 beta. In my case it is content editable divs that are position relative. If I remove the position relative deletion of lines is no longer buggy. With position relative when the user deletes the last character in the div (before I would expect the div to be deleted) the div is removed, and the content of the next div is appended with a br to the following div.

So if I have:
// CSS
div { position: relative; }
// HTML
<div>first</div>
<div>second</div>
<div>t</div>
<div>fourth</div>
<div>fifth</div>
<div>sixth</div>

If my cursor is following the t on the third line, and I hit backspace once, the resulting DOM looks like this:
<div>first</div>
<div>second</div>
<div><br>fifth fourth</div>
<div>sixth</div>
Comment 3 Aaron Boushley 2012-10-22 07:11:37 PDT
This is also happening in Safari 6.0.1 (7536.26.14)
Comment 4 Luke Plant 2012-10-22 08:18:50 PDT
Can we at least move this to 'CONFIRMED' now? It is 100% reproducible in at least 4 WebKit browser versions, old and new, with a simple test case.
Comment 5 Aaron Boushley 2012-10-22 09:08:29 PDT
It should also be noted that the user needs to be the one to create these elements. If the elements are pre-populated in the DOM, deletion appears to work just fine. So the attached test case really is the minimal case. If you were to append the 1-4 p tags and then just delete number 2 it would not show the incorrect behavior.
Comment 6 Aaron Boushley 2012-10-22 09:19:11 PDT
It also appears that this is not limited to position relative. This bug reproduces with position absolute (http://jsfiddle.net/boushley/KQXt5/1/) or position fixed (http://jsfiddle.net/boushley/KQXt5/2/) as well. The minimal and relative test case can also be found here: http://jsfiddle.net/boushley/KQXt5/
Comment 7 Santosh Mahto 2013-10-05 10:00:13 PDT
Created attachment 213456 [details]
work in progress
Comment 8 Santosh Mahto 2013-10-07 00:49:20 PDT
Created attachment 213569 [details]
Patch
Comment 9 Santosh Mahto 2013-10-07 01:02:59 PDT
The bug is happening due to postioned element(relative/absolute/fixed) is treated as special element.
When element is special , its complete node is deleted when its content are deleted. this cause the Dom tree mutation and the initial ranges gets invalidated.
The merging blocks code after deletion works on invalid ranges and cause wrong 
merging.

I could not see any reason why positioned element is treated as special element.
So in this patch i removed it.

Please update me if any comment you have.
Comment 10 Build Bot 2013-10-07 01:41:56 PDT
Comment on attachment 213569 [details]
Patch

Attachment 213569 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3497101

New failing tests:
editing/execCommand/positioned-no-special-element.html
Comment 11 Build Bot 2013-10-07 01:41:58 PDT
Created attachment 213571 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2013-10-07 01:55:39 PDT
Comment on attachment 213569 [details]
Patch

Attachment 213569 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3488112

New failing tests:
editing/execCommand/positioned-no-special-element.html
Comment 13 Build Bot 2013-10-07 01:55:41 PDT
Created attachment 213573 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 14 Santosh Mahto 2013-10-07 02:18:35 PDT
Created attachment 213574 [details]
Patch
Comment 15 Antonio Gomes 2013-10-07 05:59:20 PDT
Comment on attachment 213574 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        When paragraph is positioned(relative/absolute/fixed) then deleting
> +        that paragraph cause wrong merging of other(below) paragraph. This is
> +        happening because positioned element is treated as special element and
> +        on deletion complete paragraph element is removed. The ranges become
> +        invalid after dom mutation and causing the wrong merging of below
> +        paragraph.

it could be worth it to mention when this check was added, what it fixed and why it is not needed anymore.
Comment 16 Santosh Mahto 2013-10-07 08:44:21 PDT
(In reply to comment #15)
> (From update of attachment 213574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213574&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        When paragraph is positioned(relative/absolute/fixed) then deleting
> > +        that paragraph cause wrong merging of other(below) paragraph. This is
> > +        happening because positioned element is treated as special element and
> > +        on deletion complete paragraph element is removed. The ranges become
> > +        invalid after dom mutation and causing the wrong merging of below
> > +        paragraph.
> 
> it could be worth it to mention when this check was added, what it fixed and why it is not needed anymore.

Hmm.. 
The line of code was added long back( 9 years) in
http://trac.webkit.org/changeset/8935

After checking changeLog I could not find why it was added, it seemed to be that function isSpecialElement() was boosted  to include table and list element as special element which is valid. 

Basically when you delete the content of special element, the special element is also deleted which is valid for link, table, and list(lately not) but does not suit to  positioned(relative/absolute) element. probably the behavior was planned for positioned element as same as table/link but not considered later.

Actually I cant think why the deletion behavior of case 1 and case2 should differ although both are same on display:

case 1:  deleting A leaves empty paragraph
<p>A</p>

case 2:  deleting A deletes paragraph node because p becomes special node
<p style= "position:relative">A</p>


To me it looks changes lines are unnecessary now and only create bugs...
Please correct me if I am missing any points.

Probably darin/rniwa could help more on this.
Comment 17 Ryosuke Niwa 2013-10-07 11:00:44 PDT
Comment on attachment 213574 [details]
Patch

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

> Source/WebCore/editing/htmlediting.cpp:-420
> -    if (renderer->style()->position() != StaticPosition)
> -        return true;

There are other position values such as absolute and fixed.
I don't think we want the same behavior as relative for those two values at least.
At minimum, we need a test case. r- because of this.
Comment 18 Santosh Mahto 2013-10-08 07:44:36 PDT
Created attachment 213684 [details]
Patch
Comment 19 Build Bot 2013-10-08 08:26:41 PDT
Comment on attachment 213684 [details]
Patch

Attachment 213684 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3748013

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/5546763.html
editing/unsupported-content/table-delete-001.html
Comment 20 Build Bot 2013-10-08 08:26:43 PDT
Created attachment 213688 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 21 Build Bot 2013-10-08 08:56:07 PDT
Comment on attachment 213684 [details]
Patch

Attachment 213684 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3711210

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/5546763.html
editing/unsupported-content/table-delete-001.html
Comment 22 Build Bot 2013-10-08 08:56:09 PDT
Created attachment 213689 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 23 Santosh Mahto 2013-10-08 09:22:44 PDT
Created attachment 213692 [details]
Patch
Comment 24 Santosh Mahto 2013-10-08 09:26:49 PDT
Reason for failing testcase
diting/unsupported-content/table-delete-003.html --> current behaviour was wrong so updated the test with expected output

editing/deleting/5546763.html  -> This test is for crash so updated the expected oputput.

editing/unsupported-content/table-delete-001.html--> current behaviour was wrong
so updated the expected output with write behaviour.


With Latest patch some table related bug is also fixed(see table-delete-003.html
and  table-delete-001.html)
Comment 25 Build Bot 2013-10-08 10:35:03 PDT
Comment on attachment 213692 [details]
Patch

Attachment 213692 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3747162

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/table-delete-001.html
Comment 26 Build Bot 2013-10-08 10:35:05 PDT
Created attachment 213700 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 27 Build Bot 2013-10-08 11:06:16 PDT
Comment on attachment 213692 [details]
Patch

Attachment 213692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3422242

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/table-delete-001.html
Comment 28 Build Bot 2013-10-08 11:06:20 PDT
Created attachment 213701 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 29 Build Bot 2013-10-08 11:32:07 PDT
Comment on attachment 213692 [details]
Patch

Attachment 213692 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3716254

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/table-delete-001.html
Comment 30 Build Bot 2013-10-08 11:32:09 PDT
Created attachment 213702 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 31 Santosh Mahto 2013-10-09 09:00:55 PDT
Created attachment 213781 [details]
Patch
Comment 32 Build Bot 2013-10-09 09:48:21 PDT
Comment on attachment 213781 [details]
Patch

Attachment 213781 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3768063

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/table-delete-001.html
Comment 33 Build Bot 2013-10-09 09:48:23 PDT
Created attachment 213788 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 34 Build Bot 2013-10-09 10:30:33 PDT
Comment on attachment 213781 [details]
Patch

Attachment 213781 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3772025

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/table-delete-001.html
Comment 35 Build Bot 2013-10-09 10:30:35 PDT
Created attachment 213791 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 36 Santosh Mahto 2013-10-09 11:00:49 PDT
Created attachment 213794 [details]
Patch
Comment 37 Ryosuke Niwa 2013-10-09 13:00:58 PDT
Comment on attachment 213794 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        LayoutTest:

Don't this need line.

> LayoutTests/ChangeLog:12
> +        And expected output is updated with correct behaviour.

We call that "rebaseline". So just say "Rebaselined some tests" but you should explain why new outputs are correct.

> LayoutTests/ChangeLog:16
> +        * editing/execCommand/positioned-no-special-element-expected.txt: Added.
> +        * editing/execCommand/positioned-no-special-element.html: Added.

I would have moved this test into editing/deleting and named it deleting-relatively-positioned-special-element.html to be more precise.

> LayoutTests/ChangeLog:20
> +        * platform/mac/editing/unsupported-content/table-delete-001-expected.txt:
> +        * platform/mac/editing/unsupported-content/table-delete-003-expected.txt:

Why don't we convert these tests to dump-as-markup/marku-as-text tests first?
The change in the expected result is hard to understand as is.

> LayoutTests/editing/execCommand/positioned-no-special-element.html:9
> +    <div id="container"  contenteditable><p>1</p><p id="paraToDelete">2</p><p>3</p><p>4</p></div>

Please spell out paragraph. Also there are two spaces before contenteditable.

> LayoutTests/editing/execCommand/positioned-no-special-element.html:12
> +<script>

Why is this script element defined outside of body?

> LayoutTests/editing/execCommand/positioned-no-special-element.html:13
> +    window.getSelection().setPosition(paraToDelete, 1);

We don't need "window.".  We should use the standard collapse function as in:
getSelection().collapse(paragraphToDelete, 1);

> LayoutTests/editing/execCommand/positioned-no-special-element.html:18
> +    Markup.dump('container');

You can call dump after setting the selection but before calling execCommand('Delete') to show the initial state.
That'll make the test result even more self-descriptive.
Comment 38 Santosh Mahto 2013-10-10 10:47:22 PDT
Created attachment 213901 [details]
Patch
Comment 39 Ryosuke Niwa 2013-10-10 11:02:54 PDT
Comment on attachment 213901 [details]
Patch

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

> LayoutTests/editing/unsupported-content/table-delete-001.html:28
>  <div class="results">
> -beforeafter
> +First<br>Second
>  </div>

We don't need this, do we?

> LayoutTests/editing/unsupported-content/table-delete-003.html:29
>  <div class="results">
>  <br>
> -after
> +Second
>  </div>

Ditto.
Comment 40 Santosh Mahto 2013-10-10 11:08:13 PDT
(In reply to comment #39)
> (From update of attachment 213901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213901&action=review
> 
> > LayoutTests/editing/unsupported-content/table-delete-001.html:28
> >  <div class="results">
> > -beforeafter
> > +First<br>Second
> >  </div>
> 
> We don't need this, do we?
> 
> > LayoutTests/editing/unsupported-content/table-delete-003.html:29
> >  <div class="results">
> >  <br>
> > -after
> > +Second
> >  </div>
> 
> Ditto.


For sake of cleanness:
I added dump before and after deletion with title BeforeDelete: and AfterDelete:, So It looked ambigous to already present 'before' and 'after'.
Comment 41 Ryosuke Niwa 2013-10-10 11:53:05 PDT
Comment on attachment 213901 [details]
Patch

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

>>> LayoutTests/editing/unsupported-content/table-delete-001.html:28
>>>  </div>
>> 
>> We don't need this, do we?
> 
> For sake of cleanness:
> I added dump before and after deletion with title BeforeDelete: and AfterDelete:, So It looked ambigous to already present 'before' and 'after'.

Why can't we just remove the entire div?
Comment 42 Santosh Mahto 2013-10-10 13:05:40 PDT
Created attachment 213918 [details]
Patch
Comment 43 Santosh Mahto 2013-10-10 13:07:39 PDT
(In reply to comment #41)
> (From update of attachment 213901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213901&action=review
> 
> >>> LayoutTests/editing/unsupported-content/table-delete-001.html:28
> >>>  </div>
> >> 
> >> We don't need this, do we?
> > 
> > For sake of cleanness:
> > I added dump before and after deletion with title BeforeDelete: and AfterDelete:, So It looked ambigous to already present 'before' and 'after'.
> 
> Why can't we just remove the entire div?
Done!
Comment 44 WebKit Commit Bot 2013-10-10 23:17:14 PDT
Comment on attachment 213918 [details]
Patch

Clearing flags on attachment: 213918

Committed r157292: <http://trac.webkit.org/changeset/157292>
Comment 45 WebKit Commit Bot 2013-10-10 23:17:19 PDT
All reviewed patches have been landed.  Closing bug.