Bug 62188 - Web Inspector: remove shadow dom inspection from Elements panel by default.
Summary: Web Inspector: remove shadow dom inspection from Elements panel by default.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-06 22:59 PDT by Pavel Feldman
Modified: 2012-05-02 02:28 PDT (History)
14 users (show)

See Also:


Attachments
patch (31.45 KB, patch)
2011-06-07 12:32 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-06-06 22:59:31 PDT
Today, expanding <input> element shows dom nodes that were not authored by the client. It adds a lot of confusion and brings no value to the end user.
I can only see shadow dom inspection valuable for the WebKit engineers working with the shadow dom. Could we add invisible switch / keyboard shortcut that would opt into that mode please?
Comment 1 Pavel Feldman 2011-06-07 07:41:23 PDT
So I looked at it again. In addition to confusing the end user with the "shadow" concepts, implementation added in http://trac.webkit.org/changeset/85751 is bad. It adds complexity into already too complex InspectorDOMAgent. In particular, in some places of code it assumes that shadow root is a part of the DOM hierarchy, in the others it treats it differently.

I'd suggest:
- rolling out this change
- waiting for clients to ask for it
- doing it properly if they do

We should not compromise product quality and code complexity for the sake of experimental features with no demand.
Comment 2 Andrey Kosyakov 2011-06-07 07:56:44 PDT
(In reply to comment #1)
> So I looked at it again. In addition to confusing the end user with the "shadow" concepts, implementation added in http://trac.webkit.org/changeset/85751 is bad. It adds complexity into already too complex InspectorDOMAgent. In particular, in some places of code it assumes that shadow root is a part of the DOM hierarchy, in the others it treats it differently.
> 
> I'd suggest:
> - rolling out this change
> - waiting for clients to ask for it
> - doing it properly if they do
> 
> We should not compromise product quality and code complexity for the sake of experimental features with no demand.

Pavel, would you please elaborate your notion of "code complexity"? In InspectorDOMAgent, this affects some 15 lines, which appears to me as fairly humble for a 1.5K lines file (and probably way less impact that shadow trees have in other places).

We use same instrumentation methods in the InspectorInstrumentation interface in order to keep the interface narrow (also, the existing names seemed a good match for what's happening).

As for the front-end/back-end interface (i.e. inspector protocol), I should have probably reused similar methods, but since these had usual DOM parent/child relationship explicitly in their names, I went for a separate method. We may consider renaming the protocol methods, perhaps, as part of the protocol cleanup.

WRT "confusing the end user" -- in my view, having an ability to see the implementation of shadow DOM helps user to understand how the element will be rendered (and, to some extent, what may be styled).

As for "no demand", I think it would be fair to hear from Dimitry & co, at least.
Comment 3 Dimitri Glazkov (Google) 2011-06-07 09:01:03 PDT
I agree with Andrey. Shadow DOM inspection is the same kind of useful as showing UA stylesheet info. Though if there are bugs in implementation, we should be fixing them.
Comment 4 Pavel Feldman 2011-06-07 09:20:44 PDT
Dimitry, I would appreciate if you respected my judgement in the inspector subsystem as much as I respect yours in the areas you are working on. 

As an author of the InspectorDOMAgent I claim that code that the change that was landed is simply wrong.

As an Elements panel maintainer who works close with the inspector users, I say that non-user nodes under input tag is confusing.
 
As a feature owner working on project priorities for 2 years I think it should have never been landed.

There is no interest for it in the field.

Why do you close my bug as invalid?
Comment 5 Dimitri Glazkov (Google) 2011-06-07 09:31:19 PDT
I apologize -- shouldn't have closed it. BFF? :)
Comment 6 Dimitri Glazkov (Google) 2011-06-07 09:38:28 PDT
(In reply to comment #4)

> As an Elements panel maintainer who works close with the inspector users, I say that non-user nodes under input tag is confusing.

I think it's confusing, yes -- but it reveals precise implementation details that are useful and necessary, if you're trying to style non-trivial forms control or media elements. If anything we should be embracing this more fully and exposing information like pseudo ids and more as the shadow DOM APIs mature.

> 
> As a feature owner working on project priorities for 2 years I think it should have never been landed.

I don't want to go against your opinion on this. If this is implemented poorly or wrong, you should take it out. But it's a good feature that we will need to implement eventually.

> 
> There is no interest for it in the field.

I would give it time. It's hard to gauge interest in the field for new features.

> 
> Why do you close my bug as invalid?

Apologies again. Shouldn't have done that.
Comment 7 Pavel Feldman 2011-06-07 09:42:41 PDT
Reasons I want it rolled out:
- crash upon  <input> attributes modification that was found once the code hit the field. We already need to patch the branch and the rollout seams the most reliable way of doing that
- overhead in the domain payloads
- arguable DOM agent changes that might hit us with more crashes
- Again, inut children are considered regression

Let us roll it out now. We can then get back to it and decide what it should look like. I know you need it, but it is a minority feature we should not compromise the quality for. And you can reapply the patch for now :P BFF indeed!
Comment 8 Dimitri Glazkov (Google) 2011-06-07 09:44:46 PDT
You and your logic. SGTM.
Comment 9 Andrey Kosyakov 2011-06-07 10:05:19 PDT
(In reply to comment #7)
> Reasons I want it rolled out:
> - crash upon  <input> attributes modification that was found once the code hit the field. We already need to patch the branch and the rollout seams the most reliable way of doing that

This is a single-line patch, which may actually be safer than rolling out large chunk of code that landed quite some time ago.

> - overhead in the domain payloads

Quite humble, I don't think we'll regress performance there: we add some 100 bytes per those (presumable few and rare) nodes that have shadow DOM. Yes, this could probably be smaller. No, I don't think this regresses performance in real life.

> - arguable DOM agent changes that might hit us with more crashes

I would like to understand better the nature of the "arguable" bit -- you did not go into details on what's wrong on the InspectorDOMAgent part. This mostly reuses the existing code (yes it had a pretty stupid crash, which we unfortunately missed due to this particular code path being not covered by tests; now that we have the tests for it, at least all code paths that I changed in the agent are covered, so I don't think we should expect future crashes for it)

> - Again, inut children are considered regression

Now, this appears to be a debatable bit -- looks like a feature to some. I think this sort of statements should at least come attributed to particular persons, rather than be expressed in passive voice.
Comment 10 Dimitri Glazkov (Google) 2011-06-07 10:08:00 PDT
> > - Again, inut children are considered regression
> 
> Now, this appears to be a debatable bit -- looks like a feature to some. I think this sort of statements should at least come attributed to particular persons, rather than be expressed in passive voice.

I agree with Andrey here. It's not a regression, it's a feature. Especially considering it exposes useful styling information.
Comment 11 Pavel Feldman 2011-06-07 10:11:54 PDT
> 
> I agree with Andrey here. It's not a regression, it's a feature. Especially considering it exposes useful styling information.

Could you please file a feature request with the end user scenario? It is a bug to me since it exposes "id" attributes not visible to the web facing API.
Comment 12 Dimitri Glazkov (Google) 2011-06-07 10:14:43 PDT
(In reply to comment #11)
> > 
> > I agree with Andrey here. It's not a regression, it's a feature. Especially considering it exposes useful styling information.
> 
> Could you please file a feature request with the end user scenario? It is a bug to me since it exposes "id" attributes not visible to the web facing API.

The id stuff just got rolled out. http://trac.webkit.org/changeset/88216
Comment 13 Andrey Kosyakov 2011-06-07 10:24:33 PDT
(In reply to comment #11)
> > 
> > I agree with Andrey here. It's not a regression, it's a feature. Especially considering it exposes useful styling information.
> 
> Could you please file a feature request with the end user scenario? It is a bug to me since it exposes "id" attributes not visible to the web facing API.

Well, everything we expose there is not web facing. Yet I consider this to be not unusual for developer tools to expose some platform internals that are not accessible via the API, as long as this helps developers better understand the way their application works. We may want to make this conditional (e.g. context menu item), or make it better stand out visually from the rest of the document, to avoid this confusion.
Comment 14 Pavel Feldman 2011-06-07 10:46:06 PDT
Now we start asking questions we should have before landing the change.

OK, ids are gone. What about styling? Are there classes? Will they match? I assume they don't today. Also confusing. What should be $0 behavior? Should it expose nodes? I assume it does today, but should it? I could document.body.appendChild($0) from shadow dom. Will it work?

How about we start with the bug full of user stories? I mean I respect your anxiousness to get it in, but let's keep the quality of the product high.
Comment 15 Andrey Kosyakov 2011-06-07 12:32:27 PDT
Created attachment 96283 [details]
patch
Comment 16 Pavel Feldman 2011-06-07 12:44:57 PDT
Comment on attachment 96283 [details]
patch

I reverted this locally and I can see that there should be _tagHTML, not buildTagDOM.
Comment 17 Andrey Kosyakov 2011-06-07 12:49:39 PDT
(In reply to comment #16)
> I reverted this locally and I can see that there should be _tagHTML, not buildTagDOM.

Nope, this got renamed as of r87294 (bug 61282)
Comment 18 Pavel Feldman 2011-06-07 12:52:09 PDT
Comment on attachment 96283 [details]
patch

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

Okay.

> Source/WebCore/inspector/front-end/ElementsPanel.js:493
> +            if (this.recentlyModifiedNodes[i].updated) {

why did this change?
Comment 19 Pavel Feldman 2011-06-07 12:52:34 PDT
> why did this change?

Ignore this why.
Comment 20 Kent Tamura 2011-06-08 01:42:26 PDT
Hmm, I loved this feature.
Could we keep it only in debug build?
Comment 21 Andrey Kosyakov 2011-06-08 01:49:05 PDT
(In reply to comment #20)
> Hmm, I loved this feature.
> Could we keep it only in debug build?

I'll try to resuscitate at some point, but as you can see from the discussion above, some are quite concerned with implications of exposing shadow DOM internals in inspector. Dimitri has raised another issue for this (bug 62220), let's move the discussion there. I'll add some open questions/concerns that probably need to be resolved before we re-land this.
Comment 22 Timothy Hatcher 2011-12-21 20:03:38 PST
I would find it useful to see them. I've often had to guess when I'm styling pseudo elements like ::-webkit-search-decoration. I assume that element would be in the shadow DOM and I could see it and tweak styles directly instead the reload churn I have today.

That said, I think it should be hidden by default with a switch to show them. The switch should be in the Inspector UI somewhere and be toggable.

Another option, show th shadow DOM elements if their corresponding CSS pseudo elements are styled by the developer specifically (like my example above.) That shows the auto knows about them and would likely want to see them (maybe always see them in this case.) Otherwise the Inspector is really hiding info from the author who is using an advanced WebKit feature.
Comment 23 Jan Dolecek 2012-04-26 10:41:12 PDT
I was kinda confused when I couldn't find the shadow dom in inspector after upgrading chrome :/ I wanted to play around with some components.

Can there be an option somewhere to enable this removed feature? I would love to have it. Thanks
Comment 24 Pavel Feldman 2012-05-02 02:28:08 PDT
(In reply to comment #23)
> I was kinda confused when I couldn't find the shadow dom in inspector after upgrading chrome :/ I wanted to play around with some components.
> 
> Can there be an option somewhere to enable this removed feature? I would love to have it. Thanks

1. Get latest Chrome Beta (or Dev To Canary)
2. Open about:flags
3. Check Enable Developer Tools experiments
4. Open devtools, settings
5. Check "shadow dom"
6. Re-open devtools

It'll be there since then.