[Webkit-unassigned] [Bug 87805] [Shadow DOM] <style> inside Shadow subtree should be scoped inside the subtree.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=87805





--- Comment #6 from Takashi Sakamoto <tasak at google.com>  2012-06-05 23:33:08 PST ---
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.