Bug 94274 – Move Shadow DOM inspection feature out from experiments

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Hajime Morrita]

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.

[Pavel Feldman]

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.

[Dimitri Glazkov (Google)]

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.

[Hajime Morrita]

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.

[Hayato Ito]

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

[Hajime Morrita]

> 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 :-)

[Hayato Ito]

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 :-)

[Hajime Morrita]

Created attachment 162212 [details]
Patch

[Hajime Morrita]

Created attachment 162218 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 162218 [details]
Patch

Should we move the supportsShadowDOM somewhere to a global place?

[WebKit Review Bot]

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

[Hajime Morrita]

Created attachment 162423 [details]
Patch

[Hajime Morrita]

> Should we move the supportsShadowDOM somewhere to a global place?
Good point. done.

[Pavel Feldman]

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.

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 162446 [details]
Patch

[Pavel Feldman]

Dmitry, do you want to show input/media shadow when shadow Dom option is checked in the inspector?

[Dimitri Glazkov (Google)]

(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?

[Pavel Feldman]

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.

[Pavel Feldman]

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).

[Pavel Feldman]

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).

[Hajime Morrita]

(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.

[Hajime Morrita]

Created attachment 162683 [details]
Patch

[Hajime Morrita]

Updated the patch. It just moves showShadowDOM feature out from experiment.

[Pavel Feldman]

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?

[Hajime Morrita]

(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.

[Pavel Feldman]

> 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.

[Pavel Feldman]

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.

[Hajime Morrita]

(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.

[Hajime Morrita]

Created attachment 166166 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166166 [details]
Patch for landing

Clearing flags on attachment: 166166

Committed r129861: <http://trac.webkit.org/changeset/129861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.