Open Bug 949151 Opened 11 years ago Updated 2 years ago

The separators for wide widgets don't match the spec (too tall and should disappear on mouse over)

Categories

(Firefox :: Theme, defect)

Product:
Firefox
Component:
Theme
Type:
defect
Points:
3

Tracking

()

People

(Reporter: jaws, Unassigned)

References

Details

(Whiteboard: [Australis:P4])

Attachments

(1 file, 5 obsolete files)

Patch v6: fix separator style for combined buttons on Linux and Windows
11.48 KB, patch
Gijs
: review-
Details | Diff | Splinter Review 
The separators for wide widgets in the toolbar are too tall and should disappear if an adjacent button is hovered.
Blocks: 942853
Oh, btw, you forgot to block the australis-merge and australis-cust bugs, and add [Australis:P3] in whiteboard. I've seen this on most Australis bugs.
(In reply to Tim Nguyen [:ntim] from comment #2)
> Oh, btw, you forgot to block the australis-merge and australis-cust bugs,
> and add [Australis:P3] in whiteboard. I've seen this on most Australis bugs.

Thanks, I forgot to put the priority on this bug. This shouldn't need to block australis-merge or australis-cust since it already blocks a tracked bug.
Whiteboard: [Australis:P4]
I think it has been resolved by bug 960517
(In reply to Guillaume C. [:ge3k0s] from comment #4)
> I think it has been resolved by bug 960517

Nope, it didn't fix it for me.
Gonna work on this after the findbar stuff.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — — Splinter Review 
Haven't tested the patch on linux and Windows 7. OS X seemed unaffected (by looking at the code).
Attachment #8405839 - Flags: review?(mdeboer)
Tim, sorry for the long delay... and in the meantime this patch has bitrotted significantly. :/

Could you post a new patch for me to review, please?
Comment on attachment 8405839 [details] [diff] [review]
Patch v1

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

::: browser/themes/linux/browser.css
@@ +672,5 @@
> +  display: none;
> +}
> +
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) {

I'm not sure how this will look in RTL mode... did you check that?

::: browser/themes/windows/browser.css
@@ +684,5 @@
>  }
>  
> +#nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> +#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-icon,
> +#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-text {

I have the feeling that the border-corrections you're doing here is already been done somewhere else. It is at least very similar to how the combined-buttons are styled in the menu-panel.

Also, how do we solve this in the OSX theme?

@@ +732,5 @@
>  #nav-bar .toolbarbutton-1:not([disabled=true]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container,
>  @conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> +#nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {

I *think* that that combined buttons inside a toolbar already have the `toolbarbutton-1` class-name, so the rules you added here shouldn't be necessary.

