Closed Bug 1261049 Opened 4 years ago Closed 4 years ago

Clear button position isn't consistent among tabs (several pixels difference)

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox48 verified)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: arni2033, Assigned: jsnajdr)

References

(Depends on 1 open bug)

Details

(Whiteboard: [devtools-ux][btpp-backlog])

Attachments

(2 files, 1 obsolete file)

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160330030326
STR:
1. Enable all tabs in devtools
2. Switch between Canvas, Performance, Memory, Network tabs

AR: In each tab clear button takes unique place
ER: Clear buttons should share the same position


Notes:
1) And also, probably, Netmonitor's toolbar shouldn't be so thick. No other toolbar has filter field
   with so big height. I think it's a part of this bug anyway.
2) This bug may be beyond my competence; probably somebody is already taking care of it somewhere,
   but I'm better not to risk letting this bug appear in release w/o developers knowing
3) References:  bug 1223037, bug 1220208, bug 1251256
Assignee: nobody → jsnajdr
This video was recorded on HiDPI (DPI = 125%). But even with  DPI = 100%  it is almost the same.
Screencast:
> https://dl.dropboxusercontent.com/s/nwor6gfugui0di6/bug%201261049%20comment%201.webm?dl=0
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Oh, and also, if you ever planned to use Fitt's law to Clear button, then now is the best moment.

Applying Fitt's law to Clear button would mean that it's placed right next to the left border of devtools (so if Firefox window is maximized, then clicking on the leftmost pixel of toolbar will click Clear button). Fitt's law is already used for left scrollbutton in breadcrumbs.
I'd suggest we stick with the positioning that's in the console (which also seems to be same as in the netmonitor).  That fits with the general position for buttons with .devtools-toolbar.  Changing that convention so it instead lines up on the left border is a separate discussion.
Whiteboard: [devtools-ux]
Priority: -- → P3
Whiteboard: [devtools-ux] → [devtools-ux][btpp-backlog]
This patch fixes the button position issues. There are 3 possible causes:

1. Different margins for toolbarbuttons inside a devtools-toolbarbutton-group and outside. Happens in the Performance tool. Fixed by removing the custom styling for devtools-toolbarbutton-group and using margin: 2px 1px for all devtools-toolbarbutton's (in toolbars.css).

2. Different margins for HTML devtools-button and XUL devtools-toolbarbutton. Happens in the Memory tool. Fixed by defining the same margin and padding for both (in toolbars.css) and removing the extra styling from memory.css.

3. Redefined padding-left for a particular toolbar. Happens (potentially, if it had a clear icon) in Inspector. Fixed by removing the extra styling from inspector.css.

I also fixed the styles for buttons' :-moz-focusring so that the outline looks reasonably when focused.

Because of the style unification for toolbar-button-group, the default margin for a toolbar button changed from "2px 3px" to "2px 1px". The spacing is still wide enough and the toolbars look more compact. However, I had to verify that this doesn't break anything. That required changes in animation inspector and responsive.html:

- animation inspector has a customized toolbar style: more compact, with vertical separators between buttons. I fixed the margin and padding styles in animationinspector.css, because the layout would be broken otherwise. Also, removed a devtools-toolbarbutton class from one of the labels, because it makes no sense there.
- responsive.html also redefines the devtools-button appearance almost completely, added a padding definition so that it remains the same as before this patch.

Requesting review from bgrins, please assign to someone else if there's a better person for this.
Attachment #8739410 - Flags: ui-review?(hholmes)
Attachment #8739410 - Flags: review?(bgrinstead)
Comment on attachment 8739410 [details] [diff] [review]
Clear button position isn't consistent among tabs

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

Forwarding to Tim (if available)
Attachment #8739410 - Flags: review?(bgrinstead) → review?(ntim.bugs)
Comment on attachment 8739410 [details] [diff] [review]
Clear button position isn't consistent among tabs

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

r=me assuming you've tested this across platforms.
Attachment #8739410 - Flags: review?(ntim.bugs) → review+
Comment on attachment 8739410 [details] [diff] [review]
Clear button position isn't consistent among tabs

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

::: devtools/client/themes/memory.css
@@ -119,5 @@
> - * Due to toolbar styles of `.devtools-toolbarbutton:not([label])` which overrides
> - * .devtools-toolbarbutton's min-width of 78px, reset the min-width.
> - */
> -#import-snapshot,
> -#clear-snapshots {

Just tested this on OSX, and I think we should keep this rule for #import-snapshot, because otherwise the import button looks tiny.
- fixed the #import-snapshot button style in Memory Tool: turned out that only flex-grow: 1 property is needed.
- did a try run and used the Linux and Windows builds to verify platforms other than OS X
Attachment #8739939 - Flags: review?(ntim.bugs)
Attachment #8739410 - Attachment is obsolete: true
Attachment #8739410 - Flags: ui-review?(hholmes)
Attachment #8739939 - Flags: review?(ntim.bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cad934baac3c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Successfully reproduce this bug on Nightly 48.0a1 (2016-03-31) (Build ID: 20160331030231) on Linux, 64 Bit by following the comment 0's instruction !

This Bug is now verified as fixed on Latest Firefox Nightly (2016-04-14) 

Build ID: 20160414030247
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
OS: Linux 3.19.0-58-generic x86-64
QA Whiteboard: [testday-20160415]
Status: RESOLVED → VERIFIED
Clear button still twitches up and down when I switch between console and netmonitor. Is that according to new design? Should I ignore that twitching or file a separate bug regarding netmonitor?
Flags: needinfo?(jsnajdr)
Hello Arni, I cannot reproduce the up&down twitching you're describing. I'm on OS X. Could you post a video again, and details about your platform?

However, I found that the icon is not aligned in the Memory tool - it's 1px left relative to the other tools. So I'll have to have another look at this bug anyway.
Flags: needinfo?(jsnajdr)
(In reply to arni2033 from comment #13)
> Clear button still twitches up and down when I switch between console and
> netmonitor. Is that according to new design? Should I ignore that twitching
> or file a separate bug regarding netmonitor?

I'm not noticing the clear button moving, but maybe it's content next to it that is making it look like it's moving? or maybe it's specific to your platform (I'm on OSX) ?

(In reply to Jarda Snajdr [:jsnajdr] from comment #14)
> However, I found that the icon is not aligned in the Memory tool - it's 1px
> left relative to the other tools. So I'll have to have another look at this
> bug anyway.
Filed bug 1267433
Flags: needinfo?(arni2033)
Flags: needinfo?(arni2033)
Depends on: 1267751
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.