Bug 77936 – HTMLMediaElement shouldn't use Element::ensureShadowRoot

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

It's difficult to support multiple shadow subtrees if a shadow root is recreated.
We should change this.

[Shinya Kawanaka]

We would like to eliminate Element::ensureShadowRoot().
HTMLMediaElement should not use it, too.

[Shinya Kawanaka]

Created attachment 151823 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151823 [details]
Patch

Attachment 151823 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13207474

New failing tests:
fast/dom/shadow/shadow-disable.html

[WebKit Review Bot]

Created attachment 151841 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Shinya Kawanaka]

Created attachment 151856 [details]
Patch

[Shinya Kawanaka]

Created attachment 151898 [details]
Patch

[Eric Carlson]

Comment on attachment 151898 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        <video> and <audio> add UserAgentShadowRoot dynamically, and assume that it's the oldest ShadowRoot.
> +        However, before <video> and <audio> add UserAgentShadowRoot, an AuthorShadowRoot can be added by a user.
> +        It breaks the assumption that the UserAgentShadowRoot is the oldest.
> +
> +        When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot.
> +        So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to
> +        <video> and <audio>.

I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list?

> Source/WebCore/ChangeLog:26
> +        * dom/Element.h:
> +        (WebCore::Element::willAddAuthorShadowRoot):
> +        * dom/ElementShadow.cpp:
> +        (WebCore::ElementShadow::addShadowRoot):
> +        * dom/ElementShadow.h:
> +        (ElementShadow):
> +        * dom/ShadowRoot.cpp:
> +        (WebCore::allowsAuthorShadowRoot):
> +        (WebCore::ShadowRoot::create):

This is a bug about HTMLMediaElement, so it seems like these generic changes should be made in separate fix/patch.

> LayoutTests/fast/dom/shadow/shadowdom-for-media.html:22
> +var shadowRootForVideoWithControls = addShadowDOM(videoWithControls);
> +var oldestShadowRootForVideoWithControls = internals.oldestShadowRoot(videoWithControls);
> +var youngerShadowRootForVideoWithControls = internals.youngerShadowRoot(oldestShadowRootForVideoWithControls);

Ditto my question from above about assuming the purpose/identity/structure of an element based on its position in the list - why?

[Shinya Kawanaka]

(In reply to comment #7)
> (From update of attachment 151898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151898&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        <video> and <audio> add UserAgentShadowRoot dynamically, and assume that it's the oldest ShadowRoot.
> > +        However, before <video> and <audio> add UserAgentShadowRoot, an AuthorShadowRoot can be added by a user.
> > +        It breaks the assumption that the UserAgentShadowRoot is the oldest.
> > +
> > +        When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot.
> > +        So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to
> > +        <video> and <audio>.
> 
> I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list?

The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of  AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so.

More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM.


> 
> > Source/WebCore/ChangeLog:26
> > +        * dom/Element.h:
> > +        (WebCore::Element::willAddAuthorShadowRoot):
> > +        * dom/ElementShadow.cpp:
> > +        (WebCore::ElementShadow::addShadowRoot):
> > +        * dom/ElementShadow.h:
> > +        (ElementShadow):
> > +        * dom/ShadowRoot.cpp:
> > +        (WebCore::allowsAuthorShadowRoot):
> > +        (WebCore::ShadowRoot::create):
> 
> This is a bug about HTMLMediaElement, so it seems like these generic changes should be made in separate fix/patch.

Right.

> 
> > LayoutTests/fast/dom/shadow/shadowdom-for-media.html:22
> > +var shadowRootForVideoWithControls = addShadowDOM(videoWithControls);
> > +var oldestShadowRootForVideoWithControls = internals.oldestShadowRoot(videoWithControls);
> > +var youngerShadowRootForVideoWithControls = internals.youngerShadowRoot(oldestShadowRootForVideoWithControls);
> 
> Ditto my question from above about assuming the purpose/identity/structure of an element based on its position in the list - why?

[Shinya Kawanaka]

Created attachment 152105 [details]
Patch

[Hajime Morrita]

@shinya I guess some diagram would definitely help your explanation
to be understood.

[Shinya Kawanaka]

(In reply to comment #10)
> @shinya I guess some diagram would definitely help your explanation
> to be understood.

https://docs.google.com/a/chromium.org/drawings/d/1rq6JixSqgUpXNNMkGFs8BR2LEg5U0IIyFsooloKjXAU/edit

Added a diagram. I hope it would be helpful.

[Eric Carlson]

Comment on attachment 152105 [details]
Patch

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

> Source/WebCore/ChangeLog:38
> [phone]  Shinya Kawanaka  <[e-mail]>
> +
> +
> +        When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot.
> +        So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to
> +        <video> and <audio>.
> +
> +        Test: fast/dom/shadow/shadowdom-for-media.html
> +

You forgot to remove this from the ChangeLog.

[Eric Carlson]

(In reply to comment #8)
> (In reply to comment #7)
> > 
> > I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list?
> 
> The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of  AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so.
> 
> More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM.

I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. 

My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there?

[Shinya Kawanaka]

(In reply to comment #13)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > 
> > > I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list?
> > 
> > The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of  AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so.
> > 
> > More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM.
> 
> I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. 
> 
> My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there?

Sorry for late response. Actually there is a lot of reasons why I chose this design.
(1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason.
(2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build.
(3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack.

[Shinya Kawanaka]

Created attachment 152670 [details]
Fixed ChangeLog

[Hajime Morrita]

Comment on attachment 152670 [details]
Fixed ChangeLog

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

> Source/WebCore/html/HTMLMediaElement.cpp:4212
> +    if (ElementShadow* elementShadow = shadow())

Is it possible to have shadow() but not to have oldestShadowRoot()?

> Source/WebCore/html/HTMLMediaElement.h:88
> +    void willAddAuthorShadowRoot() OVERRIDE;

Let's say virtual to align the code around.

[Shinya Kawanaka]

Comment on attachment 152670 [details]
Fixed ChangeLog

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

>> Source/WebCore/html/HTMLMediaElement.cpp:4212
>> +    if (ElementShadow* elementShadow = shadow())
> 
> Is it possible to have shadow() but not to have oldestShadowRoot()?

Yes in some timing, but it won't happen in this case...

>> Source/WebCore/html/HTMLMediaElement.h:88
>> +    void willAddAuthorShadowRoot() OVERRIDE;
> 
> Let's say virtual to align the code around.

Right.

[Shinya Kawanaka]

Created attachment 152696 [details]
Patch

[Hajime Morrita]

Comment on attachment 152696 [details]
Patch

Looks like reaching a good shape.
I hope "create shadow if not there yet" pattern to be factored out somehow.
But it could be done by a separate patch.

[WebKit Review Bot]

Comment on attachment 152696 [details]
Patch

Clearing flags on attachment: 152696

Committed r122816: <http://trac.webkit.org/changeset/122816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

(In reply to comment #17)
> (From update of attachment 152670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152670&action=review
> 
> >> Source/WebCore/html/HTMLMediaElement.cpp:4212
> >> +    if (ElementShadow* elementShadow = shadow())
> > 
> > Is it possible to have shadow() but not to have oldestShadowRoot()?
> 
> Yes in some timing, but it won't happen in this case...
> 
  Why? It may be clear to you, but it obviously isn't to others, please clarify.

[Eric Carlson]

(In reply to comment #14)
> (In reply to comment #13)
> > I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. 
> > 
> > My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there?
> 
> Sorry for late response. Actually there is a lot of reasons why I chose this design.
> (1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason.

Where does the spec say that? The only thing I see is say specifically about <video> and <audio> is that the subtree "Contains no insertion points"[1]. It also says that all HTML elements "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here. Why?


> (2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build.

Wait, isn't this patch necessary *because* UserAgentShadowRoot is special (different)? 


> (3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack.

Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? 

I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does.

So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. 


[1] http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#html-elements-and-their-shadow-dom-subtrees

[Dimitri Glazkov (Google)]

Comment on attachment 152696 [details]
Patch

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

Please let's make sure that we don't make assumptions about the position of UA shadow root.

I would even suggest that we always create an empty shadow root in HTMLMediaElement upon instantiation to conform with the spec.

> Source/WebCore/html/HTMLMediaElement.cpp:4212
> +        createShadowSubtree();

This seems wrong. This:

var video = document.createElement("video");
var r00t = new WebKitShadowRoot(video);
video.controls = true;

will result in r00t being the oldest shadow root and tripping the ASSERT. Right?

[Hajime Morrita]

Comment on attachment 152696 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:4212
>> +        createShadowSubtree();
> 
> This seems wrong. This:
> 
> var video = document.createElement("video");
> var r00t = new WebKitShadowRoot(video);
> video.controls = true;
> 
> will result in r00t being the oldest shadow root and tripping the ASSERT. Right?

No. willAddShadowRoot() guarantees the host to have UA shadow root in the oldest position.
This ensure-ua-shadow-before-any-author-shadows pattern is the foundation for
making replaced elements shadow-ready.

[Hajime Morrita]

Hi Eric,

I'm sorry for r+ing this before your feedback.
Let me explain the rationale that forces ua-shadows being the first(oldest) shadow:

At first, any UA shadow should be the oldest if there is such a shadow on the host element.
This is because author shadow should be able to  "decorate" it like adding extra container
element or additional buttons around the original control.
And only younger shadows can decorate the older.

This patch makes sure this contraint by ensuring UA shadow just before the first author
shadow arrives, which is done in willAddShadowRoot().
Possible alternative is to insert the UA shadow before the existing author shadows.
But it's tricky because ShadowRoot takes append-only design and there are assumptions
that ShadowRoot is never removed once it is added to the host element.

For this reason, UA shadow need to be the oldest. So what is the best way to find the UA shadow
in the shadow list?

In my understanding, your point is that we could have some explicit state to indicate the fact.
Actually the patch contains a debug-only state for that purpose. We could use it not only for debug
but also finding an ua shadow.

On the other hand, I want to ensure the position invariant of UA shadow. 
If we search an UA shadow by inspecting its type, this invariant needs to be checked separately.
This isn't very robust. After all, we need either
A. to find ua shadow by type and assert the position invariant, or
B. to find ua shadow by position and assert the type invariant.

And this patch picks B. I don't think there is significant difference between these two approach.

Well,  in a second thought, I noticed that both A and Be can be felt fragile
as long as such assertion is done by each caller site.
I think we can introduce ElementShadow::userAgentShadowRoot()
and having such an assert in that method. It will be more robust and intention revealing.
I filed Bug 91564 for that.

Does this make sense?

[Shinya Kawanaka]

Hi Eric,
though morrita has already answered some of your questions, but I would like to answer to you, too. (Just I want to explain what I did). I'm sorry my explanation was poor before.

(In reply to comment #23)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. 
> > > 
> > > My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there?
> > 
> > Sorry for late response. Actually there is a lot of reasons why I chose this design.
> > (1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason.
> 
> Where does the spec say that? The only thing I see is say specifically about <video> and <audio> is that the subtree "Contains no insertion points"[1]. It also says that all HTML elements "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here. Why?
> 
> 
> > (2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build.
> 
> Wait, isn't this patch necessary *because* UserAgentShadowRoot is special (different)? 

For (1), (2):

> "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here

Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory.
So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did.

>  isn't this patch necessary *because* UserAgentShadowRoot is special (different)? 

And... in some meaning, UserAgentShadowRoot is different from AuthorShadowRoot. This is absolutely right.
But we would like to treat with as the same as possible. So we use its type only for the ShadowRoot instantiation. After ShadowRoot is instantiated, we don't use its type.

And it is right this patch is necessary because UserAgentShadoweRoot is special. Right.
But the reason why UserAgentShadowRoot is special is that it should be instantiated when the host is instantiated. 

> 
> 
> > (3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack.
> 
> Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? 
> 
> I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does.
> 
> So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. 

I understand that it is not so wise. However, since we decided not to use the type of ShadowRoot in release, I don't have any option to determine the type by position.
I think you said we should have ShadowRoot type and we should use it. But as morrita said, I don't think there is not significant difference...

Also... as morrita said, we woud like introduce userAgentShadowRoot() to make our code intention clearer. I hope this will solve your anticipation...


> 
> 
> [1] http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#html-elements-and-their-shadow-dom-subtrees

[Eric Carlson]

Morrita -

(In reply to comment #26)
> 
> I'm sorry for r+ing this before your feedback.
> Let me explain the rationale that forces ua-shadows being the first(oldest) shadow:
> 
> At first, any UA shadow should be the oldest if there is such a shadow on the host element.
> This is because author shadow should be able to  "decorate" it like adding extra container
> element or additional buttons around the original control.
> And only younger shadows can decorate the older.
> 
> This patch makes sure this contraint by ensuring UA shadow just before the first author
> shadow arrives, which is done in willAddShadowRoot().
> Possible alternative is to insert the UA shadow before the existing author shadows.
> But it's tricky because ShadowRoot takes append-only design and there are assumptions
> that ShadowRoot is never removed once it is added to the host element.
> 
> For this reason, UA shadow need to be the oldest. So what is the best way to find the UA shadow
> in the shadow list?
> 
> In my understanding, your point is that we could have some explicit state to indicate the fact.
> Actually the patch contains a debug-only state for that purpose. We could use it not only for debug
> but also finding an ua shadow.
> 
> On the other hand, I want to ensure the position invariant of UA shadow. 
> If we search an UA shadow by inspecting its type, this invariant needs to be checked separately.
> This isn't very robust. After all, we need either
> A. to find ua shadow by type and assert the position invariant, or
> B. to find ua shadow by position and assert the type invariant.
> 
> And this patch picks B. I don't think there is significant difference between these two approach.
> 
> Well,  in a second thought, I noticed that both A and Be can be felt fragile
> as long as such assertion is done by each caller site.
> I think we can introduce ElementShadow::userAgentShadowRoot()
> and having such an assert in that method. It will be more robust and intention revealing.
> I filed Bug 91564 for that.
> 
> Does this make sense?

My concern has been with the pervasive use of "oldest" and "younger" - implementation details - throughout the code and tests, so my thought to use the debug-only 'type' field was just a suggestion. Encapsulating the use of position with a method like ElementShadow::userAgentShadowRoot (with appropriate assertions) sounds like a great solution.

Thank you for taking the time to think about my concerns and for your reply!

[Eric Carlson]

(In reply to comment #27)
> (In reply to comment #23)
> Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory.
> So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did.
> 
I understand why it is done lazily, but that is contrary to the spec text. If the spec says something that doesn't make sense, we might decide to do something else but we should *also* try to get the spec changed (fixed). Have you done this? I think you know where to find the spec editor :-).


> > Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? 
> > 
> > I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does.
> > 
> > So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. 
> 
> I understand that it is not so wise. However, since we decided not to use the type of ShadowRoot in release, I don't have any option to determine the type by position.
> 
First, let me say that I am happy with Morrita's suggested solution.  

I have to point out, however, that your argument here is nonsense. Saying that you "don't have any option" because of a decision that *you* made is circular reasoning, it is a logical fallacy. You decided not to include the ShadowRoot type in a release build, so *you* could change that decision.

[Dimitri Glazkov (Google)]

(In reply to comment #29)
> (In reply to comment #27)
> > (In reply to comment #23)
> > Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory.
> > So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did.
> > 
> I understand why it is done lazily, but that is contrary to the spec text. If the spec says something that doesn't make sense, we might decide to do something else but we should *also* try to get the spec changed (fixed). Have you done this? I think you know where to find the spec editor :-).

To be fair, the way I wrote this is using the "equivalence" language. In other words, as long as the lazy instantiation is not detectable from script, we're satisfying the spec text.