Bug 87805

Created attachment 145543 [details] Patch

Comment on attachment 145543 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145543&action=review This looks good overall, I think we need a bit more test coverage: 1) at least make sure that all of the state transitions are accounted for 2) look at nested shadow dom and multiple shadow roots per element. > Source/WebCore/html/HTMLStyleElement.cpp:55 > , m_isRegisteredWithScopingNode(false) > + , m_scoped(false) Can we not combine these two? Is there ever a situation when you are scoped, but have no scoping node? > Source/WebCore/html/HTMLStyleElement.cpp:96 > +void HTMLStyleElement::updateForStyleScoped() Would it work better if we name it as a notification to react and combine into one method?: styleScopedUpdated(attribute.isNull()); > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:28 > + debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('color')); /* red */ > + debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */ > + debug("F: " + document.defaultView.getComputedStyle(f, null).getPropertyValue('color')); /* black */ > + debug("F: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */ this can be a function. > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:35 > + document.body.offsetLeft; Why do we need this here? > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:42 > + document.body.offsetLeft; .. or here?

Created attachment 145728 [details] Patch

Thank you for reviewing. (In reply to comment #2) > (From update of attachment 145543 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145543&action=review > > This looks good overall, I think we need a bit more test coverage: > 1) at least make sure that all of the state transitions are accounted for > 2) look at nested shadow dom and multiple shadow roots per element. > I see. I added more tests. I think, some existing test would probably cover the case: moving styled scope into shadow dom subtree from document or into document from shadow dom subtree. > > Source/WebCore/html/HTMLStyleElement.cpp:55 > > , m_isRegisteredWithScopingNode(false) > > + , m_scoped(false) > > Can we not combine these two? Is there ever a situation when you are scoped, but have no scoping node? I see. Done. > > > Source/WebCore/html/HTMLStyleElement.cpp:96 > > +void HTMLStyleElement::updateForStyleScoped() > > Would it work better if we name it as a notification to react and combine into one method?: styleScopedUpdated(attribute.isNull()); Done. > > > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:28 > > + debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('color')); /* red */ > > + debug("E: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */ > > + debug("F: " + document.defaultView.getComputedStyle(f, null).getPropertyValue('color')); /* black */ > > + debug("F: " + document.defaultView.getComputedStyle(e, null).getPropertyValue('border')); /* blue 1px border */ > > this can be a function. Done. > > > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:35 > > + document.body.offsetLeft; > > Why do we need this here? > > > LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html:42 > > + document.body.offsetLeft; > > .. or here? I see. These offestLefts don't make sense. I moved these offsetLefts to the next line of setAttribute or removeAttribute. Best regards, Takashi Sakamoto

Comment on attachment 145728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145728&action=review Thank you for adding the tests! I am now reasonably confident that when we land this, we have enough coverage to capture regressions in the intended behavior. > Source/WebCore/ChangeLog:6 > + Modified any style in shadow subtree to be treated as a scoped style "style" is not the right reference here. In WebKit (and web platform), the word "style" by itself is ambiguous. In your particular case, it would be good to make sure you are discussing the STYLE element, or HTMLStyleElement. Can you change your references to "style" to either "STYLE element" or "HTMLStyleElement"? > Source/WebCore/ChangeLog:40 > + (WebCore::HTMLStyleElement::updateForStyleScoped): > + Newly added. This method is for updating a style's scope when scoped > + attribute is set to be true. Currently there are the following four > + cases: > + 1, the style is "scoped" and is in document tree, > + 2, the style is "scoped" and is in shadow subtree, > + 3, the style is not "scoped" and is in document tree, and > + 4, the style is not "scoped" and is in shadow subtree. > + If the style is not in document, just updating only m_scoped. > + And this method treats the case 3 to 1 and 4 to 2. In both case, > + the style must be registered with scoping node. However > + in 4 to 2, the style has been already registered with shadow root. > + So firstly the style must be unregistered with shadow root. > + (WebCore::HTMLStyleElement::updateForStyleNotScoped): > + Newly added. This method is for updating a style's scope when scoped > + attribute is set to be null, i.e. false. The method treats > + the case 1 to 3 and 2 to 4. This is no longer relevant. Can you update the entry? I would recommend moving the problem description (more general) out of the file modification explanations (more specific). > Source/WebCore/html/HTMLStyleElement.cpp:54 > - , m_isRegisteredWithScopingNode(false) > + , m_scoped(false) Having both m_scoped and scoped() in the same class, and scoped() not reflecting the value of m_scoped is bad. This code will be hard to understand. It seems that Roland had a good name that matched what was happening and didn't clash with scoped(). Also, when I suggested combining them, I didn't realize you'd just rip one of them out like that. The m_isRegisteredWithScopingNode serves an important purpose: it is almost always equal to scoped(), except for the times when the attribute was set (or unset), but the underlying style-scoped machinery has not yet been adjusted. In these cases, we can rely on m_isRegisteredWithScopingNode to determine the actual state. It seems to me that the correct solution is to keep m_isRegisteredWithScopingNode and rely on isInShadowTree to detect cases 2 and 4 from your problem description. > Source/WebCore/html/HTMLStyleElement.cpp:-62 > - // During tear-down, willRemove isn't called, so m_isRegisteredWithScopingNode may still be set here. > - // Therefore we can't ASSERT(!m_isRegisteredWithScopingNode). This comment is ok to remove. > Source/WebCore/html/HTMLStyleElement.cpp:89 > +void HTMLStyleElement::styleScopedUpdated(bool scoped) I apologize for leading you to the wrong name, I now see it. This should be scopedAttributeChanged. Since we are already on HTMLStyleElement, we don't need "style" in the name, and mentioning "attribute" describes it precisely. > Source/WebCore/html/HTMLStyleElement.cpp:100 > + // Note: As any style in a shadow tree is treated as "scoped", "Note:" is superfluous. > Source/WebCore/html/HTMLStyleElement.cpp:106 > + registerWithScopingNode(parentNode()); Can just early return here. > Source/WebCore/html/HTMLStyleElement.cpp:-158 > - if (scoped() && !m_isRegisteredWithScopingNode) Dropping this condition seems wrong. Are you sure your new condition is its equivalent? > Source/WebCore/html/HTMLStyleElement.cpp:172 > + ContainerNode* scope = m_scoped? parentNode() : shadowRoot(); need space between "m_scoped" and the "?".

Thank you for reviewing. I will recreate a patch. However I would like to ask one question. We cannot rely on inDocument() to determine whether style elements are registered or not? (In reply to comment #5) > Having both m_scoped and scoped() in the same class, and scoped() not reflecting the value of m_scoped is bad. This code will be hard to understand. It seems that Roland had a good name that matched what was happening and didn't clash with scoped(). > > Also, when I suggested combining them, I didn't realize you'd just rip one of them out like that. The m_isRegisteredWithScopingNode serves an important purpose: it is almost always equal to scoped(), except for the times when the attribute was set (or unset), but the underlying style-scoped machinery has not yet been adjusted. In these cases, we can rely on m_isRegisteredWithScopingNode to determine the actual state. > > It seems to me that the correct solution is to keep m_isRegisteredWithScopingNode and rely on isInShadowTree to detect cases 2 and 4 from your problem description. If using scoped() and m_isRegisteredWithScopingNode in the same way (=the original code's way), I think, we cannot determine where the style was registered when scoped attribute is changed to be true. I tested this by the following example: <style id="mystyle" scoped>div { color: red };</style> var style = document.getElementById("mystyle"); style.setAttribute("scoped", "a"); // parseAttribute is invoked with !value.isNull style.setAttribute("scoped", "b"); // parseAttribute is invoked with !value.isNull style.setAttribute("scoped", "c"); // parseAttribute is invoked with !value.isNull style.setAttribute("scoped", "d"); // parseAttribute is invoked with !value.isNull During HTMLStyleElement::parseAttribute, we can only know whether currently scoped or not and whether currently in shadow tree or not. So we cannot determine: (a) [old] in shadow DOM subtree and not scoped --> [new] in shadow DOM subtree and scoped (b) [old] in shadow DOM subtree and scoped --> [new] in shadow DOM subtree and scoped In the case (b), we don't need to update. But in the case (a), we have to remove the style from shadow root. However in the both case, m_isRegisteredWithScopingNode is true, scoped() is true and isInShadowTree() is true. This is the reason why I removed m_isRegisteredWithScopingNode and I added the layout test, setAttribute("scoped", "scoped") for already scoped style. I think, there are several ways to fix the above issue: (1) modifying scoped() to see m_scoped, i.e. bool scoped() { return m_scoped; } (2) modifying the type of m_isRegisteredWithScopingNode from bool to enum { NotRegistered, RegisteredAsScoped, RegisteredInShadowTree }. (3) adding a new boolean flag to find where a style element was registered, e.g. m_isRegisteredInShadowRoot or something. However I saw comments: “we cannot rely on scoped()”. I think, what we need is just scoped() that we can rely on, i.e. m_scoped. > > Source/WebCore/html/HTMLStyleElement.cpp:-158 > > - if (scoped() && !m_isRegisteredWithScopingNode) > > Dropping this condition seems wrong. Are you sure your new condition is its equivalent? It depends on “whether we want to register <style scoped> even if it's not inDocument”. If we don’t need to register <style scoped> if its’ not inDocument, I think, m_isRegisteredWithScopingNode is always false when the <style scoped> has been just inserted into document. So, > > Source/WebCore/html/HTMLStyleElement.cpp:-158 > > - if (scoped() && !m_isRegisteredWithScopingNode) is the same as > > - if (scoped()) Now <style> in shadow subtree is also treated as <style scoped>, > > - if (scoped() || isInShadowTree()) I agree that my patch depends on insertedInto and removedFrom methods’ behavior. In my understandings, insertedInto and removedFrom method can tell the time when a style element not in document is inserted into document and the time when a style in document is removed from document. Best regards, Takashi Sakamoto

(In reply to comment #6) > Thank you for reviewing. > > I will recreate a patch. However I would like to ask one question. We cannot rely on inDocument() to determine whether style elements are registered or not? I am not sure what you mean by "style elements are registered" here. If you mean "registered with a scoping node", then obviously no. when inDocument() == false, the node should not be registered with any scoping node, but when inDocument() == true, it may or may not be registered with a scoping node. If you mean something else, you may have to explain it a bit more :) > If using scoped() and m_isRegisteredWithScopingNode in the same way (=the original code's way), I think, we cannot determine where the style was registered when scoped attribute is changed to be true. I tested this by the following example: > > <style id="mystyle" scoped>div { color: red };</style> > > var style = document.getElementById("mystyle"); > style.setAttribute("scoped", "a"); // parseAttribute is invoked with !value.isNull > style.setAttribute("scoped", "b"); // parseAttribute is invoked with !value.isNull > style.setAttribute("scoped", "c"); // parseAttribute is invoked with !value.isNull > style.setAttribute("scoped", "d"); // parseAttribute is invoked with !value.isNull I don't understand what you're testing here. Can you explain a bit more? > > During HTMLStyleElement::parseAttribute, we can only know whether currently scoped or not and whether currently in shadow tree or not. So we cannot determine: > > (a) [old] in shadow DOM subtree and not scoped --> [new] in shadow DOM subtree and scoped > (b) [old] in shadow DOM subtree and scoped --> [new] in shadow DOM subtree and scoped > > In the case (b), we don't need to update. But in the case (a), we have to remove the style from shadow root. However in the both case, m_isRegisteredWithScopingNode is true, scoped() is true and isInShadowTree() is true. So, what you need is a knowledge of how the scoping came about: whether by virtue of just being in shadow DOM subtree, or with a scoped attribute. I think I understand. > > This is the reason why I removed m_isRegisteredWithScopingNode and I added the layout test, setAttribute("scoped", "scoped") for already scoped style. > > I think, there are several ways to fix the above issue: > (1) modifying scoped() to see m_scoped, i.e. bool scoped() { return m_scoped; } This is wrong. scoped() should always reflect the attribute value. > (2) modifying the type of m_isRegisteredWithScopingNode from bool to enum { NotRegistered, RegisteredAsScoped, RegisteredInShadowTree }. This seems good. > (3) adding a new boolean flag to find where a style element was registered, e.g. m_isRegisteredInShadowRoot or something. Here, you will always have one invalid combination, which seems like a bad thing. Thank you for explaining this! I am glad you're tackling such complex problem and making progress. Keep going! :)

Created attachment 146465 [details] Patch

Thank you for answering my question. (In reply to comment #7) > (In reply to comment #6) > > Thank you for reviewing. > > > > I will recreate a patch. However I would like to ask one question. We cannot rely on inDocument() to determine whether style elements are registered or not? > > I am not sure what you mean by "style elements are registered" here. If you mean "registered with a scoping node", then obviously no. when inDocument() == false, the node should not be registered with any scoping node, but when inDocument() == true, it may or may not be registered with a scoping node. > > If you mean something else, you may have to explain it a bit more :) > > > If using scoped() and m_isRegisteredWithScopingNode in the same way (=the original code's way), I think, we cannot determine where the style was registered when scoped attribute is changed to be true. I tested this by the following example: > > > > <style id="mystyle" scoped>div { color: red };</style> > > > > var style = document.getElementById("mystyle"); > > style.setAttribute("scoped", "a"); // parseAttribute is invoked with !value.isNull > > style.setAttribute("scoped", "b"); // parseAttribute is invoked with !value.isNull > > style.setAttribute("scoped", "c"); // parseAttribute is invoked with !value.isNull > > style.setAttribute("scoped", "d"); // parseAttribute is invoked with !value.isNull > > I don't understand what you're testing here. Can you explain a bit more? I mean, if I can expect that parseAttribute is invoked if and only if an attribute value is actually changed, i.e. from false to true or from true to false. If so, we can determine old scoped value from current scoped value. However, the above test shows that it doesn't depend on whether scoped attribute's bool value changes or not. > > During HTMLStyleElement::parseAttribute, we can only know whether currently scoped or not and whether currently in shadow tree or not. So we cannot determine: > > > > (a) [old] in shadow DOM subtree and not scoped --> [new] in shadow DOM subtree and scoped > > (b) [old] in shadow DOM subtree and scoped --> [new] in shadow DOM subtree and scoped > > > > In the case (b), we don't need to update. But in the case (a), we have to remove the style from shadow root. However in the both case, m_isRegisteredWithScopingNode is true, scoped() is true and isInShadowTree() is true. > > So, what you need is a knowledge of how the scoping came about: whether by virtue of just being in shadow DOM subtree, or with a scoped attribute. I think I understand. > > > > > This is the reason why I removed m_isRegisteredWithScopingNode and I added the layout test, setAttribute("scoped", "scoped") for already scoped style. > > > > I think, there are several ways to fix the above issue: > > (1) modifying scoped() to see m_scoped, i.e. bool scoped() { return m_scoped; } > > This is wrong. scoped() should always reflect the attribute value. > > > (2) modifying the type of m_isRegisteredWithScopingNode from bool to enum { NotRegistered, RegisteredAsScoped, RegisteredInShadowTree }. > > This seems good. > > > (3) adding a new boolean flag to find where a style element was registered, e.g. m_isRegisteredInShadowRoot or something. > > Here, you will always have one invalid combination, which seems like a bad thing. I see. I added the enum.

(In reply to comment #9) > > > <style id="mystyle" scoped>div { color: red };</style> > > > > > > var style = document.getElementById("mystyle"); > > > style.setAttribute("scoped", "a"); // parseAttribute is invoked with !value.isNull > > > style.setAttribute("scoped", "b"); // parseAttribute is invoked with !value.isNull > > > style.setAttribute("scoped", "c"); // parseAttribute is invoked with !value.isNull > > > style.setAttribute("scoped", "d"); // parseAttribute is invoked with !value.isNull > > > > I don't understand what you're testing here. Can you explain a bit more? > > I mean, if I can expect that parseAttribute is invoked if and only if an attribute value is actually changed, i.e. from false to true or from true to false. If so, we can determine old scoped value from current scoped value. > However, the above test shows that it doesn't depend on whether scoped attribute's bool value changes or not. Hmm.. The test above never sets the value of the attribute to "false". Remember, this is a boolean attribute (http://www.whatwg.org/specs/web-apps/current-work/multipage/common-microsyntaxes.html#boolean-attributes). Only removing the attribute will qualify as "false".

Comment on attachment 146465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146465&action=review This turned out to be a sweet patch. Thanks for your hard work, tasak@! A few nits that you are welcome to ignore (except spelling errors :). > Source/WebCore/ChangeLog:41 > + method is invoked. Therefor we cannot rely on scoped()" Therefor -> therefore > Source/WebCore/ChangeLog:58 > + Added one new method scopedAttributeChanged's decalartion and decalartion -> declaration > Source/WebCore/html/HTMLStyleElement.h:89 > + enum ScopedStyleRegisterType { ScopedStyleRegistrationState > Source/WebCore/html/HTMLStyleElement.h:94 > + ScopedStyleRegisterType m_registeredWithScopingNode; m_scopedStyleRegistrationState

Created attachment 146776 [details] Patch

Thank you. I fixed the errors. Best regards, Takashi Sakamoto (In reply to comment #11) > (From update of attachment 146465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146465&action=review > > This turned out to be a sweet patch. Thanks for your hard work, tasak@! A few nits that you are welcome to ignore (except spelling errors :). > > > Source/WebCore/ChangeLog:41 > > + method is invoked. Therefor we cannot rely on scoped()" > > Therefor -> therefore > > > Source/WebCore/ChangeLog:58 > > + Added one new method scopedAttributeChanged's decalartion and > > decalartion -> declaration > > > Source/WebCore/html/HTMLStyleElement.h:89 > > + enum ScopedStyleRegisterType { > > ScopedStyleRegistrationState > > > Source/WebCore/html/HTMLStyleElement.h:94 > > + ScopedStyleRegisterType m_registeredWithScopingNode; > > m_scopedStyleRegistrationState

Comment on attachment 146776 [details] Patch Rejecting attachment 146776 [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: Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 44>At revision 134927. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/12944483

Comment on attachment 146776 [details] Patch Clearing flags on attachment: 146776 Committed r120062: <http://trac.webkit.org/changeset/120062>

All reviewed patches have been landed. Closing bug.