DevTools: make toolbar button click targets square

DevTools: make toolbar button click targets square

BUG=498750
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196980

[pfeldman, 2015-06-11 15:51:03]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2015-06-11 15:52:58]

http://grab.by/I03C

[pfeldman, 2015-06-11 15:53:30]

http://grab.by/I03I

[dgozman, 2015-06-11 16:14:12]

This patch asks to remove material experiment.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/emulation/responsiveDesignView.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/emulation/responsiveDesignView.css:320: /*
border-right: 2px solid rgb(0, 0, 0); */
commented css

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/main/module.json (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:163: "order": 100,
100 is for close button. 99?

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:164: "actionId":
"main.toggle-element-search"
Remove actionId.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:174: "className":
"WebInspector.Main.WarningErrorCounter",
Why swapped with toggle drawer?

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sidebarPane.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sidebarPane.css:75: /* border-top: none; */
commented css

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sources/sourcesPanel.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:39: height: 28px;
You've removed material from inspectorViewToolbar, and left it here. Let's
align.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:44: overflow: hidden;
merge into first rule

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/timelinePanel.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/timeline/timelinePanel.css:31: border-bottom: 1px
solid #f3f3f3;
Border of the same color as background? Remove it then?

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:113:
this._items[i].setVisible(!previousIsSeparator);
What if I explicitly call setVisible on a separator?

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/toolbar.css (left):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:95: .narrow .long-click-glyph {
Remove makeNarrow.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:9: padding: 0 2px;
This may strike when two toolbars are near each other.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:49: border: none;
Two "border" declarations in the same rule.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:95: .toolbar-item.checkbox:hover,
Should this be checkbox:enabled?

[pfeldman, 2015-06-11 16:38:07]

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/emulation/responsiveDesignView.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/emulation/responsiveDesignView.css:320: /*
border-right: 2px solid rgb(0, 0, 0); */
On 2015/06/11 16:14:11, dgozman wrote:
> commented css

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/main/module.json (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:163: "order": 100,
On 2015/06/11 16:14:12, dgozman wrote:
> 100 is for close button. 99?

These are different toolbars.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:164: "actionId":
"main.toggle-element-search"
On 2015/06/11 16:14:12, dgozman wrote:
> Remove actionId.

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/main/module.json:174: "className":
"WebInspector.Main.WarningErrorCounter",
On 2015/06/11 16:14:11, dgozman wrote:
> Why swapped with toggle drawer?

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sidebarPane.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sidebarPane.css:75: /* border-top: none; */
On 2015/06/11 16:14:12, dgozman wrote:
> commented css

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/sources/sourcesPanel.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:39: height: 28px;
On 2015/06/11 16:14:12, dgozman wrote:
> You've removed material from inspectorViewToolbar, and left it here. Let's
> align.

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/sources/sourcesPanel.css:44: overflow: hidden;
On 2015/06/11 16:14:12, dgozman wrote:
> merge into first rule

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/timeline/timelinePanel.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/timeline/timelinePanel.css:31: border-bottom: 1px
solid #f3f3f3;
On 2015/06/11 16:14:12, dgozman wrote:
> Border of the same color as background? Remove it then?

Those are different elements. Timeline toolbar is white.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/Toolbar.js (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/Toolbar.js:113:
this._items[i].setVisible(!previousIsSeparator);
On 2015/06/11 16:14:12, dgozman wrote:
> What if I explicitly call setVisible on a separator?

stack overflow!

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
File Source/devtools/front_end/ui/toolbar.css (right):

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:9: padding: 0 2px;
On 2015/06/11 16:14:12, dgozman wrote:
> This may strike when two toolbars are near each other.

That's probably fine for now?

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:49: border: none;
On 2015/06/11 16:14:12, dgozman wrote:
> Two "border" declarations in the same rule.

Done.

https://codereview.chromium.org/1176343002/diff/20001/Source/devtools/front_e...
Source/devtools/front_end/ui/toolbar.css:95: .toolbar-item.checkbox:hover,
Input is even deeper there + we don't disable them (yet?!)

[dgozman, 2015-06-11 21:12:20]

lgtm

[pfeldman, 2015-06-11 22:02:27]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-06-11 22:02:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176343002/60001

[commit-bot: I haz the power, 2015-06-11 23:45:33]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196980