870605 - Add separator between menu button and the other widgets in toolbar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Zhenshuo Fang (:fang) - Firefox UX Team]

Attachment: Mock-up

Add separator between menu button and the other widgets in toolbar

Comment 1

[Brandon Cheng]

I'll start working on this. Will it be safe to assume the menu button will always be to the far right?

Comment 2

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

Yes, it should always be the last element in the toolbar, however that knowledge may not be necessary to fix this bug.

You may just need to update http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#847 to include a reference to the #PanelUI-menu-button

Comment 3

[Brandon Cheng]

I went ahead and added #PanelUI-menu-button::before to that and ended up with the screenshot below.

http://i.imgur.com/mmtHmCP.png

It's not exactly how we want it. The separator we want stretches longer and is much more opaque. It also doesn't make a lot of sense to stick a #PanelUI-menu-button reference in the middle of social code.

I haven't seen a another long separator like that anywhere else in Firefox, so I'm assuming the styling code is not yet there in the code base. Unless there's something else I don't know, I'll go ahead and create a new styling for this based off the one you linked to. :)

Note: I do plan on using #PanelUI-menu-button::before psuedo element to do this. I don't think there should be an issue with this implementation.

Comment 4

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

(In reply to Brandon Cheng from comment #3)
> I went ahead and added #PanelUI-menu-button::before to that and ended up
> with the screenshot below.
> 
> http://i.imgur.com/mmtHmCP.png
> 
> It's not exactly how we want it. The separator we want stretches longer and
> is much more opaque. It also doesn't make a lot of sense to stick a
> #PanelUI-menu-button reference in the middle of social code.
> 
> I haven't seen a another long separator like that anywhere else in Firefox,
> so I'm assuming the styling code is not yet there in the code base. Unless
> there's something else I don't know, I'll go ahead and create a new styling
> for this based off the one you linked to. :)
> 
> Note: I do plan on using #PanelUI-menu-button::before psuedo element to do
> this. I don't think there should be an issue with this implementation.

Sounds good :)

Comment 5

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

Removing the items from M7 that do not block us from landing on m-c.

Comment 6

[Tim Nguyen :ntim]

The best way of doing this would be adding <seperator id="PanelUI-seperator"></seperator> before the Panel UI button.
Then apply this CSS :
>#PanelUI-seperator {
>    display: -moz-box;
>    background: linear-gradient(rgba(255,255,255,0), rgba(255,255,255,.3) 30%, rgba(255,255,255,.3) 70%, rgba(255,255,255,0)),
>                linear-gradient(rgba(23,51,78,0), rgba(23,51,78,.3) 30%, rgba(23,51,78,.3) 70%, rgba(23,51,78,0)),
>               linear-gradient(rgba(255,255,255,0), rgba(255,255,255,.3) 30%, rgba(255,255,255,.3) 70%, rgba(255,255,255,0));
>    width: 1px;
>    height: 32px;
>    margin: 0 3px;
>    border:none;
>  }

Comment 7

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

Actually, I think this separator should be part of the button, more like a border-start. When hovering this button, it will be odd to show a separator as well as the current surrounding borders on the toobarbutton-icon like we do today.

The button for the customization should have an always visible border-start, then when hovering on Windows and Linux, it should lighten up the full height of the toolbar and the width of the toolbar from the border-start to the border-end of the button (or the end of the toolbar if the button is at the end of the toolbar). When the button is pressed, this section of the toolbar should darken.

This matches closer to the two buttons at the bottom of the menu panel, except it also adds a separator that those two buttons don't have (and shouldn't).

Comment 8

[Tim Nguyen :ntim]

>#PanelUI-menu-button::before {
>    content: "";
>    display: -moz-box;
>    background: linear-gradient(rgba(255,255,255,0), rgba(255,255,255,.3) 30%, rgba(255,255,255,.3) 70%, rgba(255,255,255,0)),
>                linear-gradient(rgba(23,51,78,0), rgba(23,51,78,.3) 30%, rgba(23,51,78,.3) 70%, rgba(23,51,78,0)),
>               linear-gradient(rgba(255,255,255,0), rgba(255,255,255,.3) 30%, rgba(255,255,255,.3) 70%, rgba(255,255,255,0));
>    width: 1px;
>    height: 32px;
>    margin: 0 3px;
>  }
This is enough, but it should still stretch the to the whole navbar, border-start wouldn't be enough, since shorlander uses background-image to make the seperator. This code is also used by shorlander's interactive mocks.
I don't understand why it's still a small border start actually, it isn't like this in shorlander's mockups. Here's a version like you said :

#PanelUI-menu-button:before {
    content:" ";
    height:20px;
    display:block;
    background-image:linear-gradient(rgba(255, 255, 255, 0), rgba(255, 255, 255, 0.3) 30%, rgba(255, 255, 255, 0.3) 70%, rgba(255, 255, 255, 0)), linear-gradient(rgba(23, 51, 78, 0), rgba(23, 51, 78, 0.3) 30%, rgba(23, 51, 78, 0.3) 70%, rgba(23, 51, 78, 0)), linear-gradient(rgba(255, 255, 255, 0), rgba(255, 255, 255, 0.3) 30%, rgba(255, 255, 255, 0.3) 70%, rgba(255, 255, 255, 0)) !important;
    background-size: 1px auto, 1px auto, 1px auto;
    background-position: left top, center top, right top;
    background-repeat: no-repeat;
    -moz-box-flex: 0;
    width: 2px;
    margin-right:4px;
    background-color:transparent;
    border:none;
    transition:height 0.2s;
}
#PanelUI-menu-button:hover:before, #PanelUI-menu-button:-moz-any(:hover:active, [open],[checked]):not([disabled]):before {
    height:30px;
}
#PanelUI-menu-button .toolbarbutton-icon {border:none !important;background:none !important;box-shadow:none !important;}
#PanelUI-menu-button {padding:0 !important;padding-right:4px !important;}
#PanelUI-menu-button:-moz-any(:hover:active, [open],[checked]):not([disabled]) {
    background-color:rgba(0,0,0,0.1);
}

This is similar to your request, but you might need to needinfo shorlander about this.

I'll write a version with RTL support soon.

Comment 9

[Tim Nguyen :ntim]

I forgot to remove some !important from the code sorry :(

Comment 10

[Brandon Cheng]

I've had a version written that's been working pretty well, but it requires a set 40px height and slightly different padding settings across operating systems. The main issue is that the separator goes edge to edge on the navigation bar (when we have padding set to the nav-bar).

ntim007, your version doesn't seem to be behaving on OS X. The separator appears above the icon. I can't seem to get stretchable height (flexbox) working on the :before elements. Achieving that would be awesome.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Brandon Cheng from comment #10)
> I've had a version written that's been working pretty well, but it requires
> a set 40px height and slightly different padding settings across operating
> systems. The main issue is that the separator goes edge to edge on the
> navigation bar (when we have padding set to the nav-bar).
> 
> ntim007, your version doesn't seem to be behaving on OS X. The separator
> appears above the icon. I can't seem to get stretchable height (flexbox)
> working on the :before elements. Achieving that would be awesome.

I suspect setting -moz-box-align: stretch (and possibly -moz-box-orient: vertical) on the container (which is the menu button itself for ::before frames, AIUI) might be able to do that. Note that you may need to add specific styling for the icon to still remain correctly sized and centered despite the stretch setting... Finally, since you last looked at this, we've also moved some padding from the navbar to the customization target, which may help you here. Are you still interested in fixing this? :-)

Comment 12

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Brandon Cheng from comment #10)
> > I've had a version written that's been working pretty well, but it requires
> > a set 40px height and slightly different padding settings across operating
> > systems. The main issue is that the separator goes edge to edge on the
> > navigation bar (when we have padding set to the nav-bar).
> > 
> > ntim007, your version doesn't seem to be behaving on OS X. The separator
> > appears above the icon. I can't seem to get stretchable height (flexbox)
> > working on the :before elements. Achieving that would be awesome.
> 
> I suspect setting -moz-box-align: stretch (and possibly -moz-box-orient:
> vertical) on the container (which is the menu button itself for ::before
> frames, AIUI) might be able to do that. Note that you may need to add
> specific styling for the icon to still remain correctly sized and centered
> despite the stretch setting... Finally, since you last looked at this, we've
> also moved some padding from the navbar to the customization target, which
> may help you here. Are you still interested in fixing this? :-)

