718250 - We should display the dimensions of the node in the NodeInfobar

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Paul Rouget [:paul]]

It would be nice to have a label with the dimensions for the node (width x height).
We could have this in the infobar, or maybe in the toolbar.

PS: Here, I am not talking about the Layout View.

Comment 1

[sys.sgx]

I agree with this. It will be very usefull indeed.

Comment 2

[Rob Campbell [:rc] (:robcee)]

it should also do double duty to show width and height of images when selected.

Comment 3

[Rob Campbell [:rc] (:robcee)]

also, I think I'm leaning towards the toolbar for this.

Comment 4

[Paul Rouget [:paul]]

The layout view patch adds the dimension of the node in the toolbar

Comment 5

[Paul Rouget [:paul]]

triage, filter on centaur

Comment 6

[Paul Rouget [:paul]]

Not duplicate because we are not sure to have the layout button in the toolbar.

Comment 8

[Paul Rouget [:paul]]

The layout view doesn't add anything in the toolbar anymore.

Comment 9

[Paul Rouget [:paul]]

WONTFIX (it's in the BoxModel tab in the toolbox).

Bug triage, filter on PINKISBEAUTIFUL

Comment 10

[sys.sgx]

Paul, it's not the same thing I believe. One of Chrome's best features is that you can track such info right where your mouse pointer is inside the page.
Plus, the BoxModel does not update on the fly (at least yet), you have to click several times to view the dimensions, for each element on the page.

Comment 11

[Paul Rouget [:paul]]

(In reply to sys.sgx from comment #10)
> Paul, it's not the same thing I believe. One of Chrome's best features is
> that you can track such info right where your mouse pointer is inside the
> page.

I think you're right. Re-opening this bug, again :)

> Plus, the BoxModel does not update on the fly (at least yet), you have to
> click several times to view the dimensions, for each element on the page.

We have a bug about that: bug 766526

Comment 12

[Girish Sharma [:Optimizer]]

(In reply to Paul Rouget [:paul] from comment #11)
> > Plus, the BoxModel does not update on the fly (at least yet), you have to
> > click several times to view the dimensions, for each element on the page.
> 
> We have a bug about that: bug 766526

since this bug is fixed now, is it okay to close this one ?

Comment 13

[Girish Sharma [:Optimizer]]

Ah, the first this in my previous comment is referring to bug 766526

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-node-dims-to-nodeinfobar-718250.patch

Very simple patch based on pbrosset's remote highlighter.

Comment 15

[Joe Walker [:jwalker]]

Comment on attachment 8340020 [details] [diff] [review]
add-node-dims-to-nodeinfobar-718250.patch

Review of attachment 8340020 [details] [diff] [review]:
-----------------------------------------------------------------

This show out of date info if the dimensions change during inspection doesn't it?

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8340020 [details] [diff] [review]
add-node-dims-to-nodeinfobar-718250.patch

(In reply to Joe Walker [:jwalker] from comment #15)
> Comment on attachment 8340020 [details] [diff] [review]
> add-node-dims-to-nodeinfobar-718250.patch
> 
> Review of attachment 8340020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This show out of date info if the dimensions change during inspection
> doesn't it?

Nope, it is updated on mutations.

You can test it on http://fiddle.jshell.net/mFtyG/2/show/. It is a div that resizes itself after 5 seconds.

Comment 17

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #16)
> Comment on attachment 8340020 [details] [diff] [review]
> add-node-dims-to-nodeinfobar-718250.patch
> 
> (In reply to Joe Walker [:jwalker] from comment #15)
> > Comment on attachment 8340020 [details] [diff] [review]
> > add-node-dims-to-nodeinfobar-718250.patch
> > 
> > Review of attachment 8340020 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This show out of date info if the dimensions change during inspection
> > doesn't it?
> 
> Nope, it is updated on mutations.

On mutations? It should get updated on reflow.

Here is how (for one window):

> function ReflowListener(aWindow) {
>   this.docshell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                          .getInterface(Ci.nsIWebNavigation)
>                          .QueryInterface(Ci.nsIDocShell);
>   this.docshell.addWeakReflowObserver(this);
> }
> 
> ReflowListener.prototype = {
>   QueryInterface: XPCOMUtils.generateQI([Ci.nsIReflowObserver, Ci.nsISupportsWeakReference]),
> 
>   reflow: function () {
>     // update dimensions
>   },
> 
>   reflowInterruptible: function () {
>     // update dimensions
>   },
> 
>   destroy: function () {
>     this.docshell.removeWeakReflowObserver(this);
>   },
> };

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Rebased

(In reply to Paul Rouget [:paul] from comment #17)

> On mutations? It should get updated on reflow.
> 

That would unnecessarily complicate things. At the moment we display and update the nodeInfoBar whenever the highlighter is updated. If we used a mutation observer for the highlighter and a reflow observer for the nodeInfoBar each would have to continually communicate state, besides, we would then have to calculate element positions twice, once for the box model highlighter and once for the nodeInfoBar. It makes more sense to leave it as it is.

Comment 19

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> Created attachment 8356585 [details] [diff] [review]
> Rebased
> 
> (In reply to Paul Rouget [:paul] from comment #17)
> 
> > On mutations? It should get updated on reflow.
> > 
> 
> That would unnecessarily complicate things. At the moment we display and
> update the nodeInfoBar whenever the highlighter is updated. If we used a
> mutation observer for the highlighter and a reflow observer for the
> nodeInfoBar each would have to continually communicate state, besides, we
> would then have to calculate element positions twice, once for the box model
> highlighter and once for the nodeInfoBar. It makes more sense to leave it as
> it is.

I'm not sure to fully understand this.

"mutations" are for DOM updates. We should update #ID.class1.classN on mutations.

"mozAfterPaint" are for paint updates. We should update the highlighter on paint.

"reflows" are for layout changes. We should update the dimensions on reflows.

A reflow doesn't always trigger a paint. So we can't rely on the mozAfterPaint event to update the dimensions. mutations have nothing to do with layout.

So when is the infobar updated? On scroll and paint if I'm not mistaken? On mutations as well? But for sure not on reflow.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8356585 [details] [diff] [review]
Rebased

(In reply to Paul Rouget [:paul] from comment #19)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #18)
> > Created attachment 8356585 [details] [diff] [review]
> > Rebased
> > 
> > (In reply to Paul Rouget [:paul] from comment #17)
> > 
> > > On mutations? It should get updated on reflow.
> > > 
> > 
> > That would unnecessarily complicate things. At the moment we display and
> > update the nodeInfoBar whenever the highlighter is updated. If we used a
> > mutation observer for the highlighter and a reflow observer for the
> > nodeInfoBar each would have to continually communicate state, besides, we
> > would then have to calculate element positions twice, once for the box model
> > highlighter and once for the nodeInfoBar. It makes more sense to leave it as
> > it is.
> 
> I'm not sure to fully understand this.
> 
> "mutations" are for DOM updates. We should update #ID.class1.classN on
> mutations.
> 
> "mozAfterPaint" are for paint updates. We should update the highlighter on
> paint.
> 
> "reflows" are for layout changes. We should update the dimensions on reflows.
> 
> A reflow doesn't always trigger a paint. So we can't rely on the
> mozAfterPaint event to update the dimensions. mutations have nothing to do
> with layout.
> 
> So when is the infobar updated? On scroll and paint if I'm not mistaken? On
> mutations as well? But for sure not on reflow.

Ah, so you are saying that we should *also* update on reflow because the nodeInfoBar contains ids, classnames and dimensions.

Now I understand.

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-node-dims-to-nodeinfobar-718250.patch

(In reply to Joe Walker [:jwalker] from comment #15)
> Comment on attachment 8340020 [details] [diff] [review]
> add-node-dims-to-nodeinfobar-718250.patch
> 
> Review of attachment 8340020 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This show out of date info if the dimensions change during inspection
> doesn't it?

Nope, it is as up to date as the highlighter itself.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #17)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #16)
> > Comment on attachment 8340020 [details] [diff] [review]
> > add-node-dims-to-nodeinfobar-718250.patch
> > 
> > (In reply to Joe Walker [:jwalker] from comment #15)
> > > Comment on attachment 8340020 [details] [diff] [review]
> > > add-node-dims-to-nodeinfobar-718250.patch
> > > 
> > > Review of attachment 8340020 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This show out of date info if the dimensions change during inspection
> > > doesn't it?
> > 
> > Nope, it is updated on mutations.
> 
> On mutations? It should get updated on reflow.
> 

Agreed but Patrick is working on that in bug 997198.

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8415214 [details] [diff] [review]
add-node-dims-to-nodeinfobar-718250.patch

In fact, I should add a quick test to this.

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Added test

Try:
https://tbpl.mozilla.org/?tree=Try&rev=3314c98fc5f7

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Fixed tests on try

Dimensions can vary from try server to try server. I have removed checks from the test for cases where these results could vary.

Comment 28

[Joe Walker [:jwalker]]

The reflow listener is close enough to landing that I think we should wait for that and get this right, shouldn't we? Also could you ask Patrick or Paul for a review, please.

Comment 29

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8415269 [details] [diff] [review]
Fixed tests on try

(In reply to Joe Walker [:jwalker] from comment #28)
> The reflow listener is close enough to landing that I think we should wait
> for that and get this right, shouldn't we? Also could you ask Patrick or
> Paul for a review, please.

It is as "right" as all the other information that we have in the highlighter. Anyhow, assigning to pbrosset as requested.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8415269 [details] [diff] [review]
Fixed tests on try

Review of attachment 8415269 [details] [diff] [review]:
-----------------------------------------------------------------

This works well for me.
I only have 2 minor-ish comments below, but I'm happy if we land this as is.

I found a bug related to the highlihgter not updating to animated elements while picking an element from the page, but this is not related your changes so I'll file a new bug.

::: toolkit/devtools/server/actors/highlighter.js
@@ +734,5 @@
> +      // Dimensions
> +      let dimensionBox = this.nodeInfo.dimensionBox;
> +      let rect = this.currentNode.getBoundingClientRect();
> +      dimensionBox.textContent = Math.ceil(rect.width) + " x " +
> +                                 Math.ceil(rect.height);

The only comment I have about the _updateInfobar function is that I'd rather do an early return `if (!this.currentNode) { return }` so that one level of indentation can be removed.

Also, isn't there a way to reuse the dimensions gotten earlier in _highlightBoxModel instead of calling again getBoundingClientRect()?
I'm not sure about this one because the show function seems to be calling _showInfobar before _update, which is weird, because we only know if the infobar should be displayed after we've called _update.

Comment 31

[Patrick Brosset <:pbro>]

Oh and I forgot about the tests. You said you fixed failing tests but didn't post the try build URL. Are tests passing now?
Wouldn't it be simpler to have a test HTML page that uses fixed dimensions? So that you can be sure test elements have the same dimensions on all try platforms?

Comment 32

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Comment on attachment 8415269 [details] [diff] [review]
> Fixed tests on try
> 
> Review of attachment 8415269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This works well for me.
> I only have 2 minor-ish comments below, but I'm happy if we land this as is.
> 
> I found a bug related to the highlihgter not updating to animated elements
> while picking an element from the page, but this is not related your changes
> so I'll file a new bug.
> 
> ::: toolkit/devtools/server/actors/highlighter.js
> @@ +734,5 @@
> > +      // Dimensions
> > +      let dimensionBox = this.nodeInfo.dimensionBox;
> > +      let rect = this.currentNode.getBoundingClientRect();
> > +      dimensionBox.textContent = Math.ceil(rect.width) + " x " +
> > +                                 Math.ceil(rect.height);
> 
> The only comment I have about the _updateInfobar function is that I'd rather
> do an early return `if (!this.currentNode) { return }` so that one level of
> indentation can be removed.
> 

Will do.

> Also, isn't there a way to reuse the dimensions gotten earlier in
> _highlightBoxModel instead of calling again getBoundingClientRect()?

We could we are dealing with 16 vertices here so caching and comparing the two would be complex and slow. Also, we are not only concerned with the bounding rect here as e.g. if somebody sets padding - 1 and border + 1 we need to detect that.

> I'm not sure about this one because the show function seems to be calling
> _showInfobar before _update, which is weird, because we only know if the
> infobar should be displayed after we've called _update.

I think that is because if the infobar is not shown and we call update we bail out. Maybe we should always update and switch these back to their logical order. I will take a look.

Comment 33

[Jeff Griffiths (:canuckistani) (:⚡︎)]

From uservoice:

I've been finding myself wishing the box model view showed positioning information. Specifically, I'd like to see the following attributes:

* display
* position
* top, right, bottom, left
* z-index

( http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/5714404-show-total-dimensions-in-the-box-model )

Comment 34

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Sorry, also from uservoice:

http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/5893514-positioning-information-in-box-model-view

Comment 35

[Patrick Brosset <:pbro>]

(In reply to Jeff Griffiths (:canuckistani) from comment #33)
> From uservoice:
> 
> I've been finding myself wishing the box model view showed positioning
> information. Specifically, I'd like to see the following attributes:
I think this refers to the box-model panel in the inspector sidebar, not to the highlighter.
> 
> * display
Interesting, we could indeed add this, I don't think there's a bug for this yet.
> * position
Is already present in the box-model panel
> * top, right, bottom, left
Is not yet there, but a bug is in progress I believe: bug 940275
> * z-index
Yep, could add this too.
> 
> (
> http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/
> suggestions/5714404-show-total-dimensions-in-the-box-model )

Comment 36

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #31)
> Oh and I forgot about the tests. You said you fixed failing tests but didn't
> post the try build URL. Are tests passing now?
> Wouldn't it be simpler to have a test HTML page that uses fixed dimensions?
> So that you can be sure test elements have the same dimensions on all try
> platforms?

Yup:
https://tbpl.mozilla.org/?tree=Try&rev=dc8b047a3829

Comment 37

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-node-dims-to-nodeinfobar-718250.patch

Return early in _updateInfobar() when currentNode is undefined.

Comment 38

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: add-node-dims-to-nodeinfobar-718250.patch

Fixed obvious bug.

Comment 39

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/531774aeb6c2

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/531774aeb6c2

Comment 41

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Aurora 32.0a2 20140721004001 under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.9.4 - the dimensions are now displayed when elements are selected in Web Inspector.