Closed Bug 718250 Opened 12 years ago Closed 10 years ago

We should display the dimensions of the node in the NodeInfobar

Categories

(DevTools :: Inspector, defect, P2)

x86
All
defect

Tracking

(firefox32 verified)

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified

People

(Reporter: paul, Assigned: miker)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
I agree with this. It will be very usefull indeed.
it should also do double duty to show width and height of images when selected.
also, I think I'm leaning towards the toolbar for this.
The layout view patch adds the dimension of the node in the toolbar
Depends on: 683954
triage, filter on centaur
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Not duplicate because we are not sure to have the layout button in the toolbar.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Priority: -- → P2
The layout view doesn't add anything in the toolbar anymore.
WONTFIX (it's in the BoxModel tab in the toolbox).

Bug triage, filter on PINKISBEAUTIFUL
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → WONTFIX
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.
(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
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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 ?
Ah, the first this in my previous comment is referring to bug 766526
Summary: We should display the dimensions of the node somewhere in the inspector → We should display the dimensions of the node in the NodeInfobar
Assignee: nobody → mratcliffe
Depends on: 916443
No longer depends on: 683954
Very simple patch based on pbrosset's remote highlighter.
Attachment #8340020 - Flags: review?(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?
Attachment #8340020 - Flags: review?(jwalker)
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.
Attachment #8340020 - Flags: review?(jwalker)
(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);
>   },
> };
Attachment #8340020 - Flags: review?(jwalker)
Attached patch Rebased (obsolete) — Splinter Review
(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.
Attachment #8340020 - Attachment is obsolete: true
Attachment #8356585 - Flags: review?(jwalker)
(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 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.
Attachment #8356585 - Flags: review?(jwalker)
(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.
Attachment #8356585 - Attachment is obsolete: true
Attachment #8415214 - Flags: review?(jwalker)
Comment on attachment 8415214 [details] [diff] [review]
add-node-dims-to-nodeinfobar-718250.patch

In fact, I should add a quick test to this.
Attachment #8415214 - Flags: review?(jwalker)
Attached patch Added test (obsolete) — Splinter Review
Try:
https://tbpl.mozilla.org/?tree=Try&rev=3314c98fc5f7
Attachment #8415214 - Attachment is obsolete: true
Attachment #8415222 - Flags: review?(jwalker)
Attached patch Fixed tests on try (obsolete) — Splinter Review
Dimensions can vary from try server to try server. I have removed checks from the test for cases where these results could vary.
Attachment #8415222 - Attachment is obsolete: true
Attachment #8415222 - Flags: review?(jwalker)
Attachment #8415269 - Flags: review?(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 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.
Attachment #8415269 - Flags: review?(jwalker) → review?(pbrosset)
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.
Attachment #8415269 - Flags: review?(pbrosset) → review+
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?
(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.
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 )
(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 )
(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
Return early in _updateInfobar() when currentNode is undefined.
Attachment #8415269 - Attachment is obsolete: true
Attachment #8430853 - Flags: review+
Blocks: 1018204
Fixed obvious bug.
Attachment #8430853 - Attachment is obsolete: true
Attachment #8434221 - Flags: review+
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/531774aeb6c2
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Keywords: verifyme
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
Depends on: 1099061
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.