Bug 21554 – Hovering the "src" attribute for an image should show the image dimensions

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

[Timothy Hatcher]

From http://screwlewse.com/?p=59

"I want to see image dimensions when I hover over the markup instead of seeing the src, which I can see already right there in the markup."

[Daniel Bates]

Created attachment 43643 [details]
Patch

[Daniel Bates]

The patch I attached just shows the computed height and width on hover, but maybe we want to show the "natural" height and width of the image as well?

[Daniel Bates]

Created attachment 43645 [details]
Patch

Shows both the displayable and natural dimensions (if applicable) of the image when you hover over an images src.

Note, I only show the natural dimensions if they differ from the displayable dimensions.

[Daniel Bates]

Created attachment 43646 [details]
Patch with manual test

Swapped order of height x width shown in tooltip to be width x height so as to conform to the ordering seen in the title of the window when you open a single image in Safari.

Added a manual test case, /manual-tests/inspector/display-image-dimensions-on-hover-src.html

[Timothy Hatcher]

Comment on attachment 43646 [details]
Patch with manual test

> -        var title = this._nodeTitleInfo(this.representedObject, this.hasChildren, WebInspector.linkifyURL).title;
> +        var self = this;
> +        var callback = function(propertiesPayload) {
> +            var getPropertyValue = function(propertyName) {
> +                for (e in propertiesPayload)
> +                    if (propertiesPayload[e].name == propertyName)
> +                        return propertiesPayload[e].value.description;
> +                return null;
> +            };
> +            self._updateTitleWithRespectToNodeProperties(getPropertyValue);
> +        };
> +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);
> +        InjectedScriptAccess.getProperties(prototype, true, callback);

This is going to slow down the Elements panel, since it will delay updating the title waiting for the async InjectedScriptAccess.getProperties. (Slower in Chrome where the async is corss process. So we need to minimize these calls.)

We are paying a heavy cost of InjectedScriptAccess.getProperties for every DOM element just to filter it later for image elements. The InjectedScriptAccess.getProperties call was not designed for one-off property access, as you found, since it has .name properties. It is really only used for property inspection. I am not sure what the best solution is.

Also these should use named functions, with the brace o nthe next line like:

> +        function callback(propertiesPayload)
> +        {
> +            function getPropertyValue(propertyName)
> +            {

> +                            if (node.nodeName.toLowerCase() == "img") {

Use === in cases like this.

> +                                tooltipText = offsetWidth + "px x " + offsetHeight + "px";

This should use \u00d7 for the multiplication sign, not "x". I don't think "px" is needed twice. You could do "123x456 pixels" like Safari, but that will need to be localized with a WebInspector.UIString. Or just leave off the units.

> +                                if (offsetHeight != naturalHeight || offsetWidth != naturalWidth)

Use !==.

> +                                    tooltipText += " (natural: " + naturalWidth + "px x " + naturalHeight + "px)";

This text will need to be localized, and should not be appened to another string that might be localized. Other languages mgiht want to show them in a different order.

So: WebInspector.UIString("%d\u00d7%d", naturalWidth, naturalHeight)

[Timothy Hatcher]

I'm curiosu what Pavel thinks about the InjectedScriptAccess.getProperties code.

[Pavel Feldman]

Comment on attachment 43646 [details]
Patch with manual test

> +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);

No need to call this prototype - it was real prototype in the code you took it from, while in here it is just a node wrapper you create.
"new WebInspector.ObjectProxy(this.representedObject.id)" would also do.

> +        InjectedScriptAccess.getProperties(prototype, true, callback);
> +    },
> +
> +    _updateTitleWithRespectToNodeProperties: function(getPropertyValue)
> +    {

This looks too complex to keep it in here. I'd suggest at least introducing WebInspector.ObjectProxy.getPropertiesAsync(objectProxy, callback) that would make properties request and invoke given callback with the result object that would essentially be a map of name/values. You will then take the ones you need from there.

I can also imagine us showing tips for other kinds of the nodes, so it might make sense to make this code a bit more optimized. Getting all the properties and wrapping them might be too expensive. I'd suggest introducing InjectedScriptAccess.getPropertiesSimple(objectWrapper, ["array", "of", "names", "to", "query", "for"], callback); that would work for basic properties only (ones with primitive value types) and would not wrap them with ObjectPropertyProxy, i.e. simply send JSON object in return. I can do this part for you if you find explanation unclear.

[Pavel Feldman]

By the way, why manual test? You should be able to verify generated tip against expectations using inspector testing harness. See LayoutTests/inspector/styles-iframe.html as an expample.

1. You don't need runAfterIframeIsLoaded since you have no iframes in your case, just use its continuation.
2. Rename frontend_dumpStyles to frontend_dumpTooltip or something
3. You call frontend_expandDOMSubtree(WebInspector.domAgent.document); as in that example in order to expand the whole dom tree in elements panel
4. You continue execution in testController.runAfterPendingDispatches callback. It is safe to traverse WebInspector.domAgent.document structure there. In the styles-iframe test, IFRAME node's id is resolved. You would need to resolve IMG's one.
5. Last bit is to call your method calculating tooltip for node with that id asynchronously and to report result into testController.notifyDone (as in the example).

Send me a note if you need help!

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 43646 [details])
> > -        var title = this._nodeTitleInfo(this.representedObject, this.hasChildren, WebInspector.linkifyURL).title;
> > +        var self = this;
> > +        var callback = function(propertiesPayload) {
> > +            var getPropertyValue = function(propertyName) {
> > +                for (e in propertiesPayload)
> > +                    if (propertiesPayload[e].name == propertyName)
> > +                        return propertiesPayload[e].value.description;
> > +                return null;
> > +            };
> > +            self._updateTitleWithRespectToNodeProperties(getPropertyValue);
> > +        };
> > +        var prototype = new WebInspector.ObjectProxy(this.representedObject.id, [], 0);
> > +        InjectedScriptAccess.getProperties(prototype, true, callback);
> 
> This is going to slow down the Elements panel, since it will delay updating the
> title waiting for the async InjectedScriptAccess.getProperties. (Slower in
> Chrome where the async is corss [sic] process. So we need to minimize these calls.)
> 
> We are paying a heavy cost of InjectedScriptAccess.getProperties for every DOM
> element just to filter it later for image elements. The
> InjectedScriptAccess.getProperties call was not designed for one-off property
> access, as you found, since it has .name properties. It is really only used for
> property inspection. I am not sure what the best solution is.
> 

Spoke to Pavel on IRC today (11/22). Will implement WebInspector.ObjectProxy.getPropertiesAsync (as Pavel suggested <https://bugs.webkit.org/show_bug.cgi?id=21554#c7>) and file a bug report so that we can further optimize property access.

> Also these should use named functions, with the brace o nthe next line like:
> 
> > +        function callback(propertiesPayload)
> > +        {
> > +            function getPropertyValue(propertyName)
> > +            {
> 
> > +                            if (node.nodeName.toLowerCase() == "img") {
> 
> Use === in cases like this.

Will change.

> > +                                tooltipText = offsetWidth + "px x " + offsetHeight + "px";
> 
> This should use \u00d7 for the multiplication sign, not "x". I don't think "px"
> is needed twice. You could do "123x456 pixels" like Safari, but that will need
> to be localized with a WebInspector.UIString. Or just leave off the units.

Will change to conform to Safari (i.e. "123x456 pixels").

> > +                                if (offsetHeight != naturalHeight || offsetWidth != naturalWidth)
> 
> Use !==.

Will change.

> > +                                    tooltipText += " (natural: " + naturalWidth + "px x " + naturalHeight + "px)";
> 
> This text will need to be localized, and should not be appened to another
> string that might be localized. Other languages mgiht want to show them in a
> different order.

I thought to append this to the tooltip only if the displayable size differed from the natural size.

> So: WebInspector.UIString("%d\u00d7%d", naturalWidth, naturalHeight)

So, I take it you only want to show the natural dimensions? Not the displayable dimensions?

[Daniel Bates]

(In reply to comment #8)
> By the way, why manual test? You should be able to verify generated tip against
> expectations using inspector testing harness. See
> LayoutTests/inspector/styles-iframe.html as an expample.
> 
> 1. You don't need runAfterIframeIsLoaded since you have no iframes in your
> case, just use its continuation.
> 2. Rename frontend_dumpStyles to frontend_dumpTooltip or something
> 3. You call frontend_expandDOMSubtree(WebInspector.domAgent.document); as in
> that example in order to expand the whole dom tree in elements panel
> 4. You continue execution in testController.runAfterPendingDispatches callback.
> It is safe to traverse WebInspector.domAgent.document structure there. In the
> styles-iframe test, IFRAME node's id is resolved. You would need to resolve
> IMG's one.
> 5. Last bit is to call your method calculating tooltip for node with that id
> asynchronously and to report result into testController.notifyDone (as in the
> example).
> 
> Send me a note if you need help!

I'll look into this.

[Timothy Hatcher]

You can show both sizes, you will just need two localized strings.

"%d\u00d7%d pixels"

and

"%d\u00d7%d pixels (Natural: %d\u00d7%d pixels)"

[Daniel Bates]

Created attachment 43866 [details]
Patch with manual test

Updated patch based on Timothy's and Pavel's comments.

[Pavel Feldman]

Comment on attachment 43866 [details]
Patch with manual test

A handful of style nits, otherwise good to go. Please put // TODO: Replace with InjectedScriptAccess.getBasicProperties(obj, [names]) in place of the extended Q&A block. That will save few bytes on loading / parsing.

>  
> +WebInspector.ElementsTreeElement.createTooltipForImageNode = function(properties)
> +{

It is Ok to define this one of prototype even though it does not use 'this'. We do that in other places.

> +    var tooltipText = null;
> +    var offsetHeight = properties["offsetHeight"];

You can use properties.offsetHeight (and there is no need in explicit intermediate variable I guess).


>  WebInspector.ElementsTreeElement.prototype = {
>      get highlighted()
>      {
> @@ -787,14 +801,30 @@ WebInspector.ElementsTreeElement.prototy
>          if (this._editing)
>              return;
>  

> +            return callback();

No need to return it, just call it.

> +        var objectProxy = new WebInspector.ObjectProxy(this.representedObject.id);
> +        WebInspector.ObjectProxy.getPropertiesAsync(objectProxy, callback);

It would be even better if you provided the names of the properties needed here, so that it would
be easier to understand what the 'properties' object you carry is needed for.



> +        var result = [];
> +        for (e in propertiesPayload)

We typically iterate over arrays using index 'for (var i = 0; i < properties.length; ++i)'

[Daniel Bates]

Created attachment 43881 [details]
Patch with test case

Updated patch based on Timothy's and Pavel's comments.

Replaced manual test case with DRT layout test based on inspector/styles-iframe.html

[Pavel Feldman]

Comment on attachment 43881 [details]
Patch with test case

Thanks for addressing the comments. It now looks really good and clean. Test is very good too. Couple of minot nits and re are ready to land:

img-tooltip.html should be called elements-img-tooltip (we try to group things by panels)

> +    function createPropertiesMapThenCallback(propertiesPayload)
> +    {

Since the interaction between front-end and injected script is asynchronous, node that you passed into getProperties might
be already deleted. You should be ready for that. 'propertiesPayload' will be 'false' in that case and you should pass it further
as undefined or []. You should then be able to handle missing properties when generating tooltip.

> +
> +    var innerMapping = WebInspector.domAgent._idToDOMNode;
> +    for (var nodeId in innerMapping) {
> +        if (innerMapping[nodeId].nodeName === "IMG") {
> +            WebInspector.ElementsTreeElement.prototype.createTooltipForImageNode(innerMapping[nodeId], callback);
> +            break;
> +        }
> +    }
> +}

Nit: You should do return in place of break and call testController.notifyDone(); after the loop. This way when elements panel breaks and has no IMG, you will not need to wait for timieout.

You probably need to store img inline in order not to wait for load event, but you should make it more simple. I'll attach a white rectangle of the same size to the bug. it is 80 times smaller.

[Pavel Feldman]

Created attachment 43907 [details]
[IMAGE] Blank image of needed size for tests.

[Daniel Bates]

Created attachment 43908 [details]
Patch with test case

Updated patch based on Pavel's comments.

[Pavel Feldman]

Comment on attachment 43908 [details]
Patch with test case

Thanks for doing that!

[WebKit Commit Bot]

Comment on attachment 43908 [details]
Patch with test case

Rejecting patch 43908 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11668 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4970 tests run.
86.81s total testing time

4969 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

[Pavel Feldman]

Comment on attachment 43908 [details]
Patch with test case

Flaky test?

[WebKit Commit Bot]

Comment on attachment 43908 [details]
Patch with test case

Rejecting patch 43908 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11668 test cases.
fast/canvas/webgl/gl-object-get-calls.html -> failed

Exiting early after 1 failures. 4970 tests run.
89.19s total testing time

4969 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

[Eric Seidel (no email)]

Failures caused by bug 31919.

[Eric Seidel (no email)]

Comment on attachment 43908 [details]
Patch with test case

I've moved the commit-queue to another machine for now. Let's try again.

[WebKit Commit Bot]

Comment on attachment 43908 [details]
Patch with test case

Clearing flags on attachment: 43908

Committed r51420: <http://trac.webkit.org/changeset/51420>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.