Bug 153911

Summary: Web Inspector: REGRESSION: Elements Tab > Node Details Sidebar > Properties Section is spammed with TypeErrors
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: graouts, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Image] Bug
none
[PATCH] Proposed Fix
hi: review+
[IMAGE] Top level <elem> shows all displayable properties
none
[IMAGE] Prototype levels show API views with invokable getters none

Description Nikita Vasilyev 2016-02-04 21:41:26 PST 
Created attachment 270727 [details]
[Image] Bug

Steps:
1. Open https://webkit.org
2. Inspect <body>
3. In the right sidebar, select "Node"
4. Expand "Properties", then "HTMLElement" sections

Actual:
Lots of errors:
    children: TypeError: The Element.children getter can only be used on instances of Element
    classList: TypeError: The Element.classList getter can only be used on instances of Element
    className: TypeError: The Element.className getter can only be used on instances of Element
    ...

Expected:
Not TypeErrors.

Notes:
Works well in Safari on OS X 10.11.3.
Comment 1 Radar WebKit Bug Importer 2016-02-04 21:41:43 PST 
<rdar://problem/24520098>
Comment 2 Joseph Pecoraro 2016-02-05 00:25:03 PST 
Technically the Node pane has been broken for a while. The fact that it now outputs TypeErrors is actually a progression in DOM bindings behavior. I'm sure we have an older bug about this which I can search for tomorrow.
Comment 3 Joseph Pecoraro 2016-02-12 13:38:36 PST 
Does anyone use this section? Should we just delete it?
Comment 4 Nikita Vasilyev 2016-02-13 21:51:34 PST 
(In reply to comment #3)
> Does anyone use this section? Should we just delete it?

We can run a poll on https://twitter.com/WebKit to find out.

This section would be way more useful if we showed user-defined
properties first. Many frameworks have data associated with elements.
We do that in our code occasionally, too:

ConsoleMessageView.js

    this._element.__message = this._message
Comment 5 BJ Burg 2016-05-29 08:29:37 PDT 
*** Bug 142833 has been marked as a duplicate of this bug. ***
Comment 6 BJ Burg 2016-05-29 08:42:58 PDT 
(In reply to comment #3)
> Does anyone use this section? Should we just delete it?

We should keep it. It's hard to know the API otherwise. As you put in the duped bug, this should be an API view.
Comment 7 BJ Burg 2016-05-29 08:45:40 PDT 
(In reply to comment #4)
> (In reply to comment #3)
> > Does anyone use this section? Should we just delete it?
> 
> We can run a poll on https://twitter.com/WebKit to find out.
> 
> This section would be way more useful if we showed user-defined
> properties first. Many frameworks have data associated with elements.
> We do that in our code occasionally, too:
> 
> ConsoleMessageView.js
> 
>     this._element.__message = this._message

Let's postpone this discussion for another bug and another day.
This bug should address the regression. I filed a separate bug: https://bugs.webkit.org/show_bug.cgi?id=158193
Comment 8 Timothy Hatcher 2016-06-03 11:13:36 PDT 
*** Bug 158334 has been marked as a duplicate of this bug. ***
Comment 9 Joseph Pecoraro 2017-03-14 18:00:21 PDT 
I've approached this with a mostly API view except for the top level thing which will get us most of the properties (like the console).

So, for body we will get:

    • <body>
      - Properties view, so you see all the properties

    • HTMLBodyElement (Prototype)
    • HTMLElement (Prototype)
    • Element (Prototype)
    • Node (Prototype)
    • EventTarget (Prototype)
    • Object (Prototype)
      - "Pure" API views, so you can see the exact methods available

I also made sure we get the best possible API view:

    (1) Getters can be invoked to see the value for the selected node.
    (2) NativeFunctionParameter strings where possible.
    (3) Hovering properties shows "node.property" where possible.

I think this get us the best of all worlds. We also don't need any protocol additions.
Comment 10 Joseph Pecoraro 2017-03-14 18:10:34 PDT 
Created attachment 304455 [details]
[PATCH] Proposed Fix
Comment 11 Joseph Pecoraro 2017-03-14 18:11:43 PDT 
Created attachment 304456 [details]
[IMAGE] Top level <elem> shows all displayable properties
Comment 12 Joseph Pecoraro 2017-03-14 18:12:13 PDT 
Created attachment 304457 [details]
[IMAGE] Prototype levels show API views with invokable getters
Comment 13 Devin Rousso 2017-03-14 21:41:59 PDT 
Comment on attachment 304455 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:198
> +            object.callFunction(inspectedPage_node_collectPrototypes, undefined, false, nodePrototypesReady.bind(this));

NIT: use `const` variables for `undefined` and `false`

const args = undefined;
const generatePreview = false;

> Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:236
> +                if (title.match(/Prototype$/)) {

NIT: I think it's slightly faster to use `/Prototype$/.test(title)`.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:240
> +    setPrototypeNameOverride(override)

Any reason not to use a setter?  Or do you want this to be more of a "mode change" in the way it's read (meaning it's not just a value but instead it's a fully fledged function and will do stuff) ?
Comment 14 Joseph Pecoraro 2017-03-15 10:53:15 PDT 
Comment on attachment 304455 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:240
>> +    setPrototypeNameOverride(override)
> 
> Any reason not to use a setter?  Or do you want this to be more of a "mode change" in the way it's read (meaning it's not just a value but instead it's a fully fledged function and will do stuff) ?

Yeah, I want this to feel like a mode change, not some property that you can set at any time. Something you set initially and never touch again. And its so rare (just this one place) that its not a constructor param.
Comment 15 Joseph Pecoraro 2017-03-15 11:16:00 PDT 
<https://trac.webkit.org/changeset/213991>