I am. I'm looking at fixing 373626 before this though (which I just made a comment on). Thanks for pointer with -moz-box-align: stretch. I'll explore that. :)

Comment 13

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

(In reply to Brandon Cheng from comment #12)
> I'm looking at fixing 373626 before this though (which I just made a
> comment on).

For posterity, I'm pretty sure you meant 873626 :)

Comment 14

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

Hey Brandon, now that bug 873626 is fixed, do you think you can pick this back up?

Comment 15

[Brandon Cheng]

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #14)
> Hey Brandon, now that bug 873626 is fixed, do you think you can pick this
> back up?

Absolutely. I was actually waiting on bug 893661 so we could have standard navbar height on all platforms before continuing this, but there's no reason I can't start working.

(Reasonable) Deadline for first patch revision: 12/22

Comment 16

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

Hey Brandon, were you still going to hack on this? We'd love to have this patch... :)

Comment 17

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #11)
> Finally, since you last looked at this, we've
> also moved some padding from the navbar to the customization target, which
> may help you here. Are you still interested in fixing this? :-)

This helps a lot. I was able to get a much simpler version just now with a few lines using background positioning (and no psuedo elements!). This works consistently on all 3 platforms. Will upload patch soon.

Comment 18

[Brandon Cheng]

Should patches still be made on the ux branch now that Australis has been merged into central?

Comment 19

[:Gijs (out for now; he/him)]

(In reply to Brandon Cheng from comment #18)
> Should patches still be made on the ux branch now that Australis has been
> merged into central?

Nope, against mozilla-central (or fx-team), please. :-)

Comment 20

[Brandon Cheng]

Attachment: Patch v1

This first version is just to track progress. I haven't added the hover effect Jared mentioned in comment 7 yet.

The background code was added to panelUIOverlay.inc.css, but let me know if it should be moved to browser.css since it's toolbar css.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Brandon Cheng from comment #20)
> Created attachment 8365794 [details] [diff] [review]
> Patch v1
> 
> This first version is just to track progress. I haven't added the hover
> effect Jared mentioned in comment 7 yet.
> 
> The background code was added to panelUIOverlay.inc.css, but let me know if
> it should be moved to browser.css since it's toolbar css.

Yeah, I guess browser.css is probably better...

Do you have an estimate for when you think this could be finished? We're trying very hard to ship in 29 (which is merged in a week from now) and we would really like this bug to make it.

Regarding the hover effect, the latest mockups I've seen ( http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html ) don't have the hover effect comment 7 describes, and I think we'd be happy to just have the separator land. :-)

Comment 22

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to Brandon Cheng from comment #20)
> > Created attachment 8365794 [details] [diff] [review]
> > Patch v1
> > 
> > This first version is just to track progress. I haven't added the hover
> > effect Jared mentioned in comment 7 yet.
> > 
> > The background code was added to panelUIOverlay.inc.css, but let me know if
> > it should be moved to browser.css since it's toolbar css.
> 
> Yeah, I guess browser.css is probably better...
> 
> Do you have an estimate for when you think this could be finished? We're
> trying very hard to ship in 29 (which is merged in a week from now) and we
> would really like this bug to make it.
> 
> Regarding the hover effect, the latest mockups I've seen (
> http://people.mozilla.org/~shorlander/mockups-interactive/australis-
> interactive-mockups/windows8.html ) don't have the hover effect comment 7
> describes, and I think we'd be happy to just have the separator land. :-)

If just having the separator in 29 is the goal, the patch at the moment would work.

Also, there doesn't seem to be a universal browser.css. I would have to insert the same code in 3 different files. Would that really be the preferred way to do it compared to having it in the PanelUI shared folder?

Comment 23

[:Gijs (out for now; he/him)]

Attachment: Add separator between menu button and the other widgets in toolbar (Australis)

