Bug 77610 - Introduces BuiltinShadowRoot and methods.
Summary: Introduces BuiltinShadowRoot and methods.
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77603
  Show dependency treegraph
 
Reported: 2012-02-01 23:23 PST by Shinya Kawanaka
Modified: 2012-02-08 09:19 PST (History)
8 users (show)

See Also:


Attachments
Patch (36.49 KB, patch)
2012-02-05 22:36 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (40.54 KB, patch)
2012-02-05 23:17 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2012-02-01 23:23:25 PST
Most of elements which use shadow root touch only built-in shadow root rather than user generated shadow root.
We should have a method to support it.
Comment 1 Shinya Kawanaka 2012-02-05 22:36:57 PST
Created attachment 125576 [details]
Patch
Comment 2 Shinya Kawanaka 2012-02-05 23:16:47 PST
Ah, I forgot to remove Element::ensureShadowRoot from symbol export lists.
Comment 3 Shinya Kawanaka 2012-02-05 23:17:34 PST
Created attachment 125579 [details]
Patch
Comment 4 Dominic Cooney 2012-02-05 23:29:23 PST
Comment on attachment 125579 [details]
Patch

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

> Source/WebCore/ChangeLog:44
> +        (WebCore::ColorInputType::createShadowSubtree):

Would it be simpler to, instead of introducing a new BuiltinShadowRoot type and new methods for built-in shadows, to just have elements which have built-in shadows like *InputType, HTMLDetailsElement, HTMLKeygenElement, HTMLMediaElement, HTMLTextAreaElement, etc. have a member that points to its built-in shadow root? It avoids the virtual method call, avoids new API, avoids a new type, etc.

You can leave the shadow root mechanism to manage the refcount of the built-in shadow root, since there is no way to remove them, and just use a raw pointer to the built-in shadow root.
Comment 5 Shinya Kawanaka 2012-02-06 00:45:03 PST
Comment on attachment 125579 [details]
Patch

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

>> Source/WebCore/ChangeLog:44
>> +        (WebCore::ColorInputType::createShadowSubtree):
> 
> Would it be simpler to, instead of introducing a new BuiltinShadowRoot type and new methods for built-in shadows, to just have elements which have built-in shadows like *InputType, HTMLDetailsElement, HTMLKeygenElement, HTMLMediaElement, HTMLTextAreaElement, etc. have a member that points to its built-in shadow root? It avoids the virtual method call, avoids new API, avoids a new type, etc.
> 
> You can leave the shadow root mechanism to manage the refcount of the built-in shadow root, since there is no way to remove them, and just use a raw pointer to the built-in shadow root.

Let me mention that there is a case that a shadow root is recreated now. For example, input element recreates shadow root when its type attribute is changed.
So we have to have a new API to insert/remove built-in shadow root now. Otherwise we have to change HTMLInputElement behavior.

This patch is a first step to support shadow subtree. Before adding multiple shadow subtrees, we must have a way to distinguish built-in shadow root from user generated shadow root because currently they are mixed. It is very confusing.

In my plan,
  (1) distinguish built-in and user generated shadow root.
  (2) allow us to have multiple shadow subtrees (with built-in shadow root and generated shadow root clearly separated)
  (3) removes ensureShadowRoot.
  (3) merge them if possible.

Maybe we can have a pointer to a built-in shadow root, however, I don't want to do in this patch. If we introduce such a pointer in this patch, we cannot share ensureBuiltinShadowRoot method. This will make this patch complex. I have a plan to kill this method later, so let's do that in later patch (3).
Comment 6 Dimitri Glazkov (Google) 2012-02-06 09:30:38 PST
Generally, I am not convinced that we need to differentiate between "built-in" and user shadow DOM subtrees. If there are some special cases where the shadow DOM subtrees are added and removed, we need to understand what this means in the general shadow DOM perspective. For example, in the case of decorators, we'll have a very similar situation.

Also, the "built-in" terminology seems a bit off. Perhaps we should seek similarity with styles and call them User Agent shadow DOM subtrees?
Comment 7 Dimitri Glazkov (Google) 2012-02-06 09:33:05 PST
(In reply to comment #5)
> (From update of attachment 125579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125579&action=review

> Let me mention that there is a case that a shadow root is recreated now. For example, input element recreates shadow root when its type attribute is changed.
> So we have to have a new API to insert/remove built-in shadow root now. Otherwise we have to change HTMLInputElement behavior.

Does this not seem like an idea worth pursuing? Don't get me wrong, I don't want to block your progress. But if we can't _explain_ things as trivial as input elements in terms the current Shadow DOM spec, perhaps this should be viewed as a deficiency of the spec, not an implementation quirk? Just thinking outloud here. WDYT?
Comment 8 Hajime Morrita 2012-02-06 16:19:55 PST
(In reply to comment #7)
> Does this not seem like an idea worth pursuing? Don't get me wrong, I don't want to block your progress. But if we can't _explain_ things as trivial as input elements in terms the current Shadow DOM spec, perhaps this should be viewed as a deficiency of the spec, not an implementation quirk? Just thinking outloud here. WDYT?

TL;DR:
- Let's start from more trivial shadow implementations like <media> and <summary>
   which doesn't swap shadows dynamically. Then come back here.
- Allowing access to non-last ShadowRoot looks reasonable. But allowing to replace it 
   may be doubtful.

---

Some existing elements, especially <input> family, are implemented on an shadow dom concept
of early day's, which allows us to replace shadow DOM, doesn't have multiple shadows, etc. 
Such conceptual gaps make it tricky to adopt multiple shadows.

Ideally, we should refactor stuff to match the latest model. It will be easy for most of 
WebCore internal use. But maybe <input> is harder than average. 
At least we need to reshape the <input> shadow trees.

On introducing new member that Dominic mentioned, it's conceptually more natural,
that means it doesn't need any backdoor.
But in C++, maintaing any extra reference tends to cause a trouble.

So I prefer to allow  C++ implementation only to reach ShadowRoot chain to get "hidden" pointer,
but not to allow replacing such ShadowRoots dynamically. that looks an abuse of WebCore authority. 
It's like a __proto__ in JavaScript. We should be cautious about issuing it.

But anyway, we can start from more trivial part like <media> and <summary>.
The multiple shadows concept is a wild where anyone hasn't stepped in.
We need to learn how it should be programmed by dogfooding it ;-)
Comment 9 Shinya Kawanaka 2012-02-06 18:43:12 PST
OK...

Let me try to make more steps.
I'll create bugs for the steps soon.
Comment 10 Dimitri Glazkov (Google) 2012-02-08 09:19:14 PST
Comment on attachment 125579 [details]
Patch

removing the patch from the review queue for now.