Bug 94274

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.

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.

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.

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

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

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

Created attachment 162212 [details] Patch

Created attachment 162218 [details] Patch

Comment on attachment 162218 [details] Patch Should we move the supportsShadowDOM somewhere to a global place?

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

Created attachment 162423 [details] Patch

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

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.

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.

Created attachment 162446 [details] Patch

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

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

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

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

Created attachment 162683 [details] Patch

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

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?

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

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

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

Created attachment 166166 [details] Patch for landing

Comment on attachment 166166 [details] Patch for landing Clearing flags on attachment: 166166 Committed r129861: <http://trac.webkit.org/changeset/129861>

All reviewed patches have been landed. Closing bug.