Last Comment Bug 735691 - Make toolbar buttons borderless in the default state
: Make toolbar buttons borderless in the default state
Status: RESOLVED FIXED
: addon-compat
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Windows 7
: -- enhancement with 3 votes (vote)
: Firefox 14
Assigned To: Dão Gottwald [:dao]
:
Mentors:
: 675595 (view as bug list)
Depends on: 738304 738320 738548 747690 761990 775191
Blocks: 700246 734373
  Show dependency treegraph
 
Reported: 2012-03-14 08:37 PDT by Dão Gottwald [:dao]
Modified: 2012-07-18 11:40 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP patch (16.61 KB, patch)
2012-03-14 08:44 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
WIP patch v2 (20.46 KB, patch)
2012-03-14 12:50 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch (21.01 KB, patch)
2012-03-14 16:26 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v2 (22.29 KB, patch)
2012-03-15 05:58 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch v3 (22.29 KB, patch)
2012-03-15 08:48 PDT, Dão Gottwald [:dao]
shorlander: review-
Details | Diff | Splinter Review
patch v4 (22.43 KB, patch)
2012-03-21 05:32 PDT, Dão Gottwald [:dao]
shorlander: review+
Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-03-14 08:37:36 PDT
I'll lay some groundwork here, we'll probably want to do additional polishing in bug 734373.
Comment 1 Dão Gottwald [:dao] 2012-03-14 08:44:58 PDT
Created attachment 605777 [details] [diff] [review]
WIP patch

This may be completely busted, I haven't tested it yet.
Comment 2 Dão Gottwald [:dao] 2012-03-14 12:50:26 PDT
Created attachment 605905 [details] [diff] [review]
WIP patch v2
Comment 3 Dão Gottwald [:dao] 2012-03-14 12:51:22 PDT
*** Bug 675595 has been marked as a duplicate of this bug. ***
Comment 4 Dão Gottwald [:dao] 2012-03-14 16:26:14 PDT
Created attachment 606020 [details] [diff] [review]
patch
Comment 5 Dão Gottwald [:dao] 2012-03-15 05:58:33 PDT
Created attachment 606178 [details] [diff] [review]
patch v2

fixed a few glitches I discovered
Comment 6 Dão Gottwald [:dao] 2012-03-15 08:48:31 PDT
Created attachment 606235 [details] [diff] [review]
patch v3

... and due to the previous changes, the svg mask needed another update ...
Comment 7 Stephen Horlander [:shorlander] 2012-03-15 10:47:57 PDT
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 2012-03-15 10:50:51 PDT
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] 2012-03-15 10:52:07 PDT
In small icon mode buttons are already borderless.
Comment 10 Valerio 2012-03-15 10:59:28 PDT
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] 2012-03-15 11:04:26 PDT
I filed Bug 734326 for getting the bookmarks appearance updated :)
Comment 12 Stephen Horlander [:shorlander] 2012-03-15 11:06:37 PDT
(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 2012-03-15 11:21:00 PDT
I filed Bug 736179 for update the Small Icons mode
Comment 14 Dão Gottwald [:dao] 2012-03-15 11:26:42 PDT
(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] 2012-03-20 09:50:04 PDT
Stephen, ping?
Comment 16 Stephen Horlander [:shorlander] 2012-03-21 02:51:14 PDT
(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] 2012-03-21 04:46:16 PDT
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] 2012-03-21 05:32:08 PDT
Created attachment 607918 [details] [diff] [review]
patch v4
Comment 19 Dão Gottwald [:dao] 2012-03-21 05:34:49 PDT
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] 2012-03-21 08:18:11 PDT
I tested this now, the two issues are fixed.
Comment 21 Stephen Horlander [:shorlander] 2012-03-21 08:48:32 PDT
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] 2012-03-21 09:07:27 PDT
> > +@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] 2012-03-22 09:38:37 PDT
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] 2012-03-22 09:41:00 PDT
(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] 2012-04-12 14:48:43 PDT
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 2012-04-12 15:12:49 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.