Bug 82021 – Position should be able to have ShadowRoot as a container.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

Currently Position and Range are not allowed to have ShadowRoot as a container.
However, this makes it difficult to point direct children of ShadowRoot. Position and Range try pointing direct children of ShadowRoot, but since it's not allowed, they become null. Since a lot of code does not expect Position and Range become null,  a lot of crashes are caused when doing something on elements in shadow subtrees.

We should try to allow Position and Range to have ShadowRoot as a container.
Note that we should be careful not to expose a ShadowRoot in JS.

[Ryosuke Niwa]

This is going to be a really patch to review. You basically need to manually audit the entire editing code to make sure nothing depends on the current behavior. When text from controls transitioned to use new shadow DOM code, there were quite few places where we had to make changes.

[Shinya Kawanaka]

Created attachment 133987 [details]
Patch

[Hajime Morrita]

I support this approach.

From a higher viewpoint, it's the only path to allow position on shadow root
if we want to  define certain types of positions. And we want to do it.
Otherwise we'd have no way to define any position between shadow children.

Veteran built-in elements avoid this ambiguousness by forming its shadow tree carefully
and making tricky position impossible by giving -webkit-user-modify:none.  
But such an approach is fragile and can cause problems like http://wkb.ug/65738

[Shinya Kawanaka]

Could someone review this?

[Dimitri Glazkov (Google)]

Comment on attachment 133987 [details]
Patch

The patch makes sense to me, but I defer to Ryosuke, because he knows this area a lot better. Niwa-san, review? review? review? :)

[Ryosuke Niwa]

Comment on attachment 133987 [details]
Patch

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

How about all the places in VisibleSelection that call shadowAncestorNode and nonBoundaryShadowTreeRootNode? Editor::nextVisibleRange, Editor::rangeOfString, etc... call call some shadow-related functions.

In short, I don't know how to review this patch. Unfortunately, having passed all existing tests is not a good enough indication that a given change is correct in editing.

> Source/WebCore/dom/Position.cpp:94
> -    ASSERT(!((anchorType == PositionIsBeforeChildren || anchorType == PositionIsAfterChildren)
> -        && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get()))));
> +    ASSERT(!m_anchorNode.get()
> +        || (!((anchorType == PositionIsBeforeChildren || anchorType == PositionIsAfterChildren)
> +             && (m_anchorNode->isTextNode() || editingIgnoresContent(m_anchorNode.get())))));

Why are we making this change? As far as I know, we should never be making a position before/after children with a null container.

> LayoutTests/ChangeLog:22
> [phone]  Shinya Kawanaka  <[e-mail]>
> +
> +        Position should be able to have ShadowRoot as a container.
> +        https://bugs.webkit.org/show_bug.cgi?id=82021
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/dom/shadow/doubleclick-on-meter-in-shadow-crash.html: Added.
> +        * fast/dom/shadow/rightclick-on-meter-in-shadow-crash.html: Added.
> +

Duplicate entry.

[Shinya Kawanaka]

Created attachment 134720 [details]
Patch

[Shinya Kawanaka]

(In reply to comment #3)
> I support this approach.
> 
> From a higher viewpoint, it's the only path to allow position on shadow root
> if we want to  define certain types of positions. And we want to do it.
> Otherwise we'd have no way to define any position between shadow children.
> 
> Veteran built-in elements avoid this ambiguousness by forming its shadow tree carefully
> and making tricky position impossible by giving -webkit-user-modify:none.  
> But such an approach is fragile and can cause problems like http://wkb.ug/65738

Such a bug is caused by another issue. The reason of the bug is UA shadow elements is selectable.
But the reason of this bug is elements are not selectable correctly.

# I confirmed my patch doesn't fix http://wkb.ug/65738

[Shinya Kawanaka]

I've extracted all code where Position::containerNode() is used.

https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0Aty2DXLelNGhdFhuaWliQWFnNEY0blNtX2lLUTRDZHc&pli=1#gid=0

Maybe we should create explicit tests for them to prove this patch does not introduce vulnerability...

[Shinya Kawanaka]

I've talked with rniwa and morrita in IRC, we have decided to add this patch behind the SHADOW_DOM flag.

Fixing editing bugs in Shadow DOM is really necessary this patch. Most of them depends on this.
So when this patch is landed, we can proceed fixing editing bugs one by one. This could be a great step.

[Shinya Kawanaka]

Created attachment 140698 [details]
Patch

[Build Bot]

Comment on attachment 140698 [details]
Patch

Attachment 140698 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12642640

[Gyuyoung Kim]

Comment on attachment 140698 [details]
Patch

Attachment 140698 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12648292

[Build Bot]

Comment on attachment 140698 [details]
Patch

Attachment 140698 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12643642

[Dimitri Glazkov (Google)]

Comment on attachment 140698 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        Currently this change is only enabled if shadow dom flag is enabled, since we cannot
> +        prove this change does not destroy the existing behavior. However, this change is really required
> +        to fix other editing bugs in Shadow DOM. A bunch of patches and tests will be added to

I am ok with this as a temporary measure, but we need to have a plan to move forward. At some point, we will turn Shadow DOM on by default :)

