Bug 33950 – contentEditable with "position:relative" paragraphs is buggy

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

[Luke Plant]

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.

[Luke Plant]

Created attachment 86121 [details]
HTML page showing the problem

Adding page as attachment, in case the hosted version disappears.

[Aaron Boushley]

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>

[Aaron Boushley]

This is also happening in Safari 6.0.1 (7536.26.14)

[Luke Plant]

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.

[Aaron Boushley]

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.

[Aaron Boushley]

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/

[Santosh Mahto]

Created attachment 213456 [details]
work in progress

[Santosh Mahto]

Created attachment 213569 [details]
Patch

[Santosh Mahto]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 213574 [details]
Patch

[Antonio Gomes]

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.

[Santosh Mahto]

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

[Ryosuke Niwa]

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.

[Santosh Mahto]

Created attachment 213684 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 213692 [details]
Patch

[Santosh Mahto]

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)

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 213781 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Santosh Mahto]

Created attachment 213794 [details]
Patch

[Ryosuke Niwa]

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.

[Santosh Mahto]

Created attachment 213901 [details]
Patch

[Ryosuke Niwa]

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.

[Santosh Mahto]

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

[Ryosuke Niwa]

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?

[Santosh Mahto]

Created attachment 213918 [details]
Patch

[Santosh Mahto]

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

[WebKit Commit Bot]

Comment on attachment 213918 [details]
Patch

Clearing flags on attachment: 213918

Committed r157292: <http://trac.webkit.org/changeset/157292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.