Bug 88514 - An inheritance of '-webkit-user-modify' does not stop at shadow boundary.
Summary: An inheritance of '-webkit-user-modify' does not stop at shadow boundary.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
Depends on:
Blocks: 82692 82697 88485
  Show dependency treegraph
 
Reported: 2012-06-07 02:01 PDT by Hayato Ito
Modified: 2012-06-10 21:29 PDT (History)
10 users (show)

See Also:


Attachments
test case (1.25 KB, text/html)
2012-06-07 02:03 PDT, Hayato Ito
no flags Details
reset a user-modify when using a cached result (9.20 KB, patch)
2012-06-07 21:23 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Update inheritFrom (12.71 KB, patch)
2012-06-08 02:47 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Minor update (12.27 KB, patch)
2012-06-08 03:00 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
iter (11.99 KB, patch)
2012-06-08 04:18 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (12.42 KB, patch)
2012-06-10 15:54 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (12.52 KB, patch)
2012-06-10 17:29 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-06-07 02:01:50 PDT
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'.
Comment 1 Hayato Ito 2012-06-07 02:03:26 PDT
Created attachment 146233 [details]
test case
Comment 2 Hayato Ito 2012-06-07 02:05:14 PDT
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.
Comment 3 Ryosuke Niwa 2012-06-07 18:06:39 PDT
We do this in style resolution. See StyleSelector.cpp and CSSStyleSelector.cpp.
Comment 4 Hayato Ito 2012-06-07 21:23:11 PDT
Created attachment 146470 [details]
reset a user-modify when using a cached result
Comment 5 Hayato Ito 2012-06-07 21:25:43 PDT
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.
Comment 6 Ryosuke Niwa 2012-06-07 22:02:35 PDT
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.
Comment 7 Hayato Ito 2012-06-07 22:36:26 PDT
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?
Comment 8 Hayato Ito 2012-06-08 02:47:20 PDT
Created attachment 146524 [details]
Update inheritFrom
Comment 9 Hayato Ito 2012-06-08 03:00:02 PDT
Created attachment 146527 [details]
Minor update
Comment 10 Ryosuke Niwa 2012-06-08 03:02:32 PDT
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.
Comment 11 Hayato Ito 2012-06-08 04:18:15 PDT
Created attachment 146538 [details]
iter
Comment 12 Hayato Ito 2012-06-08 04:25:19 PDT
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.
Comment 13 Ryosuke Niwa 2012-06-08 09:30:55 PDT
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.
Comment 14 Hayato Ito 2012-06-10 15:54:56 PDT
Created attachment 146754 [details]
Patch for landing
Comment 15 Hayato Ito 2012-06-10 15:55:34 PDT
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.
Comment 16 WebKit Review Bot 2012-06-10 15:56:59 PDT
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
Comment 17 Hayato Ito 2012-06-10 17:29:54 PDT
Created attachment 146761 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-06-10 20:59:13 PDT
Comment on attachment 146761 [details]
Patch for landing

Clearing flags on attachment: 146761

Committed r119949: <http://trac.webkit.org/changeset/119949>
Comment 19 WebKit Review Bot 2012-06-10 20:59:19 PDT
All reviewed patches have been landed.  Closing bug.