Bug 33950 - contentEditable with "position:relative" paragraphs is buggy
: contentEditable with "position:relative" paragraphs is buggy
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-21 06:47 PST by
Modified: 2013-10-10 23:17 PST (History)


Attachments
HTML page showing the problem (334 bytes, text/html)
2011-03-17 17:15 PST, Luke Plant
no flags Details
work in progress (1.19 KB, patch)
2013-10-05 10:00 PST, Santosh Mahto
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2013-10-07 00:49 PST, Santosh Mahto
no flags Review Patch | 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 PST, 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 PST, Build Bot
no flags Details
Patch (4.38 KB, patch)
2013-10-07 02:18 PST, Santosh Mahto
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2013-10-08 07:44 PST, Santosh Mahto
no flags Review Patch | 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 PST, 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 PST, Build Bot
no flags Details
Patch (21.19 KB, patch)
2013-10-08 09:22 PST, Santosh Mahto
no flags Review Patch | 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 PST, 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 PST, 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 PST, Build Bot
no flags Details
Patch (21.19 KB, patch)
2013-10-09 09:00 PST, Santosh Mahto
no flags Review Patch | 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 PST, 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 PST, Build Bot
no flags Details
Patch (15.11 KB, patch)
2013-10-09 11:00 PST, Santosh Mahto
no flags Review Patch | Details | Formatted Diff | Diff
Patch (25.73 KB, patch)
2013-10-10 10:47 PST, Santosh Mahto
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.06 KB, patch)
2013-10-10 13:05 PST, Santosh Mahto
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-03-17 17:15:52 PST -------
Created an attachment (id=86121) [details]
HTML page showing the problem

Adding page as attachment, in case the hosted version disappears.
------- Comment #2 From 2012-10-22 07:10:41 PST -------
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 From 2012-10-22 07:11:37 PST -------
This is also happening in Safari 6.0.1 (7536.26.14)
------- Comment #4 From 2012-10-22 08:18:50 PST -------
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 From 2012-10-22 09:08:29 PST -------
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 From 2012-10-22 09:19:11 PST -------
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 From 2013-10-05 10:00:13 PST -------
Created an attachment (id=213456) [details]
work in progress
------- Comment #8 From 2013-10-07 00:49:20 PST -------
Created an attachment (id=213569) [details]
Patch
------- Comment #9 From 2013-10-07 01:02:59 PST -------
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 From 2013-10-07 01:41:56 PST -------
(From update of attachment 213569 [details])
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 From 2013-10-07 01:41:58 PST -------
Created an attachment (id=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 From 2013-10-07 01:55:39 PST -------
(From update of attachment 213569 [details])
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 From 2013-10-07 01:55:41 PST -------
Created an attachment (id=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 From 2013-10-07 02:18:35 PST -------
Created an attachment (id=213574) [details]
Patch
------- Comment #15 From 2013-10-07 05:59:20 PST -------
(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.
------- Comment #16 From 2013-10-07 08:44:21 PST -------
(In reply to comment #15)
> (From update of attachment 213574 [details] [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 From 2013-10-07 11:00:44 PST -------
(From update of attachment 213574 [details])
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 From 2013-10-08 07:44:36 PST -------
Created an attachment (id=213684) [details]
Patch
------- Comment #19 From 2013-10-08 08:26:41 PST -------
(From update of attachment 213684 [details])
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 From 2013-10-08 08:26:43 PST -------
Created an attachment (id=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 From 2013-10-08 08:56:07 PST -------
(From update of attachment 213684 [details])
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 From 2013-10-08 08:56:09 PST -------
Created an attachment (id=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 From 2013-10-08 09:22:44 PST -------
Created an attachment (id=213692) [details]
Patch
------- Comment #24 From 2013-10-08 09:26:49 PST -------
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 From 2013-10-08 10:35:03 PST -------
(From update of attachment 213692 [details])
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 From 2013-10-08 10:35:05 PST -------
Created an attachment (id=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 From 2013-10-08 11:06:16 PST -------
(From update of attachment 213692 [details])
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 From 2013-10-08 11:06:20 PST -------
Created an attachment (id=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 From 2013-10-08 11:32:07 PST -------
(From update of attachment 213692 [details])
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 From 2013-10-08 11:32:09 PST -------
Created an attachment (id=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 From 2013-10-09 09:00:55 PST -------
Created an attachment (id=213781) [details]
Patch
------- Comment #32 From 2013-10-09 09:48:21 PST -------
(From update of attachment 213781 [details])
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 From 2013-10-09 09:48:23 PST -------
Created an attachment (id=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 From 2013-10-09 10:30:33 PST -------
(From update of attachment 213781 [details])
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 From 2013-10-09 10:30:35 PST -------
Created an attachment (id=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 From 2013-10-09 11:00:49 PST -------
Created an attachment (id=213794) [details]
Patch
------- Comment #37 From 2013-10-09 13:00:58 PST -------
(From update of attachment 213794 [details])
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 From 2013-10-10 10:47:22 PST -------
Created an attachment (id=213901) [details]
Patch
------- Comment #39 From 2013-10-10 11:02:54 PST -------
(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.
------- Comment #40 From 2013-10-10 11:08:13 PST -------
(In reply to comment #39)
> (From update of attachment 213901 [details] [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 From 2013-10-10 11:53:05 PST -------
(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?
------- Comment #42 From 2013-10-10 13:05:40 PST -------
Created an attachment (id=213918) [details]
Patch
------- Comment #43 From 2013-10-10 13:07:39 PST -------
(In reply to comment #41)
> (From update of attachment 213901 [details] [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 From 2013-10-10 23:17:14 PST -------
(From update of attachment 213918 [details])
Clearing flags on attachment: 213918

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