Bug 94274 - Move Shadow DOM inspection feature out from experiments
Summary: Move Shadow DOM inspection feature out from experiments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks: 62220 94082
  Show dependency treegraph
 
Reported: 2012-08-16 17:36 PDT by Hajime Morrita
Modified: 2012-09-28 01:41 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.08 KB, patch)
2012-09-05 04:00 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2012-09-05 04:42 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (4.03 KB, patch)
2012-09-05 22:56 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (6.74 KB, patch)
2012-09-06 01:13 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (4.15 KB, patch)
2012-09-06 23:41 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch for landing (5.18 KB, patch)
2012-09-28 01:04 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-08-16 17:36:59 PDT
To ship Shadow DOM by default, it would be great if we can get the inspector support without enabling experimental feature.
Since Shadow DOM is only enabled by a few ports, probably it needs to be hidden when the Shadow DOM flag is turned off.
Comment 1 Pavel Feldman 2012-08-17 05:19:10 PDT
If shadow dom is not supported on the backend, it won't populate corresponding payloads. As a result, shadow dom will be missing in the UI. When you have actions such as "create shadow dom" that need to know whether shadow dom is supported, you can add a protocol method "DOM.supportsShadowDOM" and query it prior to that action.
Comment 2 Dimitri Glazkov (Google) 2012-08-17 09:36:30 PDT
Cool, "Create Shadow Root" action! I haven't even thought about that. And I like the DOM.supportsShadowDOM flag idea.

Technically, since the only thing that the Shadow DOM API flag is just flipping on the public-facing API (the plumbing still works), the Inspector could still support Shadow DOM, even if it is disabled. It could even be limited to inspection of the built-in elements, but still useful in that regard.

Oh, two things I just remembered:

1) the inspector user shouldn't  _ever_ be able to edit user agent shadow roots
2) consider exposing the pseudos for built-in elements in inspector. They aren't interesting for Shadow DOM API itself, but they are very useful for web devs who hack on built-in controls.
Comment 3 Hajime Morrita 2012-08-19 17:53:30 PDT
I'm not sure if it's worth exposing built-in Shadow DOMs if the are 
no programmable Shadow DOM API - it might feel just a noise.
But I understand that it's harmless to show it.

My feeling is
- Having DOM.supportsShadowDOM is nice especially for more powerful tool support
  in the future. 
- even though it isn't mandated for shipping Shadow DOM.
Comment 4 Hayato Ito 2012-08-19 18:01:44 PDT
I am wondering why we need DOM.supportsShadowDOM.
I guess DOM.supportsXXX is a established convention, but the following is enough, isn't it?

var supportsShadowDOM = window.WebKitShadowRoot || window.ShadowRoot
Comment 5 Hajime Morrita 2012-08-19 18:10:49 PDT
> I am wondering why we need DOM.supportsShadowDOM.
> I guess DOM.supportsXXX is a established convention, but the following is enough, isn't it?
> 
> var supportsShadowDOM = window.WebKitShadowRoot || window.ShadowRoot
It's safer to tell it from C++ side since ShadowDOM availability can be
switched per-frame (domain) and Inspector has its own page/frame
which is different from the page under debug.

But in reality, no browser will use such per-frame flag control and 
it will be OK just to check WebKitShadowRoot :-)
Comment 6 Hayato Ito 2012-08-19 18:37:05 PDT
Got it. Thank you.
I still think a capability test in JS is a most reliable way to judge it. :)

(In reply to comment #5)
> > I am wondering why we need DOM.supportsShadowDOM.
> > I guess DOM.supportsXXX is a established convention, but the following is enough, isn't it?
> > 
> > var supportsShadowDOM = window.WebKitShadowRoot || window.ShadowRoot
> It's safer to tell it from C++ side since ShadowDOM availability can be
> switched per-frame (domain) and Inspector has its own page/frame
> which is different from the page under debug.
> 
> But in reality, no browser will use such per-frame flag control and 
> it will be OK just to check WebKitShadowRoot :-)
Comment 7 Hajime Morrita 2012-09-05 04:00:33 PDT
Created attachment 162212 [details]
Patch
Comment 8 Hajime Morrita 2012-09-05 04:42:22 PDT
Created attachment 162218 [details]
Patch
Comment 9 Dimitri Glazkov (Google) 2012-09-05 08:46:34 PDT
Comment on attachment 162218 [details]
Patch

Should we move the supportsShadowDOM somewhere to a global place?
Comment 10 WebKit Review Bot 2012-09-05 20:07:39 PDT
Comment on attachment 162218 [details]
Patch

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

New failing tests:
inspector/elements/edit-dom-actions.html
Comment 11 Hajime Morrita 2012-09-05 22:56:55 PDT
Created attachment 162423 [details]
Patch
Comment 12 Hajime Morrita 2012-09-05 22:57:57 PDT
> Should we move the supportsShadowDOM somewhere to a global place?
Good point. done.
Comment 13 Pavel Feldman 2012-09-05 23:41:05 PDT
You should never trust JS and always do native checks. 1) User can override JS, 2) front-end will run in a separate browser in case of remote debugging / mobile.

In your case we don't yet expose shadow Dom specific actions, so we don't need such a check at all. Payload for shadow implies there is shadow content, just use it.
Comment 14 Hajime Morrita 2012-09-06 00:21:07 PDT
Hi Pavel, thanks for taking a look!

> You should never trust JS and always do native checks. 1) User can override JS, 2) front-end will run in a separate browser in case of remote debugging / mobile.
> 

Right. I'll add an native check API.

> In your case we don't yet expose shadow Dom specific actions, so we don't need such a check at all. Payload for shadow implies there is shadow content, just use it.

