| Summary: | Web Inspector: remove shadow dom inspection from Elements panel by default. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||
| Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, joepeck, juzna.cz+webkit, keishi, loislo, mnaganov, pfeldman, pmuellr, rik, timothy, tkent, yurys | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Pavel Feldman
2011-06-06 22:59:31 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. (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. 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. 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? I apologize -- shouldn't have closed it. BFF? :) (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. 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! You and your logic. SGTM. (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. > > - 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.
>
> 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.
(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 (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. 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. Created attachment 96283 [details]
patch
Comment on attachment 96283 [details]
patch
I reverted this locally and I can see that there should be _tagHTML, not buildTagDOM.
(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 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? > why did this change?
Ignore this why.
Hmm, I loved this feature. Could we keep it only in debug build? (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. 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. 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 (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. |