Bug 86345 - Deleting a content-editable element having a ShadowRoot causes a crash
Summary: Deleting a content-editable element having a ShadowRoot causes a crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shinya Kawanaka
URL:
Keywords:
Depends on:
Blocks: 82697
  Show dependency treegraph
 
Reported: 2012-05-14 02:14 PDT by Shinya Kawanaka
Modified: 2012-06-07 05:23 PDT (History)
9 users (show)

See Also:


Attachments
Patch (4.95 KB, patch)
2012-06-05 01:36 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch for landing (5.16 KB, patch)
2012-06-06 23:45 PDT, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-05-14 02:14:55 PDT
Repro:

<div id="container" contenteditable></div>
<script>
var shadowRoot = new WebKitShadowRoot(container);
shadowRoot.innerHTML = "<span contenteditable>HOGE</span>";
</script>


Select somewhere from after the container to the container. (Don't select HOGE but container.)
And press DELETE.
Comment 1 Shinya Kawanaka 2012-05-14 05:11:35 PDT
When an element does not have a shadow root, we cannot select contenteditable element in this example... So I think it is odd that contenteditable can be selected by this code...
Comment 2 Ryosuke Niwa 2012-05-14 12:09:55 PDT
(In reply to comment #1)
> When an element does not have a shadow root, we cannot select contenteditable element in this example... So I think it is odd that contenteditable can be selected by this code...

That's probably because we check the existence of the renderer :(
Comment 3 Shinya Kawanaka 2012-05-14 18:55:29 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > When an element does not have a shadow root, we cannot select contenteditable element in this example... So I think it is odd that contenteditable can be selected by this code...
> 
> That's probably because we check the existence of the renderer :(

Yeah... probably so. I'll investigate this more today.
Comment 4 Shinya Kawanaka 2012-06-05 01:36:35 PDT
Created attachment 145724 [details]
Patch
Comment 5 Ryosuke Niwa 2012-06-06 12:28:23 PDT
Comment on attachment 145724 [details]
Patch

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

> Source/WebCore/editing/VisibleSelection.cpp:457
> +        // boundaries again. See http://wkb.ug/87463

Can we use the regular webkit bug URL instead? I don't see a benefit in using a short URL in a comment like this.

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:8
> +<p>This test confirms that selecting an element having Shadow DOM doesn't cross editing boundaries errornously.</p>

Should we say something along the line of this test passes if WebKit didn't crash?

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:34
> +// Checks crash won't happen.
> +document.execCommand('delete');

You should use debug() to print out something to indicate we passed the test.

> LayoutTests/editing/shadow/select-contenteditable-shadowhost.html:46
> +// Check crash won't happen.
> +document.execCommand('delete');

Ditto.
Comment 6 Shinya Kawanaka 2012-06-06 23:45:53 PDT
Created attachment 146209 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-06-07 05:22:58 PDT
Comment on attachment 146209 [details]
Patch for landing

Clearing flags on attachment: 146209

Committed r119711: <http://trac.webkit.org/changeset/119711>
Comment 8 WebKit Review Bot 2012-06-07 05:23:03 PDT
All reviewed patches have been landed.  Closing bug.