735691 - Make toolbar buttons borderless in the default state

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement)

Description

[Dão Gottwald [:dao]]

I'll lay some groundwork here, we'll probably want to do additional polishing in bug 734373.

Comment 1

[Dão Gottwald [:dao]]

Attachment: WIP patch

This may be completely busted, I haven't tested it yet.

Comment 2

[Dão Gottwald [:dao]]

Attachment: WIP patch v2

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

fixed a few glitches I discovered

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v3

... and due to the previous changes, the svg mask needed another update ...

Comment 7

[Stephen Horlander [:shorlander]]

This is mostly for not showing a button in the default state and extending the hit box right?

So I shouldn't worry too much about padding, style, etc.?

Comment 8

[Valerio]

Can you also apply this style in small icons mode and to the buttons of the bookmarks toolbar in order to have more consistency?

Comment 9

[Siddhartha Dugar [:sdrocking]]

In small icon mode buttons are already borderless.

Comment 10

[Valerio]

yes, but they use -moz-appearance:toolbarbutton as well as the buttons of the bookmarks toolbar so they look different when you hover and press them

Comment 11

[Siddhartha Dugar [:sdrocking]]

I filed Bug 734326 for getting the bookmarks appearance updated :)

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Valerio from comment #10)
> yes, but they use -moz-appearance:toolbarbutton as well as the buttons of
> the bookmarks toolbar so they look different when you hover and press them

Yes, part of the Australis work is going to be gaining consistency everywhere.

Comment 13

[Valerio]

I filed Bug 736179 for update the Small Icons mode

Comment 14

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander from comment #7)
> This is mostly for not showing a button in the default state and extending
> the hit box right?
> 
> So I shouldn't worry too much about padding, style, etc.?

yep

Comment 15

[Dão Gottwald [:dao]]

Stephen, ping?

Comment 16

[Stephen Horlander [:shorlander]]

(In reply to Dão Gottwald [:dao] from comment #15)
> Stephen, ping?

Sorry for the delay, traveling wrecked my schedule. Looking at this now.

Comment 17

[Stephen Horlander [:shorlander]]

Comment on attachment 606235 [details] [diff] [review]
patch v3

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

> +@navbarLargeIcons@ .toolbarbutton-1[type=menu] > .toolbarbutton-icon {
> +  -moz-padding-end: 17px;
> +}
> +
> +@navbarLargeIcons@ .toolbarbutton-1 > .toolbarbutton-menu-dropmarker {
> +  -moz-margin-start: -15px;
> +}

This adds the padding to #feed-button and separated forward and back buttons even though the dropdown is hidden.

Also the button affordance on the forward button is missing between transitions: http://cl.ly/0q1u0v3C0X3t0J011E3f

Otherwise it looks great!

Comment 18

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 607918 [details] [diff] [review]
patch v4

I *think* this should do it. Being on Linux, I haven't tested this yet :/

Comment 20

[Dão Gottwald [:dao]]

I tested this now, the two issues are fixed.

Comment 21

[Stephen Horlander [:shorlander]]

Comment on attachment 607918 [details] [diff] [review]
patch v4

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

(In reply to Dão Gottwald [:dao] from comment #20)
> I tested this now, the two issues are fixed.

Yes, thank you! :)


> +@navbarLargeIcons@ .toolbarbutton-1[type=menu] {
> +  padding-left: 5px;
> +  padding-right: 5px;
> +}

This is throwing off the spacing for those three special-case menu buttons (http://cl.ly/112c003t0O1f0Y3J2d45). r+ with that.

Thanks!

Comment 22

[Dão Gottwald [:dao]]

> > +@navbarLargeIcons@ .toolbarbutton-1[type=menu] {
> > +  padding-left: 5px;
> > +  padding-right: 5px;
> > +}
> 
> This is throwing off the spacing for those three special-case menu buttons
> (http://cl.ly/112c003t0O1f0Y3J2d45). r+ with that.

added :not(#back-button):not(#forward-button):not(#feed-button)

http://hg.mozilla.org/mozilla-central/rev/e9938aab62e2

Comment 23

[Girish Sharma [:Optimizer]]

This makes certain custom (non Firefox) toolbar buttons with type as menu act weird.
They don't have any hover state and the drop-marker which should appear to right of text (when no image is displayed) appears behind the text in the center of the button.
Should I file a new bug.
This is related to this bug only right ?

Comment 24

[Dão Gottwald [:dao]]

(In reply to Girish Sharma from comment #23)
> Should I file a new bug.

Yes please.

> This is related to this bug only right ?

Chances are it is.

Comment 25

[Nils Maier [:nmaier]]

Don't forget about addon-compat ;)

Jorge announced a margin/width/height "trick" he lifted from Firefox 4 sources at the time:
http://blog.mozilla.com/addons/2010/12/02/toolbar-buttons-in-firefox-4/
(scroll down to the Windows section)
If authors implemented that, addons-mxr indeed seems to list a few instances of (popular) add-ons implementing this, then the hover frame will get too big. It's not a big deal, but it is not pretty either, especially with type=menu buttons.
Examples (my own extensions): DownThemAll! and MinTrayR

Comment 26

[rob64rock]

(In reply to Nils Maier [:nmaier] from comment #25)
> Don't forget about addon-compat ;)
> 
> Jorge announced a margin/width/height "trick" he lifted from Firefox 4
> sources at the time:
> http://blog.mozilla.com/addons/2010/12/02/toolbar-buttons-in-firefox-4/
> (scroll down to the Windows section)
> If authors implemented that, addons-mxr indeed seems to list a few instances
> of (popular) add-ons implementing this, then the hover frame will get too
> big. It's not a big deal, but it is not pretty either, especially with
> type=menu buttons.
> Examples (my own extensions): DownThemAll! and MinTrayR
That issue was brought up in bug 738320 which was 'Marked Invalid' since that issue supposedly can be corrected by the extension developer's end, but it was never stated exactly how. So you may want to post a bug comment on bug 738320 and ask them to be more specific on that issue.