Bug 21570 – Elements panel needs to be able to preview images

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Anthony Ricaud]

Like Firebug, the Web inspector needs an image preview. Right now, you have to click on the url to see the image and go back to the Elements panel. This is a bad workflow when you're trying to find which rule sets a background image for example.

[Alexander Pavlov (apavlov)]

Created attachment 67387 [details]
[IMAGE] Screenshot of image preview popovers in the DOM tree and Styles sidebar

I have a patch locally that implements the feature, but in the Styles sidebar a "propertyName: propertyValue" tooltip gets in the way. This is a call for discussion of a good way to mitigate this problem.

[Timothy Hatcher]

Looks neat.

Some visual comments:

* We should put a 1px rgb 128,128,128 border around the image.
* The popover arow in the first shot should be over the first character of the link text. It looks like it si just floating in space right now.

[Timothy Hatcher]

Another thing: we should disable the tooltip and put the image URL in this popover. The second shot shows both the popover and a tooltip which looks bad.

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> Looks neat.
> 
> Some visual comments:
> 
> * We should put a 1px rgb 128,128,128 border around the image.

Will do.

> * The popover arow in the first shot should be over the first character of the link text. It looks like it si just floating in space right now.

The popover is activated for the entire treeoutline item (highlighted in light blue). Perhaps I chose too wide an area for the popup activation (well, not without reason: the popup displays the IMG-specified image dimensions and renders the image with the correct repeat/repeat-x/repeat-y value, if applicable, which makes no sense for "pure" images). Do you think only the link should be the popup activator rather than the entire IMG element?

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> Another thing: we should disable the tooltip and put the image URL in this popover. The second shot shows both the popover and a tooltip which looks bad.

This also occurred to me while I was thinking about the data presentation, but this approach will make the Styles UI inconsistent: every "property container" (a LI containing SPANs for the property name and value) has a "title" with the full property signature. This is what you are seeing while the mouse is hovering over the URL (since the mouse events bubble up to the LI). Thus, some properties will have this tooltip and others won't. Do you think this is acceptable?

[Alexander Pavlov (apavlov)]

Created attachment 126772 [details]
Patch

[WebKit Review Bot]

Comment on attachment 126772 [details]
Patch

Attachment 126772 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11507854

New failing tests:
inspector/elements/elements-img-tooltip.html

[Pavel Feldman]

Comment on attachment 126772 [details]
Patch

r- per failing test.

[Alexander Pavlov (apavlov)]

Created attachment 126930 [details]
Patch

[Pavel Feldman]

Comment on attachment 126930 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:721
> +                this._dimensions = { offsetWidth: properties[0], offsetHeight: properties[1], naturalWidth: properties[2], naturalHeight: properties[3] };

You should place all the functionality into the same place, there is no need to place some of it here and the other part in the panel.

[Alexander Pavlov (apavlov)]

Created attachment 126941 [details]
Patch

[WebKit Review Bot]

Attachment 126941 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   3db9340..6b2655f  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 107692 = 3db9340249163e9ff8eca4792746293bbc60277d
r107693 = 980fd80b57e9db12d3998920b813b794c1188c9a
r107694 = 6b2655f69a6da87f57db7d8a842ffa5b3b0d73f2
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
	M	Source/WebKit/chromium/src/WebLayerTreeView.cpp
	M	Source/WebKit/chromium/ChangeLog
r107695 = 3ef0e66d03758bef5ec0a5816665dead0f5aee3f (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 126941 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsPanel.js:359
> +        // We get here for CSS property treeElements, too, so bail out if there is no "nodeName" method.

Testing for nodeName is fragile - imagine more tree elements with representedObjects containing nodeName property. You should only call this method for ElementsTreeElements.

> Source/WebCore/inspector/front-end/ElementsPanel.js:374
> +                treeElement._dimensions = { offsetWidth: properties[0], offsetHeight: properties[1], naturalWidth: properties[2], naturalHeight: properties[3] };

Do not mutate objects that don't belong to you - you should simply do var dimetions here and return them in a callback

> Source/WebCore/inspector/front-end/ElementsPanel.js:393
> +            object.callFunction(dimensions, setDimensionsData);

We now have a returnByValue parameter in callFunctionOn. So that you did not have to JSON.parse and concatenate the string. Simply return an array and use it as array.

> Source/WebCore/inspector/front-end/ElementsPanel.js:396
> +        WebInspector.RemoteObject.resolveNode(node, "", resolvedNode);

Recently, I started using direct flow of callbacks (as in our tests). It is easier to read the code that way. You should consider giving it a try.

> Source/WebCore/inspector/front-end/ElementsPanel.js:403
> +            if (listItem.treeElement._dimensions)

No need to cache - things might change.

[Alexander Pavlov (apavlov)]

Created attachment 126950 [details]
Patch

[WebKit Review Bot]

Attachment 126950 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexander Pavlov (apavlov)]

Committed r107705: <http://trac.webkit.org/changeset/107705>