The specifications seem to have changed, however... here's an updated patch, trying to match http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/osx.html and http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html . Mike, does this look OK to you? Brandon, I've left your name on the patch seeing as you did most of this - all I changed was the colors, and the background size (because without the transparency on either end I needed to do a bit of work to get this to not overlap the bottom of the toolbar). Stephen, can I just note that this looks bizarre if you enable the bookmarks toolbar, because there's no border between the two? What are we meant to do there?

Comment 24

[:Gijs (out for now; he/him)]

Attachment: Add separator between menu button and the other widgets in toolbar (Australis)

Of course, qref tends to help.

Comment 25

[Brandon Cheng]

(In reply to :Gijs Kruitbosch from comment #23)
> Created attachment 8369185 [details] [diff] [review]
> Add separator between menu button and the other widgets in toolbar
> (Australis)
> 
> The specifications seem to have changed, however... here's an updated patch,
> trying to match
> http://people.mozilla.org/~shorlander/mockups-interactive/australis-
> interactive-mockups/osx.html and
> http://people.mozilla.org/~shorlander/mockups-interactive/australis-
> interactive-mockups/windows8.html . Mike, does this look OK to you? Brandon,
> I've left your name on the patch seeing as you did most of this - all I
> changed was the colors, and the background size (because without the
> transparency on either end I needed to do a bit of work to get this to not
> overlap the bottom of the toolbar). Stephen, can I just note that this looks
> bizarre if you enable the bookmarks toolbar, because there's no border
> between the two? What are we meant to do there?

I personally think the most optimal solution here is to border the bookmarks toolbar and visually show it as something separate with a darker background-color. (I'm not a designer though, just sharing my thoughts.)

Comment 26

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

Comment on attachment 8369186 [details] [diff] [review]
Add separator between menu button and the other widgets in toolbar (Australis)

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

While the code looks fine, I think I want to hear from some UX people about what to do about the bookmarks toolbar, and to ensure that this patch is close enough to what they're envisioning.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +20,5 @@
> +  background-repeat: no-repeat;
> +}
> +
> +#PanelUI-menu-button {
> +    margin: 0 7px 0 9px;

Nit - 2 space indentation.

Comment 27

[:Gijs (out for now; he/him)]

Attachment: Add separator between menu button and the other widgets in toolbar (Australis)

Same spacing, back to the fade-out-y gradient because of the bookmarks toolbar per discussion with shorlander and madhava on IRC. Fixed the nit. Mike, does this look OK?

Comment 28

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

Comment on attachment 8370121 [details] [diff] [review]
Add separator between menu button and the other widgets in toolbar (Australis)

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

Looks good! Glad to finally get this. :)

Comment 29

[:Gijs (out for now; he/him)]

Comment on attachment 8370121 [details] [diff] [review]
Add separator between menu button and the other widgets in toolbar (Australis)

remote:   https://hg.mozilla.org/integration/fx-team/rev/1495ede52ef5

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Design incompleteness, plus it's not clear the menu panel button is separate to the rest of the toolbar, and people try to drag it (which instead kicks them out of customize mode)
Testing completed (on m-c, etc.): local testing, styling-only change.
Risk to taking this patch (and alternatives if risky): non-existent
String or IDL/UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1495ede52ef5

Comment 31

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ce2c56b2a522

Comment 32

[Tim Nguyen :ntim]

You shouldn't use the -moz- prefix for the gradients.

Comment 33

[:Gijs (out for now; he/him)]

(In reply to Tim Nguyen [:ntim] from comment #32)
> You shouldn't use the -moz- prefix for the gradients.

Quite. I updated this in my original patch and then forgot when we reverted to the original one.

remote:   https://hg.mozilla.org/integration/fx-team/rev/3521e02e5dc2

I'm not nominating this part for uplift as it'll work fine as-is.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3521e02e5dc2

Comment 35

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

(In reply to Tim Nguyen [:ntim] from comment #32)
> You shouldn't use the -moz- prefix for the gradients.

Ah, good call! I'd forgotten we'd unprefixed it!

Comment 36

[Ashickur Rahman]

I have successfully verified the fixed on Firefox 29 and 30 latest build.