Closed Bug 1141571 Opened 5 years ago Closed 5 years ago

Add a legend to the Box Model tool

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox51 verified, firefox52 verified, firefox53 verified)

VERIFIED FIXED
Firefox 40
Tracking Status
firefox51 --- verified
firefox52 --- verified
firefox53 --- verified

People

(Reporter: janx, Assigned: janx)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(4 files, 2 obsolete files)

The Box Model inspector is a great tool, but it's suboptimal that you have to know what each color is for by heart.

Therefore, it would be nice to show a legend underneath like:

    [#] margin (# is apple green)
    [#] border (# is grey)
    [#] padding (# is purple)
    [#] bounding client rect (# is sky blue)
Whiteboard: [devedition-40]
Note: Hovering on a particular color does indicate what it represents, but as almost invisible white text on apple green background in the bottom right corner.

A proper legend would be more discoverable and readable, even in screenshots.
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Setting devedition-40 bugs to P2, filter on C17C996C-A0BE-4153-8057-FAB642D0746D
Priority: -- → P2
I'll try to do this tomorrow.
Assignee: nobody → janx
Attached image boxmodel-legend.png (obsolete) —
Attached screenshot.

Note: For the color preview boxes, I chose a dotted outline with an offset of -1px, because it resembles the original more. However, it seems that on some zoom levels the offset looks more like -2px (probably a rounding error).

Maybe there is a better way to do the dotted outline?
Comment on attachment 8591640 [details] [diff] [review]
Add a legend to the Box Model tool.

(git bz forgot about the r? flag again)

Brian, please have a look.

This patch…
1. replaces the useless white-on-light-green, bottom-right tooltip with actual hover tooltips ("title" attribute).
2. defines the box model colors as CSS variables.
3. reuses these variables in a legend underneath the box model view.
Attachment #8591640 - Flags: review?(bgrinstead)
For info, we do have a legend already. It appears when you hover over regions, for now it's just the name of the region and it's placed in the margin box (bottom right corner).
You might not have seen it because we have a regression at the moment whereby it's not easily visible anymore (white text on yellow background): see bug 1150507.
I do like the idea of a more prominent legend as you propose here (although in the screenshot you attached, I think it might be a little too big), but if we go for this, we'll need to get rid of the other legend.
I think it might be better to have the legend text appearing inside the box model similar to chrome and safari. It would provide a better UX and avoid this color matching the user would have to go through to determine whether or not it is a margin, padding, or border.
Thanks for the feedback!

Actually, I had noticed the bottom-right text-on-hover (see comment 1 and comment 6), but I believe that ― even when readable ― it's not discoverable, and so it doesn't help with users who get confused about the colors and don't hover (this actually happened to me several times). And it doesn't help with screenshots either.

I replaced this weird tooltip mechanism with an actual `title` attribute, so that when you hover over any of the values, you still see the "margin-top" or "border-bottom" text in an actual tooltip. The legend is just there for convenience, but it doesn't take up valuable space (it can overflow at the bottom) and can be ignored as needed.
Blocks: 1150496, 1150507
TL;DR: With my patch, users still see useful information on hover, but can also benefit from a proper legend if they need it.
Comment on attachment 8591640 [details] [diff] [review]
Add a legend to the Box Model tool.

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

Would you mind splitting this into two patches?  One that just replaces the old 'tooltip' mechanism with the title attribute, and a second that adds the legend.

Regarding the legend, I wonder if it would be easier for the user to have the text 'margin', 'border', 'padding' in the top left of the corresponding region.  See how Firebug does it, for example (I'll attach a screenshot momentarily)
Attachment #8591640 - Flags: review?(bgrinstead)
Attached image firebug-regions.png
See how there is text in the corner of each region.  I think this is nice since it's overlaid directly on what you are looking at
Status: NEW → ASSIGNED
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Created attachment 8591836 [details]
> firebug-regions.png
> 
> See how there is text in the corner of each region.  I think this is nice
> since it's overlaid directly on what you are looking at

I would definitely prefer this approach! Having the legend overlays directly in the box model reduces that extra step of looking at the legend and matching it with the box.
(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8591640 [details] [diff] [review]
> Add a legend to the Box Model tool.
> 
> Review of attachment 8591640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would you mind splitting this into two patches?  One that just replaces the
> old 'tooltip' mechanism with the title attribute:
I have a patch in bug 1151956 ready to go that replaces all tooltip attributes with title attributes. But I wasn't as thorough as Jan, I didn't remove the old tooltip mechanism ... I havn't even seen we had this in place :|
(In reply to Gabriel Luong (:gl) from comment #13)
> (In reply to Brian Grinstead [:bgrins] from comment #12)
> > Created attachment 8591836 [details]
> > firebug-regions.png
> > 
> > See how there is text in the corner of each region.  I think this is nice
> > since it's overlaid directly on what you are looking at
> 
> I would definitely prefer this approach! Having the legend overlays directly
> in the box model reduces that extra step of looking at the legend and
> matching it with the box.

OK fair enough, I will use this approach instead.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #14)
> I have a patch in bug 1151956 ready to go that replaces all tooltip
> attributes with title attributes. But I wasn't as thorough as Jan, I didn't
> remove the old tooltip mechanism ... I havn't even seen we had this in place
> :|

Whoops, sorry for the duplicate work!

You can either rebase your patch on top of mine, or land yours first. I'm fine either way :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5915ee441cd1
Attachment #8591640 - Attachment is obsolete: true
Attachment #8592194 - Flags: review?(pbrosset)
Attachment #8592194 - Flags: review?(pbrosset) → review+
Attachment #8591646 - Attachment is obsolete: true
Sheriffs, please land the other patch; this one still needs r+.

Changed legend as suggested, I'll upload a screenshot.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88cfdec022c8
Attached image boxmodel-legend2.png
Screenshot of new box model legend.
Attachment #8592281 - Flags: review?(bgrinstead)
Comment on attachment 8592281 [details] [diff] [review]
Add a legend to the Box Model tool.

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

Looks good
Attachment #8592281 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/integration/fx-team/rev/52a143ca5a1a
https://hg.mozilla.org/integration/fx-team/rev/0ceb71fdbe2f
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/52a143ca5a1a
https://hg.mozilla.org/mozilla-central/rev/0ceb71fdbe2f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Duplicate of this bug: 1150507
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
I reproduced this issue using 47.0a1, build ID: 20160202030232, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 51.0b11, build ID: 20170103031119 and Fx 52.0a2, build ID: 20170105004018 and Fx53.0a1, build ID: 20170104030214, on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 LTS.

Cheers!
Status: RESOLVED → VERIFIED
QA Contact: cristian.comorasu
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.