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

Nikita Vasilyev
Reported 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.
Attachments
[Image] Bug (268.10 KB, image/png)
2016-02-04 21:41 PST, Nikita Vasilyev 		no flags
Details
[PATCH] Proposed Fix (13.92 KB, patch)
2017-03-14 18:10 PDT, Joseph Pecoraro 		hi: review+
DetailsFormatted DiffDiff
[IMAGE] Top level <elem> shows all displayable properties (611.18 KB, image/png)
2017-03-14 18:11 PDT, Joseph Pecoraro 		no flags
Details
[IMAGE] Prototype levels show API views with invokable getters (554.19 KB, image/png)
2017-03-14 18:12 PDT, Joseph Pecoraro 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-02-04 21:41:43 PST
<rdar://problem/24520098>
Joseph Pecoraro
Comment 2 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.
Joseph Pecoraro
Comment 3 2016-02-12 13:38:36 PST
Does anyone use this section? Should we just delete it?
Nikita Vasilyev
Comment 4 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
Blaze Burg
Comment 5 2016-05-29 08:29:37 PDT
*** Bug 142833 has been marked as a duplicate of this bug. ***
Blaze Burg
Comment 6 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.
Blaze Burg
Comment 7 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
Timothy Hatcher
Comment 8 2016-06-03 11:13:36 PDT
*** Bug 158334 has been marked as a duplicate of this bug. ***
Joseph Pecoraro
Comment 9 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.
Joseph Pecoraro
Comment 10 2017-03-14 18:10:34 PDT
Created attachment 304455 [details] [PATCH] Proposed Fix
Joseph Pecoraro
Comment 11 2017-03-14 18:11:43 PDT
Created attachment 304456 [details] [IMAGE] Top level <elem> shows all displayable properties
Joseph Pecoraro
Comment 12 2017-03-14 18:12:13 PDT
Created attachment 304457 [details] [IMAGE] Prototype levels show API views with invokable getters
Devin Rousso
Comment 13 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) ?
Joseph Pecoraro
Comment 14 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.
Joseph Pecoraro
Comment 15 2017-03-15 11:16:00 PDT
<https://trac.webkit.org/changeset/213991>
Note You need to log in before you can comment on or make changes to this bug.