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

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jsbruner, Assigned: jaws)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 28
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M9][Australis:P2], URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
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.
Depends on: 748894
Created attachment 735309 [details] [diff] [review]
Zoom separators

Patch that adds separators to the zoom buttons. Not going to request reviews until dependent bugs are completed.
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 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.
Attachment #735309 - Flags: ui-review?(shorlander)
Attachment #735309 - Flags: review?(dao)
Comment on attachment 735309 [details] [diff] [review]
Zoom separators

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

Visually this looks good :)
Attachment #735309 - Flags: ui-review?(shorlander) → ui-review+
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.
Attachment #735309 - Flags: review?(dao) → review-
Created attachment 741809 [details] [diff] [review]
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).
Attachment #741809 - Flags: ui-review+
Attachment #741809 - Flags: review?(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?
(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.
(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.
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.
(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.
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 on attachment 741809 [details] [diff] [review]
Patch (No ::after)

Yeah, I don't think it's going to be confusing.
Attachment #741809 - Flags: review?(dao) → review-
Just in case people are wondering, I'm waiting for bug 856665 to be review+'d before I continue this...

Updated

5 years ago
Whiteboard: [Australis:M?]
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.
Assignee: josiah → nobody
Comment on attachment 735309 [details] [diff] [review]
Zoom separators

Not sure why I didn't obsolete this before.
Attachment #735309 - Attachment is obsolete: true
Comment on attachment 741809 [details] [diff] [review]
Patch (No ::after)

I'll keep this for reference.
Attachment #741809 - Attachment description: Zoom separators V2 → Patch (No ::after)
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]

Updated

5 years ago
Duplicate of this bug: 909766

Comment 20

5 years ago
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.
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M?][Australis:P2]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Created attachment 796305 [details] [diff] [review]
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).
Attachment #741809 - Attachment is obsolete: true
Attachment #796305 - Flags: review?(dao)
Attachment #796305 - Flags: review?(dao) → review?(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 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.
Attachment #796305 - Flags: review?(mconley)
Created attachment 800877 [details] [diff] [review]
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.
Attachment #796305 - Attachment is obsolete: true
Attachment #800877 - Flags: review?(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.
Attachment #800877 - Flags: review?(mconley) → review+
https://hg.mozilla.org/projects/ux/rev/ec38188c7874
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]

Updated

5 years ago
Depends on: 924202

Comment 27

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ec38188c7874
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.