Bug 195793

Summary: Web Inspector: DOM: "Capture Screenshot" should only be shown if the node is attached
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Devin Rousso 2019-03-14 23:17:30 PDT
I shouldn't be able to capture a screenshot of any random node that I've logged to the console (e.g. `document.createElement("div")`).
Comment 1 Radar WebKit Bug Importer 2019-03-14 23:17:44 PDT
<rdar://problem/48916594>
Comment 2 Devin Rousso 2019-03-14 23:33:10 PDT
Created attachment 364777 [details]
Patch
Comment 3 Joseph Pecoraro 2019-03-18 15:37:33 PDT
Comment on attachment 364777 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:239
> -    if (window.PageAgent) {
> +    if (window.PageAgent && domNode.ownerDocument) {

This feels wrong.

Every Node should be associated with a document:

    document.createElement("div").ownerDocument; // => #document

We may want to update our interface, as it doesn't make sense that a detached node doesn't have an ownerDocument...

It might be good to have an isAttached() method that resolves this state.
Comment 4 Devin Rousso 2019-03-18 15:46:38 PDT
Comment on attachment 364777 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:239
>> +    if (window.PageAgent && domNode.ownerDocument) {
> 
> This feels wrong.
> 
> Every Node should be associated with a document:
> 
>     document.createElement("div").ownerDocument; // => #document
> 
> We may want to update our interface, as it doesn't make sense that a detached node doesn't have an ownerDocument...
> 
> It might be good to have an isAttached() method that resolves this state.

I would agree, but this is the current "reality" of how we're able to determine such things.  When we push a detached root to the frontend (e.g. logging a node which you've just created), we don't set a document for it (`WI.DOMManager.prototype._setDetachedRoot`).  I'd like to rework this when doing the DOM agent redesign as well.
Comment 5 Joseph Pecoraro 2019-03-18 16:08:43 PDT
> > It might be good to have an isAttached() method that resolves this state.
> 
> I would agree, but this is the current "reality" of how we're able to
> determine such things.  When we push a detached root to the frontend (e.g.
> logging a node which you've just created), we don't set a document for it
> (`WI.DOMManager.prototype._setDetachedRoot`).  I'd like to rework this when
> doing the DOM agent redesign as well.

There should be another way to check if we are actually attached or not, perhaps by walking up the parentNode hierarchy.
Comment 6 Devin Rousso 2019-03-18 20:31:45 PDT
Created attachment 365115 [details]
Patch
Comment 7 Joseph Pecoraro 2019-03-18 21:24:37 PDT
Comment on attachment 365115 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:210
> -    if (WI.domDebuggerManager.supported && isElement && !domNode.isPseudoElement() && domNode.ownerDocument) {
> +    if (WI.domDebuggerManager.supported &&isElement && !domNode.isPseudoElement() && attached) {

Style: Oops on the lost space.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:781
> +        if (WI.cssManager.canForcePseudoClasses() && this.representedObject.attached) {

Might want to extract the attached into a bool hear to avoid people potentially recomputing it later.
Comment 8 Devin Rousso 2019-03-19 10:29:32 PDT
Created attachment 365179 [details]
Patch
Comment 9 WebKit Commit Bot 2019-03-19 11:03:39 PDT
Comment on attachment 365179 [details]
Patch

Clearing flags on attachment: 365179

Committed r243152: <https://trac.webkit.org/changeset/243152>
Comment 10 WebKit Commit Bot 2019-03-19 11:03:40 PDT
All reviewed patches have been landed.  Closing bug.