@@ +762,5 @@
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon:-moz-locale-dir(rtl),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-icon:-moz-locale-dir(ltr),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-icon:-moz-locale-dir(rtl),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-text:-moz-locale-dir(ltr),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-text:-moz-locale-dir(rtl) {

There's no need to add `.toolbaritem-combined-buttons > ` here, since it's already preceded by a descendant selector.

Also, please see my first comment above about doing border corrections here.

@@ +772,5 @@
> +  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon:-moz-locale-dir(ltr),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-icon:-moz-locale-dir(ltr),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-icon:-moz-locale-dir(rtl),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-text:-moz-locale-dir(ltr),
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-text:-moz-locale-dir(rtl) {

See previous comment.

@@ +784,5 @@
>    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-text,
>    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container,
> +  @conditionalForwardWithUrlbar@ > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {

Is this really the desired effect when the combined control is hovered? This will apply all sorts of effect to each individual button, while I think that the suggested effect is more subtle... or am I wrong?

@@ +797,5 @@
>    #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon,
>    #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> +  @conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:hover > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:hover > .toolbarbutton-text {

There's no need to add `.toolbaritem-combined-buttons > ` here, since it's already preceded by a descendant selector.

Also, please use `.toolbarbutton-combined:not([disabled]):not(:active):hover`.

Additionally, why are these rules needed? Are the regular styles inherited from `toolbarbutton-1` not sufficient?
Attachment #8405839 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> Comment on attachment 8405839 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8405839 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/browser.css
> @@ +672,5 @@
> > +  display: none;
> > +}
> > +
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) {
> 
> I'm not sure how this will look in RTL mode... did you check that?
Yes, I did, since it uses border-start, there is no problem.

> ::: browser/themes/windows/browser.css
> @@ +684,5 @@
> >  }
> >  
> > +#nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> > +#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-icon,
> > +#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-text {
> 
> I have the feeling that the border-corrections you're doing here is already
> been done somewhere else. It is at least very similar to how the
> combined-buttons are styled in the menu-panel.
Border corrections didn't seem done. 

> Also, how do we solve this in the OSX theme?
I don't know, but I saw that the separator style was fixed. Can you post a screenshot of how combined buttons look on hover please ?

> @@ +732,5 @@
> >  #nav-bar .toolbarbutton-1:not([disabled=true]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container,
> >  @conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> > +#nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> > +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> > +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {
Whoops, yes you are right.
 
> @@ +762,5 @@
> > +  #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon:-moz-locale-dir(rtl),
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-icon:-moz-locale-dir(ltr),
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-icon:-moz-locale-dir(rtl),
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:last-child) > .toolbarbutton-text:-moz-locale-dir(ltr),
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:not(:first-child) > .toolbarbutton-text:-moz-locale-dir(rtl) {
> 
> There's no need to add `.toolbaritem-combined-buttons > ` here, since it's
> already preceded by a descendant selector.
> 
> Also, please see my first comment above about doing border corrections here.
OK, I will check the menu panel code

> @@ +784,5 @@
> >    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-text,
> >    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container,
> > +  @conditionalForwardWithUrlbar@ > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon,
> > +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> > +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {
Yes, I think it's the desired effect, see the bookmarks menu button.

> @@ +797,5 @@
> >    #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon,
> >    #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> > +  @conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:hover > .toolbarbutton-icon,
> > +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined:hover > .toolbarbutton-text {
> 
> There's no need to add `.toolbaritem-combined-buttons > ` here, since it's
> already preceded by a descendant selector.
> 
> Also, please use
> `.toolbarbutton-combined:not([disabled]):not(:active):hover`.
> 
> Additionally, why are these rules needed? Are the regular styles inherited
> from `toolbarbutton-1` not sufficient?
You are right.
Bug 998687 will also be addressed in my next patch.
Just looked at the menu panel code for the separators. We handle things in a non future proof way.

We target the middle button then remove the left and right borders. 

I don't think this is a good way to do it. I'm just gonna stick with my current approach for now. Unless you have a better one.
(In reply to Tim Nguyen [:ntim] from comment #12)
> Just looked at the menu panel code for the separators. We handle things in a
> non future proof way.
> 
> We target the middle button then remove the left and right borders. 
*using ID's (#zoom-reset-button and #copy-button)
(In reply to Tim Nguyen [:ntim] from comment #13)
> (In reply to Tim Nguyen [:ntim] from comment #12)
> > Just looked at the menu panel code for the separators. We handle things in a
> > non future proof way.
> > 
> > We target the middle button then remove the left and right borders. 
> *using ID's (#zoom-reset-button and #copy-button)

I know and that's also not my ideal way of doing things... but that's the status quo unfortunately.
I'd like to deal with this in a follow-up, if you don't mind, so you should use the IDs here for consistency sake.

Don't get me wrong, I'd like to do it differently as much as you do, but I think that finishing and landing this bug is more important.
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> (In reply to Tim Nguyen [:ntim] from comment #13)
> > (In reply to Tim Nguyen [:ntim] from comment #12)
> > > Just looked at the menu panel code for the separators. We handle things in a
> > > non future proof way.
> > > 
> > > We target the middle button then remove the left and right borders. 
> > *using ID's (#zoom-reset-button and #copy-button)
> 
> I know and that's also not my ideal way of doing things... but that's the
> status quo unfortunately.
> I'd like to deal with this in a follow-up, if you don't mind, so you should
> use the IDs here for consistency sake.
> 
> Don't get me wrong, I'd like to do it differently as much as you do, but I
> think that finishing and landing this bug is more important.

Yes, but by using your solution, the border styling is very weird, especially when the button is followed by a disabled button.

Anyway, I found a workaround for now, we're not going to apply a global background when hovering the widget. That's how social API buttons worked some time ago.
I found an even better solution. 
#nav-bar .toolbarbutton-combined > .toolbarbutton-icon,
#nav-bar .toolbarbutton-combined > .toolbarbutton-text {
-moz-margin-start: -1px !important;
}
I noticed that in shorlander's mockup for Windows 8, the "global" background for menu buttons is different from the button hover state inside the menu buttons.

Should I address this here ? I'm actually applying this styling on the combined buttons, so it'd be nice to address this here for consistency.
Flags: needinfo?(mdeboer)
Attached patch Patch v2 (obsolete) — — Splinter Review 
Applies on top of bug 998687
Attachment #8405839 - Attachment is obsolete: true
Attachment #8420654 - Flags: review?(mdeboer)
Mike, do you think you can do a quick review of this soon ? Thanks :)
Tim! Apologies for the long delay :/ I promise shorter review cycles from here on out.

I'll review your patch today.
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #20)
> Tim! Apologies for the long delay :/ I promise shorter review cycles from
> here on out.
> 
> I'll review your patch today.
Should I do comment 17 ?
(In reply to Tim Nguyen [:ntim] from comment #21)
> Should I do comment 17 ?

Well, yeah. This is, again, basically the same as on OSX at the moment. I'd still like you to follow that implementation (CSS-wise) as closely as possible.
Comment on attachment 8420654 [details] [diff] [review]
Patch v2

I'll wait for a fresh new patch, with moar UI goodness!
Attachment #8420654 - Flags: review?(mdeboer)
Attached patch Patch v3 (obsolete) — — Splinter Review 
Fixed some Windows issues.
Attachment #8420654 - Attachment is obsolete: true
Attachment #8434175 - Flags: review?(mdeboer)
Mike, do you think you can give a first pass review if anything is missing ? I'd like this to land before the Aurora merge. Thanks :)
Flags: needinfo?(mdeboer)
Tim, at first glance this looks good, I'll do a functional test on Win & Lin right now.
Flags: needinfo?(mdeboer)
Comment on attachment 8434175 [details] [diff] [review]
Patch v3

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

Tim, there's one blocker thing I noticed: When you hover a combined item (zoom/ edit controls) I don't see the border appear around the entire control.

I think I mentioned this before: can you please check out how the styling is done on OSX and just copy that as much as possible?

Thanks!
Attachment #8434175 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #28)
> Comment on attachment 8434175 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8434175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tim, there's one blocker thing I noticed: When you hover a combined item
> (zoom/ edit controls) I don't see the border appear around the entire
> control.
Can you post a screencast please ? I don't see the issue happening on Windows 8.

> I think I mentioned this before: can you please check out how the styling is
> done on OSX and just copy that as much as possible?
> 
> Thanks!
I've checked the OSX version, and it causes issues on Windows, the border can sometimes be too light (next to a disabled button for example), also the way I do it also works for add-ons (your soundcloud player and the open with addon).
Flags: needinfo?(mdeboer)
(In reply to Tim Nguyen [:ntim] from comment #29)
> > Tim, there's one blocker thing I noticed: When you hover a combined item
> > (zoom/ edit controls) I don't see the border appear around the entire
> > control.
> Can you post a screencast please ? I don't see the issue happening on
> Windows 8.

Ah, I saw this right away on Linux, which was the first platform I checked. Quite possible this is happening on Win correctly. I'm checking that now.
Flags: needinfo?(mdeboer)
r=me for the Windows bits, so the only thing left is Linux.
Attached patch Patch v4 (obsolete) — — Splinter Review 

        Attachment #8434175 -
        Attachment is obsolete: true

        Attachment #8437844 -
        Flags: review?(mdeboer)

    
    

    
  
Comment on attachment 8437844 [details] [diff] [review]
Patch v4

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

f+ again, please read my comment below:

::: browser/themes/linux/browser.css
@@ -576,5 @@
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-badge-container > .toolbarbutton-icon,
> -:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-icon {

I'm afraid that applying the hover style on all buttons doesn't yield the desired effect.

The thing that needs to happen is _exactly_ the same as you did on Windows. The toolbarbutton styling on the nav-bar is almost identical to the Windows Aero styles in the Windows theme.

        Attachment #8437844 -
        Flags: review?(mdeboer) → feedback+

    
    

    
  
As discussed with Tim on IRC, I'll be finishing the patch in this bug, because he doesn't have a Linux environment at the ready.
Assignee: ntim007 → mdeboer

    
    

    
  
Marco, can you add this to the current iteration?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Whiteboard: [Australis:P4] → [Australis:P4] p=3

    
  

        Attachment #8437844 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8439067 [details] [diff] [review]
Patch v5: fix separator style for combined buttons on Linux and Windows

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

::: browser/themes/linux/browser.css
@@ +541,5 @@
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-icon {
>    -moz-margin-end: 0;
>    padding: 2px 6px;
> +  border: 1px solid;
> +  border-color: transparent;

No need to split this up, right?

@@ +586,5 @@
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-badge-container > .toolbarbutton-icon,
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not([disabled=true]):hover > .toolbarbutton-icon {
>    background-color: hsla(0,0%,100%,.3);
>    background-image: linear-gradient(hsla(0,0%,100%,.7), hsla(0,0%,100%,.2));
> +  border-color: rgb(154,154,154);

Or to touch all these irrelevant lines. Please split this up into a separate patch if you think we really need to address it. And if possible, the same should be done with the background changes - it's making this patch very hard to read, and it's not actually about this bug, is it?

@@ +622,5 @@
>    padding: 3px;
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > separator {

I'm fairly sure content: isn't a valid property for non-pseudoelements. What are you trying to do here?

@@ +636,5 @@
>    background-size: 1px 18px;
>    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons:hover > separator {

... especially when you display: none the same selector again here?

@@ +647,5 @@
>    margin-top: 3px;
>    margin-bottom: 3px;
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-combined:not(:last-child) {

Not a fan of this selector (nor the first-child one above this one). Instead, why don't you make the separators be visibility:hidden and give them a negative margin to pull stuff together if necessary?

Also, why do we need 3px on Linux but 1px on Windows? That's just weird, aren't the separators meant to be the same?

::: browser/themes/windows/browser.css
@@ +713,5 @@
>    padding-right: 0;
>  }
>  
>  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +#nav-bar .toolbaritem-combined-buttons > separator {

Again, this is strange, as you're display: none'ing the item right after.

@@ +762,5 @@
>         (-moz-os-version: windows-win7) {
>  %endif
>    /* < Win8 */
>    #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +  toolbar .toolbaritem-combined-buttons > separator {

All these rules are for #nav-bar, why is this one for |toolbar|, and what is it doing? Doesn't it just get overridden by the earlier display: none rule as well?

@@ +788,5 @@
>      border-top-left-radius: 0;
>      border-bottom-left-radius: 0;
>    }
>  
> +  #nav-bar .toolbarbutton-combined:not(:first-child):not(:last-child) {

Do we actually need to do this? I didn't think this was broken before...

@@ +823,5 @@
>  %ifdef WINDOWS_AERO
>  }
>  %endif
>  
> +#nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button:not([disabled=true]):-moz-any(:hover:active, [open]) > .toolbarbutton-icon,

I'm fairly sure the menubutton will inherit the disabled state from the container, so presumably you can just have the :not([disabled=true]) bit on the descendant here, like the rule below this one?

        Attachment #8439067 -
        Flags: review?(gijskruitbosch+bugs) → review-

    
    

    
  
Gijs, excellent first pass review. Thanks!

(In reply to :Gijs Kruitbosch from comment #38)
> @@ +622,5 @@
> >    padding: 3px;
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > separator {
> 
> I'm fairly sure content: isn't a valid property for non-pseudoelements. What
> are you trying to do here?
> 
> @@ +636,5 @@
> >    background-size: 1px 18px;
> >    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons:hover > separator {
> 
> ... especially when you display: none the same selector again here?

I'm trying to change the appearance of the separators on the nav-bar. On hover, the separator will disappear.

I can move the pseudo-element specific styles out and keep only the styles relevant to the separator and pseudo-element styles together. Is that preferable (I think so, just verifying.)
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #38)
> Comment on attachment 8439067 [details] [diff] [review]
> Patch v5: fix separator style for combined buttons on Linux and Windows
> 
> Review of attachment 8439067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/linux/browser.css
> @@ +541,5 @@
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-icon {
> >    -moz-margin-end: 0;
> >    padding: 2px 6px;
> > +  border: 1px solid;
> > +  border-color: transparent;
> 
> No need to split this up, right?
It's better for other future border corrections.

> @@ +622,5 @@
> >    padding: 3px;
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > separator {
> 
> I'm fairly sure content: isn't a valid property for non-pseudoelements. What
> are you trying to do here?
It's for the first selector that uses a ::before pseudo element.

> @@ +636,5 @@
> >    background-size: 1px 18px;
> >    box-shadow: 0 0 0 1px hsla(0,0%,100%,.2);
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons:hover > separator {
> 
> ... especially when you display: none the same selector again here?
There's a clear difference. This one is hover. The other is the normal state.

> @@ +647,5 @@
> >    margin-top: 3px;
> >    margin-bottom: 3px;
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-combined:not(:last-child) {
> 
> Not a fan of this selector (nor the first-child one above this one).
> Instead, why don't you make the separators be visibility:hidden and give
> them a negative margin to pull stuff together if necessary?
> 
> Also, why do we need 3px on Linux but 1px on Windows? That's just weird,
> aren't the separators meant to be the same?
That's the weirdness of Linux's code. I think we should move the CSS into a shared directory to avoid weird inconsistencies like this.

> ::: browser/themes/windows/browser.css
> @@ +713,5 @@
> >    padding-right: 0;
> >  }
> >  
> >  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +#nav-bar .toolbaritem-combined-buttons > separator {
> 
> Again, this is strange, as you're display: none'ing the item right after.
Yes, on hover.

> @@ +762,5 @@
> >         (-moz-os-version: windows-win7) {
> >  %endif
> >    /* < Win8 */
> >    #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > +  toolbar .toolbaritem-combined-buttons > separator {
> 
> All these rules are for #nav-bar, why is this one for |toolbar|, and what is
> it doing? Doesn't it just get overridden by the earlier display: none rule
> as well?
So other toolbars don't have the faint separator we use in menu panel.

> @@ +788,5 @@
> >      border-top-left-radius: 0;
> >      border-bottom-left-radius: 0;
> >    }
> >  
> > +  #nav-bar .toolbarbutton-combined:not(:first-child):not(:last-child) {
> 
> Do we actually need to do this? I didn't think this was broken before...
Yes, since we now stick buttons together. We have to remove the border radius from the middle buttons.

> @@ +823,5 @@
> >  %ifdef WINDOWS_AERO
> >  }
> >  %endif
> >  
> > +#nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button:not([disabled=true]):-moz-any(:hover:active, [open]) > .toolbarbutton-icon,
> 
> I'm fairly sure the menubutton will inherit the disabled state from the
> container, so presumably you can just have the :not([disabled=true]) bit on
> the descendant here, like the rule below this one?
Yeah, you're right.

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #39)
> I can move the pseudo-element specific styles out and keep only the styles
> relevant to the separator and pseudo-element styles together. Is that
> preferable (I think so, just verifying.)

Sounds like a plan!

    
    

    
  
(In reply to Tim Nguyen [:ntim] from comment #40)
> > >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-icon {
> > >    -moz-margin-end: 0;
> > >    padding: 2px 6px;
> > > +  border: 1px solid;
> > > +  border-color: transparent;
> > 
> > No need to split this up, right?
> It's better for other future border corrections.

I'm not sure what you mean by future corrections, but no, replacing border: 1px solid transparent; with border: 1px solid; border-color: transparent; is not useful.

    
    

    
  
(In reply to Tim Nguyen [:ntim] from comment #40)
> (In reply to :Gijs Kruitbosch from comment #38)
> > Comment on attachment 8439067 [details] [diff] [review]
> > @@ +622,5 @@
> > >    padding: 3px;
> > >  }
> > >  
> > > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> > > +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons > separator {
> > 
> > I'm fairly sure content: isn't a valid property for non-pseudoelements. What
> > are you trying to do here?
> It's for the first selector that uses a ::before pseudo element.

I understand that, but they shouldn't share a rule with the other selector where they're not relevant / doing anything. :-)
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
      
  

    
    

    
  
Whoops, accidently reposted the older comment

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #42)
> (In reply to Tim Nguyen [:ntim] from comment #40)
> > > >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-icon {
> > > >    -moz-margin-end: 0;
> > > >    padding: 2px 6px;
> > > > +  border: 1px solid;
> > > > +  border-color: transparent;
> > > 
> > > No need to split this up, right?
> > It's better for other future border corrections.
> 
> I'm not sure what you mean by future corrections, but no, replacing border:
> 1px solid transparent; with border: 1px solid; border-color: transparent; is
> not useful.

We just need to override the border-color and the background on hover and active states. And if we override the border-width as well, it can affect some future cases (removing a border for a specific button for example).

    
    

    
  
(In reply to Tim Nguyen [:ntim] from comment #46)
> (In reply to Dão Gottwald [:dao] from comment #42)
> > (In reply to Tim Nguyen [:ntim] from comment #40)
> > > > >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-icon {
> > > > >    -moz-margin-end: 0;
> > > > >    padding: 2px 6px;
> > > > > +  border: 1px solid;
> > > > > +  border-color: transparent;
> > > > 
> > > > No need to split this up, right?
> > > It's better for other future border corrections.
> > 
> > I'm not sure what you mean by future corrections, but no, replacing border:
> > 1px solid transparent; with border: 1px solid; border-color: transparent; is
> > not useful.
> 
> We just need to override the border-color and the background on hover and
> active states.

There's no point in avoiding the border shorthand in the above rule for that.

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #38)
> Not a fan of this selector (nor the first-child one above this one).
> Instead, why don't you make the separators be visibility:hidden and give
> them a negative margin to pull stuff together if necessary?

While working on this, I noticed that the block above is already applied for menu-buttons. I suspect we'd want to maintain these styles together with the combined buttons here.

IOW, I'd like to keep it as stated in the patch, because I think your trick on the separators will not work on the menu-buttons, using pseudo-elements.

> Also, why do we need 3px on Linux but 1px on Windows? That's just weird,
> aren't the separators meant to be the same?

That's because of the different margins between buttons used on Linux. I believe this is a remnant of the native/ plastic GTK look. Some day, we may want to fix this, but I think doing it for this bug is creeping the scope a bit.

    
    

    
  
Comment on attachment 8439067 [details] [diff] [review]
Patch v5: fix separator style for combined buttons on Linux and Windows

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

::: browser/themes/windows/browser.css
@@ +735,5 @@
>    border-color: hsla(210,4%,10%,.1);
>  }
>  
>  #nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
> +#nav-bar .toolbarbutton-1:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon,

While reworking the Windows styles, I stumbled upon this and didn't understand its necessity.

What's the reason you needed to add this here? I'm not talking about the additional styles for the combined buttons' hover state, but about the others.
Flags: needinfo?(ntim007)

    
    

    
  
Added to Iteration 33.1

Mike, would this bug be [qa+] or [qa-] for QA verification?
Flags: needinfo?(mmucci)
Whiteboard: [Australis:P4] p=3 → [Australis:P4] p=3 s=33.1 [qa?]

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #49)
> Comment on attachment 8439067 [details] [diff] [review]
> Patch v5: fix separator style for combined buttons on Linux and Windows
> 
> Review of attachment 8439067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/windows/browser.css
> @@ +735,5 @@
> >    border-color: hsla(210,4%,10%,.1);
> >  }
> >  
> >  #nav-bar .toolbarbutton-1:not([disabled=true]) > .toolbarbutton-menubutton-button[open] + .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
> > +#nav-bar .toolbarbutton-1:not([disabled]):-moz-any(:hover,[open]) > .toolbarbutton-menubutton-button > .toolbarbutton-icon,
> 
> While reworking the Windows styles, I stumbled upon this and didn't
> understand its necessity.
> 
> What's the reason you needed to add this here? I'm not talking about the
> additional styles for the combined buttons' hover state, but about the
> others.

Well, I made the menu-buttons match this mockup (with 2 hover states) : https://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html
And I want to make things consistent, across both the mockup and the combined buttons.
Flags: needinfo?(ntim007)

    
    

    
  
Hi Mike, should this bug be marked as [qa+] or [qa-] for verification?
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:P4] p=3 s=33.1 [qa?] → [Australis:P4] p=3 s=33.1 [qa+]

        Attachment #8439067 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8443449 [details] [diff] [review]
Patch v6: fix separator style for combined buttons on Linux and Windows

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

r- primarily for Linux woes, but there's some issues on Windows as well that I'd love to see fixed if possible (just less sure that they are indeed easily fixable)

::: browser/themes/linux/browser.css
@@ +574,5 @@
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon {
> +  background-color: hsla(210,4%,10%,.02);
> +  border: 1px solid hsla(210,4%,10%,.1);

This should go after the more general rules for toolbarbutton icon backgrounds below.

You've also missed out toolbarbutton-text, as compared to Windows. This means the zoom buttons look wonky.

@@ +642,5 @@
> +  display: none;
> +}
> +
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker,
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-combined:not(:first-child) {

Why does this not need margin-top/bottom if it's the first button? That seems wrong.

@@ +649,5 @@
>    margin-bottom: 3px;
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-combined:not(:last-child) {
> +  -moz-margin-end: -3px;

This still doesn't make sense to me. Tim's earlier explanation was just "Linux is weird". That may be so, but the result here doesn't work out:

http://imgur.com/AR7hFiI,ten78BH

Note the distance between the lines and the icons varies between the hover and non-hover state.

::: browser/themes/windows/browser.css
@@ +741,5 @@
> +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> +#nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {
> +  background-color: hsla(210,4%,10%,.02);
> +  border-color: hsla(210,4%,10%,.1);
> +}

Like on Linux, I would prefer this to be after the much more general button styling below.

@@ +763,5 @@
>         (-moz-os-version: windows-win7) {
>  %endif
>    /* < Win8 */
>    #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
> +  toolbar .toolbaritem-combined-buttons > separator {

I'm still unconvinced that the right thing to do here is to change this selector to |toolbar| but leave the menubutton one, and also, to change this only on pre-win-8.

Also, a lot of these properties (in this rule) are duplicated from above... which means these values, inasmuch as they're different, get overridden because the selector above is still more specific (#nav-bar .toolbarbutton-combined-buttons > separator).

I'm also confused as to why the size is apparently meant to be different on win8 (16px) as compared to pre-win-8 (18px) - a change which won't take effect for the separators here because this selector is less specific...

@@ +800,5 @@
>    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-text,
>    #nav-bar .toolbarbutton-1:not([disabled]):not([checked]):not([open]):not(:active):hover > .toolbarbutton-badge-container,
> +  @conditionalForwardWithUrlbar@ > .toolbarbutton-1:-moz-any([disabled],:not([open]):not([disabled]):not(:active)) > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons:hover > .toolbarbutton-combined > .toolbarbutton-text {

This adds a box shadow which isn't there on Linux or win8... is Linux meant to follow win 8 styling rather than win7/vista/xp ?

I'm also just sad. I just checked in DOMI, and a hovered button inside these items on win7 now has 8 (!) rules applied to it wrt background-color/border-color alone, each overriding another. A non-hovered button inside the hovered item has 4. That seems excessive. Is there no simpler way to achieve this?

Also note that the only spec referenced here is for win8. On win8, the hovered state of the hovered button matches that of the hovered state of any other button, and the unhovered buttons inside the hovered item get a style somewhere inbetween non-hovered and hovered.

On win7, the patch makes the unhovered buttons inside the item look like the other hovered buttons (!?) and does something wholly different for the actually hovered item. On the whole, I find that pretty confusing.

@@ +812,5 @@
>  
>    #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-button:not([disabled]):not([open]):not(:active):hover > .toolbarbutton-icon,
>    #nav-bar .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> +  @conditionalForwardWithUrlbar@ > #forward-button:not([open]):not(:active):not([disabled]):hover > .toolbarbutton-icon,
> +  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-combined.toolbarbutton-1:hover:not(:active):not([disabled="true"]) > .toolbarbutton-icon,

Nit: please use consistent selectors. :hover at the end in all the other ones, so please do the same in the ones you're adding here.

        Attachment #8443449 -
        Flags: review?(gijskruitbosch+bugs) → review-
Iteration: --- → 33.2
Points: --- → 3
QA Whiteboard: [qa+]
Whiteboard: [Australis:P4] p=3 s=33.1 [qa+] → [Australis:P4]
QA Contact: cornel.ionce
Iteration: 33.2 → 33.3
Iteration: 33.3 → 34.1

    
    

    
  
Removed from Iteration 34.1 based on review by GMC.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Iteration: 34.1 → ---

    
  
Summary: The separators for wide widgets doesn't match the spec (too tall and should disappear on mouse over) → The separators for wide widgets don't match the spec (too tall and should disappear on mouse over)

    
  
Blocks: theme-win10

    
    

    
  
Not a Windows-10-specific issue.
No longer blocks: theme-win10

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: