Closed Bug 875479 Opened 6 years ago Closed 6 years ago

Australis toolbar buttons for Linux

Categories

(Firefox :: Theme, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: marco, Assigned: jaws)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [Australis:P3] )

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
Blocks: 892959
Blocks: 899033
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M9][Australis:P3]
Bug 874674 should be enough for M9. We can try to take this bug later, but shouldn't block landing on it.
Whiteboard: [Australis:M9][Australis:P3] → [Australis:M?][Australis:P3]
Duplicate of this bug: 933162
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.
Duplicate of this bug: 940322
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.
Attachment #826013 - Attachment is obsolete: true
Blocks: 953371
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8361033 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8361033 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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.
Attached patch Patch v1.1Splinter Review
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.
Attachment #8361033 - Attachment is obsolete: true
Attachment #8361619 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M?][Australis:P3] [feature] p=0
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?
Attachment #8361619 - Flags: review?(gijskruitbosch+bugs) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/ddaf0b95b199
Whiteboard: [Australis:M?][Australis:P3] [feature] p=0 → [Australis:P3] [feature] p=0
https://hg.mozilla.org/mozilla-central/rev/ddaf0b95b199
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
No longer blocks: fxdesktopbacklog
Whiteboard: [Australis:P3] [feature] p=0 → [Australis:P3]
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.
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.
Depends on: 973704
Depends on: 974236
Depends on: 975808
Depends on: 1024375
You need to log in before you can comment on or make changes to this bug.