859776 - Apply the Australis toolbar button design to split buttons on OS X

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement)

Description

[Josiah Bruner [:jsbruner]]

Bug 747140 has the Windows implementation for doing this on the zoom buttons. This bug will use the new Australis style for the split control buttons. Right now I believe that is simply the zoom buttons and the bookmark button.

Comment 1

[Josiah Bruner [:jsbruner]]

The bookmark separator must wait until the bookmark toolbar icon is moved via bug 748894. Therefore, I will have two patches, one for the zoom buttons and one for the bookmark button.

Comment 2

[Josiah Bruner [:jsbruner]]

Attachment: Zoom separators

Patch that adds separators to the zoom buttons. Not going to request reviews until dependent bugs are completed.

Comment 3

[Josiah Bruner [:jsbruner]]

According to mak, the bookmark separator has been added in the dependent patch, so I guess all we need is the zoom button splitter.

Comment 4

[Josiah Bruner [:jsbruner]]

Comment on attachment 735309 [details] [diff] [review]
Zoom separators

Just going to go ahead and request reviews.

Stephen, this patch needs Bug 856665's patch first.

Comment 5

[Stephen Horlander [:shorlander]]

Comment on attachment 735309 [details] [diff] [review]
Zoom separators

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

Visually this looks good :)

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 735309 [details] [diff] [review]
Zoom separators

>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -1031,16 +1031,18 @@
>                      tooltiptext="&fullScreenButton.tooltip;"/>
> 
>       <toolbaritem id="zoom-controls" class="chromeclass-toolbar-additional"
>                    title="&zoomControls.label;">
>         <toolbarbutton id="zoom-out-button" class="toolbarbutton-1"
>                        label="&fullZoomReduceCmd.label;"
>                        command="cmd_fullZoomReduce"
>                        tooltiptext="&zoomOutButton.tooltip;"/>
>+        <toolbarbutton id="zoom-separator" class="toolbar-separator"
>+                        label=""/>

Using a <toolbarbutton> for this doesn't make much sense. In fact you probably don't need an extra element at all, but could use ::before instead like we do for type="menu-button" buttons on Windows.

Comment 7

[Josiah Bruner [:jsbruner]]

Attachment: Patch (No ::after)

Updated patch. Essentially the same as before, but am using 'splitbuttonseparator' now instead of toolbarbutton.

Dão, is this acceptable? The reason I don't want to use ::before is because doing so would require extra work to make it function properly in the new "Menu Panel". If I use XUL, the panel code only needs to be slightly changed to not allow 'splitbuttonseparator's (Like it should be doing for spacers, separators, etc).

Comment 8

[Dão Gottwald [:dao]]

