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.
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>
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.
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.
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
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 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.
(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 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.
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
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
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)
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
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
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
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
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 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.
(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 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?
(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!
2011-03-17 17:15 PDT, Luke Plant
2013-10-05 10:00 PDT, Santosh Mahto
2013-10-07 00:49 PDT, Santosh Mahto
2013-10-07 01:41 PDT, Build Bot
2013-10-07 01:55 PDT, Build Bot
2013-10-07 02:18 PDT, Santosh Mahto
2013-10-08 07:44 PDT, Santosh Mahto
2013-10-08 08:26 PDT, Build Bot
2013-10-08 08:56 PDT, Build Bot
2013-10-08 09:22 PDT, Santosh Mahto
2013-10-08 10:35 PDT, Build Bot
2013-10-08 11:06 PDT, Build Bot
2013-10-08 11:32 PDT, Build Bot
2013-10-09 09:00 PDT, Santosh Mahto
2013-10-09 09:48 PDT, Build Bot
2013-10-09 10:30 PDT, Build Bot
2013-10-09 11:00 PDT, Santosh Mahto
2013-10-10 10:47 PDT, Santosh Mahto
2013-10-10 13:05 PDT, Santosh Mahto