875479 - Australis toolbar buttons for Linux

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Marco Castelluccio [:marco]]

Equivalent of bug 856665 and bug 734373 for Linux.
With the GTK icons the Australis theme looks awkward. Probably we need this also for bug 873626, because the GTK icons are also too big.

Comment 1

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

Bug 874674 should be enough for M9. We can try to take this bug later, but shouldn't block landing on it.

Comment 3

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

Attachment: WIP, start of the curved back button

This is just a WIP of something I put together a couple weeks ago. It implements the round back button, but still needs more work. Also, the conditional forward button needs to be reworked to function more like its Windows counterpart.

This patch doesn't touch the other toolbar buttons, but the nice thing about working on this bug is that much of the styles from Windows can be reused on Linux.

Comment 5

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

Comment on attachment 826013 [details] [diff] [review]
WIP, start of the curved back button

Moved to bug 477948 since that bug is specific to the keyhole back button.

Comment 6

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

Attachment: Patch

Comment 7

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

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

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

Generally this looks fine from a code perspective. We should probably talk about the issues briefly.

::: browser/themes/linux/browser.css
@@ +499,5 @@
> +  padding: 3px 7px;
> +  border: 1px solid transparent;
> +  border-radius: 2px;
> +  transition-property: background-color, border-color;
> +  transition-duration: 250ms;

Have you tested this with an add-on provided menubutton that still has a 'regular' dropmarker rather than a full-on icon, like one from the web developer toolbar?

@@ +505,5 @@
>  
> +.toolbarbutton-1 > .toolbarbutton-menubutton-button:hover > .toolbarbutton-icon,
> +.toolbarbutton-1:not([buttonover]):not([open]):hover > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
> +.toolbarbutton-1:hover > .toolbarbutton-icon {
> +  background-color: hsla(0,0%,100%,.3);

Isn't this just a shade of grey? Any particular reason not to use rgba here?

@@ +507,5 @@
> +.toolbarbutton-1:not([buttonover]):not([open]):hover > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon,
> +.toolbarbutton-1: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: 1px solid ThreeDShadow;

I'm worried that we don't control this shade in GTK themes, but the other ones are hardcoded. Seeing as we're going the "non-integration-with-icons-and-stuff" route, maybe this should be hardcoded, too.

@@ +517,5 @@
> +.toolbarbutton-1 > .toolbarbutton-menubutton-button:hover:active > .toolbarbutton-icon,
> +.toolbarbutton-1[open="true"] > .toolbarbutton-menubutton-dropmarker:not([disabled]) > .dropmarker-icon,
> +.toolbarbutton-1:not([disabled]):hover:active > .toolbarbutton-icon,
> +.toolbarbutton-1[open="true"] > .toolbarbutton-icon {
> +  background-color: ButtonShadow;

Same issues here as in the previous comment.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #7)
> > +  background-image: linear-gradient(hsla(0,0%,100%,.7), hsla(0,0%,100%,.2));
> > +  border: 1px solid ThreeDShadow;
> 
> I'm worried that we don't control this shade in GTK themes, but the other
> ones are hardcoded. Seeing as we're going the
> "non-integration-with-icons-and-stuff" route, maybe this should be
> hardcoded, too.

We can't just chose not to integrate at all, since the toolbar color comes from the GTK theme and the buttons need to look somewhat reasonable on that. Regardless of whether you end up using a mix of platform colors and hardcoded ones or only hardcoded ones, this whole stuff needs to be tested with a few GTK themes, at least with a light one, a dark one and something in between.

Comment 9

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

Attachment: Patch v1.1

I've addressed the previous feedback and tested with a light theme (Radiance), dark theme (Ambiance), as well as High Contrast themes. The buttons when placed in the TabsToolbar don't have a ton of contrast on a dark theme, but this can be fixed by adjusting the colors in the final icons for Linux.

Comment 10

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

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

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

This is alright, but why just the tabs toolbar and navbar? What about the menubar? Bookmarks toolbar? Are we keeping the same styling there for now? The -moz-any makes me nervous...

::: browser/themes/linux/browser.css
@@ +541,4 @@
>    padding: 5px !important;
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before {

Isn't this part of another bug? Or did we just forget to do separators on Linux?

Comment 11

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

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8361619 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8361619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is alright, but why just the tabs toolbar and navbar? What about the
> menubar? Bookmarks toolbar? Are we keeping the same styling there for now?
> The -moz-any makes me nervous...

We don't do the Australis toolbar button styling for buttons within the bookmarks menubar or the application menubar on Windows.

-moz-any() is only bad for performance when it is the right-most selector.

> ::: browser/themes/linux/browser.css
> @@ +541,4 @@
> >    padding: 5px !important;
> >  }
> >  
> > +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before {
> 
> Isn't this part of another bug? Or did we just forget to do separators on
> Linux?

This was never done for Linux and it was easy to add while making these other changes.

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/ddaf0b95b199

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Dão Gottwald [:dao]]

Why did this take a different route than Windows, where the clickable area is as large as the toolbar rather than being limited by the button border (which is invisible until you're over it)? Since this is a usability regression that we were careful enough not to cause in bug 734373, I think this warrants a backout.

Comment 15

[Dão Gottwald [:dao]]

Um, it looks like you did actually put the border around the icon, like we did on Windows. Unless there is some margin still making the buttons smaller, I guess the inability to predictably hit the buttons at the edges that I saw on Linux was due to the mouse pointer being rendered larger than it actually is. I seem to remember that Ubuntu does something like this, presumably to /improve/ usability, but how exactly this is supposed to work is beyond me.