Some elements like <input> can have shadow tree even if Shadow DOM JavaScript API is disabled.
I'm not sure whether it is good idea to expose shadow dom existence even when the developer 
cannot use it through JavaScript API.

Probably we can send the shadow root payload only if the Shadow DOM API is enabled.
Then we can have the check on backend and can get rid of check code on frontend side.
Comment 15 Hajime Morrita 2012-09-06 01:13:09 PDT
Created attachment 162446 [details]
Patch
Comment 16 Pavel Feldman 2012-09-06 02:06:29 PDT
Dmitry, do you want to show input/media shadow when shadow Dom option is checked in the inspector?
Comment 17 Dimitri Glazkov (Google) 2012-09-06 09:35:17 PDT
(In reply to comment #16)
> Dmitry, do you want to show input/media shadow when shadow Dom option is checked in the inspector?

Yes, I think that makes sense. How hard would it be to make them read-only?
Comment 18 Pavel Feldman 2012-09-06 10:16:09 PDT
Making it r/o in elements panel should not be hard - mark DOMNodes as read only in DOMagent.js and mute actions associated with them. Should be < 100LOC.

Making shadow (read only) not appear in $0 also can be handled from the frontend.

Overall, it should not be hard. I wonder if there are more ways to interact with it that need blocking.
Comment 19 Pavel Feldman 2012-09-06 10:20:00 PDT
Comment on attachment 162446 [details]
Patch

As per discussion: this check does not give us much. We should filter shadom Dom out on front-end side when it is not enabled (we should convert experiment into a setting that toggles it instead). And we need to address the r/o roots (thanks to Dimitri for reminding).
Comment 20 Pavel Feldman 2012-09-06 10:20:04 PDT
Comment on attachment 162446 [details]
Patch

As per discussion: this check does not give us much. We should filter shadom Dom out on front-end side when it is not enabled (we should convert experiment into a setting that toggles it instead). And we need to address the r/o roots (thanks to Dimitri for reminding).
Comment 21 Hajime Morrita 2012-09-06 17:53:13 PDT
(In reply to comment #20)
> (From update of attachment 162446 [details])
> As per discussion: this check does not give us much. We should filter shadom Dom out on front-end side when it is not enabled (we should convert experiment into a setting that toggles it instead). And we need to address the r/o roots (thanks to Dimitri for reminding).

OK, I'll go back to frontend side and see how settings work in inspector JS land.
(I'm not familiar with it.)
Thanks for the suggestion.

On readonly-ness, currently Shadow DOM in general is readonly
regardless if it comes from built-in element or not.
Comment 22 Hajime Morrita 2012-09-06 23:41:36 PDT
Created attachment 162683 [details]
Patch
Comment 23 Hajime Morrita 2012-09-06 23:43:11 PDT
Updated the patch. It just moves showShadowDOM feature out from experiment.
Comment 24 Pavel Feldman 2012-09-07 03:38:16 PDT
But now that we know that it crashes on editing input / media shadow, shouldn't we introduce read only mode prior to moving out of experimental?
Comment 25 Hajime Morrita 2012-09-10 16:12:20 PDT
(In reply to comment #24)
> But now that we know that it crashes on editing input / media shadow, shouldn't we introduce read only mode prior to moving out of experimental?

I'm sorry but  I lost the point.

In my understanding, Inspector has implemented read-only mode for Shadow DOM through
InspectorDOMAgent::assertEditableElement() and assertEditableNode(). 
The mechanism looks working as far as I tried on a canary build.
Is there any known crash for shadow editing?
If any, it shouldn't be shipped and I'm eager to investigate and fix them.
Comment 26 Pavel Feldman 2012-09-19 06:01:49 PDT
> I'm sorry but  I lost the point.
> 
> In my understanding, Inspector has implemented read-only mode for Shadow DOM through
> InspectorDOMAgent::assertEditableElement() and assertEditableNode(). 

Heh, ok, sorry, I forgot that I did it :)

> The mechanism looks working as far as I tried on a canary build.
> Is there any known crash for shadow editing?
> If any, it shouldn't be shipped and I'm eager to investigate and fix them.

I would experiment with $0 in the console - you can remove elements from shadow DOM using %0.parentNode.removeChild($0). The solution would be to either not call ConsoleAgent.addInspectedNode for read-only nodes, or to add 0 node id there.
Comment 27 Pavel Feldman 2012-09-19 06:03:10 PDT
Comment on attachment 162683 [details]
Patch

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

> Source/WebCore/inspector/front-end/SettingsScreen.js:271
> +    p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show Shadow DOM"), WebInspector.settings.showShadowDOM));

This string should be added to the English.lproj/localizedStrings.js. Otherwise looks good.
Comment 28 Hajime Morrita 2012-09-24 18:03:08 PDT
(In reply to comment #27)
> (From update of attachment 162683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162683&action=review
> 
> > Source/WebCore/inspector/front-end/SettingsScreen.js:271
> > +    p.appendChild(this._createCheckboxSetting(WebInspector.UIString("Show Shadow DOM"), WebInspector.settings.showShadowDOM));
> 
> This string should be added to the English.lproj/localizedStrings.js. Otherwise looks good.
Thanks for another review, Feldman!
I'll update this and land the patch.
Comment 29 Hajime Morrita 2012-09-28 01:04:41 PDT
Created attachment 166166 [details]
Patch for landing
Comment 30 WebKit Review Bot 2012-09-28 01:41:07 PDT
Comment on attachment 166166 [details]
Patch for landing

Clearing flags on attachment: 166166

Committed r129861: <http://trac.webkit.org/changeset/129861>
Comment 31 WebKit Review Bot 2012-09-28 01:41:11 PDT
All reviewed patches have been landed.  Closing bug.