Bug 88514 – An inheritance of '-webkit-user-modify' does not stop at shadow boundary.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Hayato Ito]

According to the Shadow DOM spec (https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#editing),  a '-webkit-user-modify' CSS property should not be inherited across shadow boundaries.
But in the attached html (Let me attach it later), '-webkit-user-modify' property of the node in Shadow DOM subtree will be set to 'read-write' wrongly. That should be 'read-only'.

[Hayato Ito]

Created attachment 146233 [details]
test case

[Hayato Ito]

The condition to reproduce this issue seems very sensitive. For example, if we remove the first <p> from the html, we cannot reproduce the issue.

[Ryosuke Niwa]

We do this in style resolution. See StyleSelector.cpp and CSSStyleSelector.cpp.

[Hayato Ito]

Created attachment 146470 [details]
reset a user-modify when using a cached result

[Hayato Ito]

Yeah, there are some efforts to stop propagation in StyleResolver. But that is not enough.
We should do similar thing when using a cached result.

(In reply to comment #3)
> We do this in style resolution. See StyleSelector.cpp and CSSStyleSelector.cpp.

[Ryosuke Niwa]

Comment on attachment 146470 [details]
reset a user-modify when using a cached result

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

> Source/WebCore/css/StyleResolver.cpp:1679
> -    applyMatchedProperties(matchResult);
> +    applyMatchedProperties(matchResult, element);

Doesn't adjusting -webkit-user-modify here override author rule to make children of shadow root editable since author styles had been applied to m_style at this point?
I think what we need to do is modify RenderStyle::inheritFrom and prevent inheriting -webkit-user-modify at the shadow boundary.

[Hayato Ito]

Thank you for the review.

(In reply to comment #6)
> (From update of attachment 146470 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146470&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1679
> > -    applyMatchedProperties(matchResult);
> > +    applyMatchedProperties(matchResult, element);
> 
> Doesn't adjusting -webkit-user-modify here override author rule to make children of shadow root editable since author styles had been applied to m_style at this point?

Nice point. Let me write a test and make sure it.

> I think what we need to do is modify RenderStyle::inheritFrom and prevent inheriting -webkit-user-modify at the shadow boundary.

Yeah, that is what I thought at first. What makes this difficult is that there are a lot of callers of RenderStyle::inheritFrom and many callers do not have an available Element instance which is required to judge there is a shadow boundary or not. So if we do so, it requires non-trivial refactoring and I guess we don't need to check shadow boundary in most places.

I am now thinking that we should change RenderStyle::inheritFrom's signature like:

enum ShadowBoundary {
  AtShadowBoundary,
  NoShadowBoundary
}
RenderStyle::inheritFrom(Style* parentStyle, ShadowBoundary = NoShadowBoundary)

In most callers, we can omit ShadowBoundary parameter, I think.

What do you think about it?

[Hayato Ito]

Created attachment 146524 [details]
Update inheritFrom

[Hayato Ito]

Created attachment 146527 [details]
Minor update

[Ryosuke Niwa]

Comment on attachment 146527 [details]
Minor update

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

> Source/WebCore/rendering/style/RenderStyle.h:396
> +    enum ShadowBoundary {
> +        AtShadowBoundary,
> +        NoShadowBoundary,
> +    };

Should be
enum IsAtShadowBoundary {
    AtShadowBoundary,
    NotAtShadowBoundary,
}

> LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt:1
> +This p is required to produce the issue.

What does this mean? What do you mean by "required to produce the issue", and what is the issue?

> LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt:3
> +This div and parent div are required to produce the issue.

Ditto.

[Hayato Ito]

Created attachment 146538 [details]
iter

[Hayato Ito]

Comment on attachment 146527 [details]
Minor update

Thank you for the review.

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

>> Source/WebCore/rendering/style/RenderStyle.h:396
>> +    };
> 
> Should be
> enum IsAtShadowBoundary {
>     AtShadowBoundary,
>     NotAtShadowBoundary,
> }

Done.

>> LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt:1
>> +This p is required to produce the issue.
> 
> What does this mean? What do you mean by "required to produce the issue", and what is the issue?

Okay. I've removed that.

The <p> tag was required to hit code path which uses a cached result of style in the original test case I posted at comment #1.
I don't think it is worth to leave such tag in the test any more. It's just confusing.

[Ryosuke Niwa]

Comment on attachment 146538 [details]
iter

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

> LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt:6
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host1"), null), userModifyPropertyName) is "read-only"
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host2"), "false"), userModifyPropertyName) is "read-only"
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host3"), "true"), userModifyPropertyName) is "read-write"
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host1"), null), userModifyPropertyName) is "read-only"
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host2"), "false"), userModifyPropertyName) is "read-only"
> +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host3"), "true"), userModifyPropertyName) is "read-write"

Please add a test description.

[Hayato Ito]

Created attachment 146754 [details]
Patch for landing

[Hayato Ito]

Thank you for the review.

Okay. Let me land this after adding a test description.

(In reply to comment #13)
> (From update of attachment 146538 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146538&action=review
> 
> > LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt:6
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host1"), null), userModifyPropertyName) is "read-only"
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host2"), "false"), userModifyPropertyName) is "read-only"
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("contenteditable-host3"), "true"), userModifyPropertyName) is "read-write"
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host1"), null), userModifyPropertyName) is "read-only"
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host2"), "false"), userModifyPropertyName) is "read-only"
> > +PASS computedStyle(prepareNodeInShadowRoot(document.getElementById("non-contenteditable-host3"), "true"), userModifyPropertyName) is "read-write"
> 
> Please add a test description.

[WebKit Review Bot]

Comment on attachment 146754 [details]
Patch for landing

Rejecting attachment 146754 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t 353 (offset 1 line).
patching file Source/WebCore/rendering/style/RenderStyle.cpp
patching file Source/WebCore/rendering/style/RenderStyle.h
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/shadow/user-modify-inheritance-expected.txt
patching file LayoutTests/fast/dom/shadow/user-modify-inheritance.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12943083

[Hayato Ito]

Created attachment 146761 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146761 [details]
Patch for landing

Clearing flags on attachment: 146761

Committed r119949: <http://trac.webkit.org/changeset/119949>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.