Add separator between menu button and the other widgets in toolbar

VERIFIED FIXED in Firefox 29

Status

()

Firefox
Toolbars and Customization
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: fang, Assigned: Brandon Cheng)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

(Whiteboard: [Australis:P3] [bugday-20140212] )

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 747673 [details]
Mock-up

Add separator between menu button and the other widgets in toolbar

Updated

4 years ago
No longer blocks: 770135
Whiteboard: [Australis:M7]

Updated

4 years ago
Blocks: 872617
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

4 years ago
I'll start working on this. Will it be safe to assume the menu button will always be to the far right?
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
Assignee: nobody → bcheng.gt
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
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.
(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 :)
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P5]

Comment 6

4 years ago
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;
>  }
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

4 years ago
>#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

4 years ago
I forgot to remove some !important from the code sorry :(
(Assignee)

Comment 10

4 years ago
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

4 years ago
(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? :-)
Flags: needinfo?(bcheng.gt)

Updated

4 years ago
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P3]
(Assignee)

Comment 12

4 years ago
(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. :)
Flags: needinfo?(bcheng.gt)
(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 :)
Hey Brandon, now that bug 873626 is fixed, do you think you can pick this back up?
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 15

4 years ago
(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
Flags: needinfo?(bcheng.gt)
Whiteboard: [Australis:M?][Australis:P3] → [Australis:P4]
Whiteboard: [Australis:P4] → [Australis:P2]
Hey Brandon, were you still going to hack on this? We'd love to have this patch... :)

Updated

4 years ago
Flags: needinfo?(bcheng.gt)

Updated

4 years ago
Blocks: 950073
Whiteboard: [Australis:P2] → [Australis:P2] [feature] p=0
(Assignee)

Comment 17

4 years ago
(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.
Flags: needinfo?(bcheng.gt)
(Assignee)

Comment 18

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

Comment 19

4 years ago
(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. :-)
(Assignee)

Comment 20

3 years ago
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.

Comment 21

3 years ago
(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. :-)
(Assignee)

Comment 22

3 years ago
(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?

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 23

3 years ago
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?
Attachment #8369185 - Flags: review?(mconley)

Comment 24

3 years ago
Created attachment 8369186 [details] [diff] [review]
Add separator between menu button and the other widgets in toolbar (Australis)

Of course, qref tends to help.
Attachment #8369186 - Flags: review?(mconley)

Updated

3 years ago
Attachment #8369185 - Attachment is obsolete: true
Attachment #8369185 - Flags: review?(mconley)

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 25

3 years ago
(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 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.
Attachment #8369186 - Flags: ui-review?(shorlander)
Attachment #8369186 - Flags: review?(mconley)
Attachment #8369186 - Flags: feedback+

Updated

3 years ago
No longer blocks: 950073
Whiteboard: [Australis:P2] [feature] p=0 → [Australis:P2]

Comment 27

3 years ago
Created attachment 8370121 [details] [diff] [review]
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?
Attachment #8370121 - Flags: review?(mconley)

Updated

3 years ago
Assignee: bcheng.gt → gijskruitbosch+bugs

Updated

3 years ago
Assignee: gijskruitbosch+bugs → bcheng.gt

Updated

3 years ago
Attachment #8369186 - Attachment is obsolete: true
Attachment #8369186 - Flags: ui-review?(shorlander)
Whiteboard: [Australis:P2] → [Australis:P3]
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. :)
Attachment #8370121 - Flags: review?(mconley) → review+

Comment 29

3 years ago
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
Attachment #8370121 - Flags: checkin+
Attachment #8370121 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1495ede52ef5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Attachment #8370121 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 31

3 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ce2c56b2a522
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Comment 32

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

Comment 33

3 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/3521e02e5dc2
(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

3 years ago
I have successfully verified the fixed on Firefox 29 and 30 latest build.
status-firefox29: fixed → verified
status-firefox30: fixed → verified
Whiteboard: [Australis:P3] → [Australis:P3] [bugday-20140212]
Depends on: 979035

Updated

3 years ago
Depends on: 979300

Updated

3 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.