Closed Bug 735691 Opened 9 years ago Closed 9 years ago

Make toolbar buttons borderless in the default state

Categories

(Firefox :: Theme, enhancement)

All
Windows 7
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 14

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 5 obsolete files)

I'll lay some groundwork here, we'll probably want to do additional polishing in bug 734373.
Attached patch WIP patch (obsolete) — Splinter Review
This may be completely busted, I haven't tested it yet.
Attached patch WIP patch v2 (obsolete) — Splinter Review
Attachment #605777 - Attachment is obsolete: true
Duplicate of this bug: 675595
Attached patch patch (obsolete) — Splinter Review
Attachment #605905 - Attachment is obsolete: true
Attachment #606020 - Flags: review?(shorlander)
Attached patch patch v2 (obsolete) — Splinter Review
fixed a few glitches I discovered
Attachment #606020 - Attachment is obsolete: true
Attachment #606020 - Flags: review?(shorlander)
Attachment #606178 - Flags: review?(shorlander)
Attached patch patch v3 (obsolete) — Splinter Review
... and due to the previous changes, the svg mask needed another update ...
Attachment #606178 - Attachment is obsolete: true
Attachment #606178 - Flags: review?(shorlander)
Attachment #606235 - Flags: review?(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.?
Can you also apply this style in small icons mode and to the buttons of the bookmarks toolbar in order to have more consistency?
In small icon mode buttons are already borderless.
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
I filed Bug 734326 for getting the bookmarks appearance updated :)
(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.
I filed Bug 736179 for update the Small Icons mode
(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
Stephen, ping?
(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 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!
Attachment #606235 - Flags: review?(shorlander) → review-
Attached patch patch v4Splinter Review
Attachment #606235 - Attachment is obsolete: true
Comment on attachment 607918 [details] [diff] [review]
patch v4

I *think* this should do it. Being on Linux, I haven't tested this yet :/
Attachment #607918 - Flags: review?(shorlander)
I tested this now, the two issues are fixed.
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!
Attachment #607918 - Flags: review?(shorlander) → review+
> > +@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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Blocks: 700246
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 ?
(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.
Depends on: 738304
Depends on: 738320
Depends on: 738548
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
Keywords: addon-compat
(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.
Depends on: 747690
Depends on: 761990
Depends on: 775191
You need to log in before you can comment on or make changes to this bug.