(In reply to Josiah Bruner [:JosiahOne] from comment #7)
> Created attachment 741809 [details] [diff] [review]
> Zoom separators V2
> 
> Updated patch. Essentially the same as before, but am using
> 'splitbuttonseparator' now instead of toolbarbutton.
> 
> Dão, is this acceptable? The reason I don't want to use ::before is because
> doing so would require extra work to make it function properly in the new
> "Menu Panel".

Can you elaborate on what extra work would be needed?

Comment 9

[Josiah Bruner [:jsbruner]]

(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to Josiah Bruner [:JosiahOne] from comment #7)
> > Created attachment 741809 [details] [diff] [review]
> > Zoom separators V2
> > 
> > Updated patch. Essentially the same as before, but am using
> > 'splitbuttonseparator' now instead of toolbarbutton.
> > 
> > Dão, is this acceptable? The reason I don't want to use ::before is because
> > doing so would require extra work to make it function properly in the new
> > "Menu Panel".
> 
> Can you elaborate on what extra work would be needed?

Well, essentially we would need to detect whether the zoom buttons are in the menu pane, and if so, do not draw this ::before element.

In addition, when I tried to use ::before, it would always draw on top of the zoom-in/zoom-out button, instead of to the side. From what I observed, it didn't seem to leave the individual zoom boxes. But that could simply because I was doing something wrong.

The menu pane should already have some implementation for not accepting toolbar separators, spacers, etc. At least from what Mike Conley has told me.

Comment 10

[Dão Gottwald [:dao]]

(In reply to Josiah Bruner [:JosiahOne] from comment #9)
> Well, essentially we would need to detect whether the zoom buttons are in
> the menu pane, and if so, do not draw this ::before element.

This would be needed either way. (See below.)

> In addition, when I tried to use ::before, it would always draw on top of
> the zoom-in/zoom-out button, instead of to the side. From what I observed,
> it didn't seem to leave the individual zoom boxes. But that could simply
> because I was doing something wrong.

This sounds like it should be easy to fix.

> The menu pane should already have some implementation for not accepting
> toolbar separators, spacers, etc. At least from what Mike Conley has told me.

That doesn't apply here, because your "splitbuttonseparator" is embedded in the customizable "toolbaritem" element. Customization only cares for the movable container, not for its arbitrary content.

Comment 11

[Josiah Bruner [:jsbruner]]

Alright. Well, I personally favor using XUL, especially in this case, since we are being contained by something, it seems easier to have:

zoom-out-button | zoom-separator | zoom-inbutton

But that is my personal preference, plus it makes it a little easier for theme creators to discover. Plus doing this allows any similar future elements to use the same implementation.

As for the Customization problem. I would propose we open another bug, that allows the ability to detect whether in element is in the Customization panel via CSS, similar to what we do with lw-themes, is in fullscreen, etc.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Josiah Bruner [:JosiahOne] from comment #11)
> Alright. Well, I personally favor using XUL, especially in this case, since
> we are being contained by something, it seems easier to have:
> 
> zoom-out-button | zoom-separator | zoom-inbutton
> 
> But that is my personal preference, plus it makes it a little easier for
> theme creators to discover. Plus doing this allows any similar future
> elements to use the same implementation.

Ideally future elements would have to do nothing other than using the <toolbaritem><toolbarbutton/><toolbarbutton/></toolbaritem> structure and things would just work.

Comment 13

[Josiah Bruner [:jsbruner]]

Well I guess it's up to you. I would use XUL, but if you think the separator should be done in CSS then that's what I'll do. Just in this case it seems to make more sense to use XUL. Now eventually if we wanted to change the entire zoom-container to using almost completely CSS, then we could.

I'm just saying that sense it is already a very structured XUL element, it could be confusing to theme developers (and perhaps our own developers), when they see the container, a zoom-out-button and a zoom-in-button. And no divider.

But you know best. :) Perhaps it wouldn't be confusing at all.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 741809 [details] [diff] [review]
Patch (No ::after)

Yeah, I don't think it's going to be confusing.

Comment 15

[Josiah Bruner [:jsbruner]]

Just in case people are wondering, I'm waiting for bug 856665 to be review+'d before I continue this...

Comment 16

[Josiah Bruner [:jsbruner]]

My academics have me swamped, so I won't be able to get this to this for awhile. Unassigning myself as I'm sure we want this landed within a couple weeks.

Comment 17

[Josiah Bruner [:jsbruner]]

Comment on attachment 735309 [details] [diff] [review]
Zoom separators

Not sure why I didn't obsolete this before.

Comment 18

[Josiah Bruner [:jsbruner]]

Comment on attachment 741809 [details] [diff] [review]
Patch (No ::after)

I'll keep this for reference.

Comment 20

[:Gijs (he/him)]

Upping prio per bug 909766.

(In reply to Josiah Bruner [:JosiahOne] from comment #20)
> Bug 747140 has the Windows implementation for doing this on the zoom
> buttons. This bug will use the new Australis style for the split control
> buttons. Right now I believe that is simply the zoom buttons and the
> bookmark button.

Per bug 909766, and also the edit-buttons.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch works, but there are two visible issues. The bookmark button has the separator too close to the dropdown icon, and the separators for the social API buttons are too high.

I believe both of these issues are unrelated to this bug though. The icon for the bookmarks bar needs revisiting (bug 897268) and the social buttons have a bug on file for them already (bug 879801).

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Hrm - I'm noticing the issues you mentioned, along with an issue with Firebug... have you tested the appearance of this with a standard XUL menu-button?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 796305 [details] [diff] [review]
Patch

I think I want to hear more about how standard menu-buttons look with this patch before I give an r+ or r-. If it's just a problem with Firebug because they're doing something funky, no problem - but if menu-buttons just look funky overall, we should fix that.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

This fixes the issue with the separator on the bookmarks and Firebug buttons. I chose 10px by trying different values and seeing what looks best. The 9px value that is used in the calc property was chosen because it is half of the 18px height of the separator.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 800877 [details] [diff] [review]
Patch v1.1

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

Yeah, this looks way better.

::: browser/themes/osx/browser.css
@@ +415,5 @@
> +  background-clip: padding-box;
> +  background-position: center;
> +  background-repeat: no-repeat;
> +  background-size: 1px 18px;
> +  box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);

According to Graphics, box shadows can be pretty expensive. We should keep a close eye on TART, ts_paint and tpaint when this patch lands.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/ec38188c7874

Comment 27

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ec38188c7874