Bug 106526 - When a selected node in nested ShadowDOM is deleted, selection have wrong range.
Summary: When a selected node in nested ShadowDOM is deleted, selection have wrong range.
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: 97279
  Show dependency treegraph
 
Reported: 2013-01-09 20:32 PST by Shinya Kawanaka
Modified: 2013-01-10 20:45 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.56 KB, patch)
2013-01-09 22:18 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch for landing (7.43 KB, patch)
2013-01-10 20:28 PST, 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 2013-01-09 20:32:31 PST
selection will contain deleted host element.
If node in non-nested ShadowDOM is deleted, selection contains BODY, though.
Comment 1 Shinya Kawanaka 2013-01-09 22:18:25 PST
Created attachment 182064 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2013-01-10 08:03:43 PST
Ryosuke, can you take a look?
Comment 3 Ryosuke Niwa 2013-01-10 09:41:16 PST
Comment on attachment 182064 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:341
> -    return element->contains(position.anchorNode()) || element->contains(position.anchorNode()->shadowAncestorNode());
> +    return element->containsIncludingShadowDOM(position.anchorNode());

This is strange since selection & editing operations are limited to one level of shadow DOM. Why should removing a node that contains shadow DOM update the selection of that removed shadow DOM?
Comment 4 Shinya Kawanaka 2013-01-10 19:00:50 PST
I think the original code did not consider nested Shadow DOM case at all.

Let's consider the following example.

div1 -- SR
        |-- div2 -- SR
                    |- div3
                         |-- #text

When text in div3 is selected, and div1 is removed from the tree.
removingNodeRemovesPosition() is called with node = div1, and position = somewhere in #text.
In this case, 
  element->contains(position.anchorNode()) || element->contains(position.anchorNode()->shadowAncestorNode());
returns false. However, this is strange. when div1 is removed, position in #text also should be removed, I think.
Comment 5 Ryosuke Niwa 2013-01-10 19:18:01 PST
(In reply to comment #4)
> I think the original code did not consider nested Shadow DOM case at all.
> 
> Let's consider the following example.
> 
> div1 -- SR
>         |-- div2 -- SR
>                     |- div3
>                          |-- #text
> 
> When text in div3 is selected, and div1 is removed from the tree.
> removingNodeRemovesPosition() is called with node = div1, and position = somewhere in #text.
> In this case, 
>   element->contains(position.anchorNode()) || element->contains(position.anchorNode()->shadowAncestorNode());
> returns false. However, this is strange. when div1 is removed, position in #text also should be removed, I think.

But whatever position we adjust to inside SR for div2 is meaningless at that point, right? I mean why should detaching an ancestor of the shadow host should affect how selection works inside a shadow DOM? It seems like positions and selections inside div2's shadow root should be completely independent of what happens outside of the shadow root, no?
Comment 6 Shinya Kawanaka 2013-01-10 19:30:33 PST
> But whatever position we adjust to inside SR for div2 is meaningless at that point, right? I mean why should detaching an ancestor of the shadow host should affect how selection works inside a shadow DOM? It seems like positions and selections inside div2's shadow root should be completely independent of what happens outside of the shadow root, no?

However, since the selected node is removed from the Document, we have to adjust FrameSelection, right?
I think we have to move FrameSelection to around div1 (or remove Selection). To achieve it, we have to check selection is in Shadow DOM.

Currently we don't move Selection if it's in nested ShadowDOM but we move Selection if it's in non-nested Shadow DOM. This is apparently wrong.
Comment 7 Ryosuke Niwa 2013-01-10 19:34:20 PST
Sorry, I misunderstood your change. I was thinking about shadowRoot.selection but your change affects FrameSelection, the one and the only selection per frame.
Comment 8 Ryosuke Niwa 2013-01-10 20:02:58 PST
Comment on attachment 182064 [details]
Patch

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

> LayoutTests/fast/dom/shadow/selection-in-nested-shadow-expected.txt:3
> +PASS anchorNode1 is container
> +PASS anchorNode2 is container
> +PASS successfullyParsed is true

This output isn't so helpful. Please give more descriptive variable names.
Comment 9 Shinya Kawanaka 2013-01-10 20:28:20 PST
Created attachment 182251 [details]
Patch for landing
Comment 10 WebKit Review Bot 2013-01-10 20:45:44 PST
Comment on attachment 182251 [details]
Patch for landing

Clearing flags on attachment: 182251

Committed r139401: <http://trac.webkit.org/changeset/139401>
Comment 11 WebKit Review Bot 2013-01-10 20:45:47 PST
All reviewed patches have been landed.  Closing bug.