Bug 88514

Created attachment 146233 [details] test case

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.

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

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

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 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.

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?

Created attachment 146524 [details] Update inheritFrom

Created attachment 146527 [details] Minor update

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.

Created attachment 146538 [details] iter

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 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.

Created attachment 146754 [details] Patch for landing

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 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

Created attachment 146761 [details] Patch for landing

Comment on attachment 146761 [details] Patch for landing Clearing flags on attachment: 146761 Committed r119949: <http://trac.webkit.org/changeset/119949>

All reviewed patches have been landed. Closing bug.