> Source/WebCore/dom/Position.h:221
> +inline ContainerNode* findParent(const Node* node)

A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position?

[Ryosuke Niwa]

Comment on attachment 140698 [details]
Patch

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

Please fix all the nits below and builds before you land.

> Source/WebCore/dom/Position.cpp:114
> +    ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get()));

If RuntimeEnabledFeatures::shadowDOMEnabled() is false, then m_anchorNode should not be a shadow root.

>> Source/WebCore/dom/Position.h:221
>> +inline ContainerNode* findParent(const Node* node)
> 
> A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position?

Yes, please make this a private static member of Position. I would also rename it to something like parent(). I don't think we want to prefix it with "find" because it doesn't really search for the parent.

> Source/WebCore/dom/Position.h:227
> +    // FIXME: Since enabling Position to have ShadowRoot as a container is not proved not to
> +    // break editing code. However, this is really required for editing in Shadow DOM.
> +    // So we implement this behind the flag. After writing a lot of tests for them to confirm
> +    // this is safe, we will remove the flag for this purpose.
> +    // See also this umbrella bugs. http://wkb.ug/82697

This comment just repeats what the code says. You should just say "See http://webkit.org/b/82697" IMO.

[Ryosuke Niwa]

Comment on attachment 140698 [details]
Patch

Ugh... I meant r+, not r-.

[Shinya Kawanaka]

Created attachment 140843 [details]
Build test

[Shinya Kawanaka]

Created attachment 140844 [details]
Build test

[Shinya Kawanaka]

Thank you for the reviewing!
I've updated the patch to fix what you mentioned. After all the bots are green, I'll land this patch.

(In reply to comment #16)
> (From update of attachment 140698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140698&action=review
> 
> Please fix all the nits below and builds before you land.
> 
> > Source/WebCore/dom/Position.cpp:114
> > +    ASSERT(!m_anchorNode || !editingIgnoresContent(m_anchorNode.get()));
> 
> If RuntimeEnabledFeatures::shadowDOMEnabled() is false, then m_anchorNode should not be a shadow root.
> 
> >> Source/WebCore/dom/Position.h:221
> >> +inline ContainerNode* findParent(const Node* node)
> > 
> > A free-standing function with a generic name like this seems like a bad thing. Can it be a static member of Position?
> 
> Yes, please make this a private static member of Position. I would also rename it to something like parent(). I don't think we want to prefix it with "find" because it doesn't really search for the parent.
> 
> > Source/WebCore/dom/Position.h:227
> > +    // FIXME: Since enabling Position to have ShadowRoot as a container is not proved not to
> > +    // break editing code. However, this is really required for editing in Shadow DOM.
> > +    // So we implement this behind the flag. After writing a lot of tests for them to confirm
> > +    // this is safe, we will remove the flag for this purpose.
> > +    // See also this umbrella bugs. http://wkb.ug/82697
> 
> This comment just repeats what the code says. You should just say "See http://webkit.org/b/82697" IMO.

[Build Bot]

Comment on attachment 140844 [details]
Build test

Attachment 140844 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12645546

[WebKit Review Bot]

Comment on attachment 140844 [details]
Build test

Attachment 140844 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12650498

[Shinya Kawanaka]

Created attachment 140856 [details]
Build test

[Shinya Kawanaka]

Created attachment 140869 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140869 [details]
Patch for landing

Clearing flags on attachment: 140869

Committed r116508: <http://trac.webkit.org/changeset/116508>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.