Closed Bug 874674 Opened 11 years ago Closed 11 years ago

Clean up toolbar icons for Linux GTK theme

Categories

(Firefox :: Toolbars and Customization, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: jaws)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P1][Australis:M9])

Attachments

(1 file, 1 obsolete file)

Bug 866863 landed some pretty icons for the menu panel, but we never got any for the Linux GTK theme.

As a stopgap, we just copied the OSX ones, but that's probably not what we want to do in the long run.

We also did some sloppy overrides in browser.css to make the cut, copy, paste, bookmark and zoom icons not look totally awful (see the patch in bug 866863). Those may get addressed by bug 867368, bug 870901, and/or bug 870897. But if not, they should get taken care of here.
We need Linux icons from shorlander.
Flags: needinfo?(shorlander)
Keywords: icon
Whiteboard: [Australis:M?] → [Australis:M7]
Assignee: nobody → mnoorenberghe+bmo
(In reply to Mike Conley (:mconley) from comment #1)
> We need Linux icons from shorlander.

Waiting on the SVG decision
Flags: needinfo?(shorlander)
What is the fallback plan if we can't use SVG? Would we be able to use one set of Linux PNG icons or would we need to somehow figure out the system theme colour and vary the icons?
Status: NEW → ASSIGNED
Flags: needinfo?(shorlander)
(In reply to Matthew N. [:MattN] from comment #3)
> What is the fallback plan if we can't use SVG? Would we be able to use one
> set of Linux PNG icons or would we need to somehow figure out the system
> theme colour and vary the icons?

I think we have a few options:

1) Keep current approach: Try and use stock-icon and create custom icons for things not found there.

2) Just use a neutral set of icons

The first approach has some issues because we are continually adding more things that might not be covered by stock-icons. Also continuing to create icons in the Tango style doesn't work very well as distros diverge stylistically.

As a fallback I would recommend we create a neutral set of icons and move the panels away from using system colors so we can control the look and feel.
Flags: needinfo?(shorlander)
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Is there a bug tracking the SVG decision? Or is it this one? :)
Flags: needinfo?(mnoorenberghe+bmo)
It was this one and at this point I think we should just go with PNGs. It was discussed briefly with myself, Jared, and shorlander in Toronto but I don't think action items were clear.

Stephen, can you go ahead and make the Linux PNG icons using the same layout as Windows/OS X?
Assignee: mnoorenberghe+bmo → shorlander
Flags: needinfo?(mnoorenberghe+bmo)
Whiteboard: [Australis:M?] → [Australis:P2]
What were the reasons against SVG?

On Windows we need to invert the icons at times because the default ones can become pretty much invisible on dark backgrounds. That's only going to get worse on Linux, except that we can't easily tell when we'd have to invert the icons.
Note also bug 875479, if we're going to do the icons here, that should probably be a dep of this one.
Blocks: 892959
(In reply to Dão Gottwald [:dao] from comment #8)
> What were the reasons against SVG?

I think perf is the only stated objection to them.
Was perf determined to be a problem with SVG images?
Blocks: 899033
(In reply to Dão Gottwald [:dao] from comment #8)
> What were the reasons against SVG?

Added complexity of the SVG approach along with inconsistency of implementation with OS X and Windows were other reasons.

(In reply to Dão Gottwald [:dao] from comment #11)
> Was perf determined to be a problem with SVG images?

Yes. Bug 764299 may help but that's not resolved yet.
(In reply to Matthew N. [:MattN] from comment #12)
> (In reply to Dão Gottwald [:dao] from comment #8)
> > What were the reasons against SVG?
> 
> Added complexity of the SVG approach along with inconsistency of
> implementation with OS X and Windows were other reasons.

We need to start somewhere. SVG icons would be very valuable on Windows (bug 878288). OS X, where we currently need to have double-size versions for all icons, they wouldn't hurt either.

> (In reply to Dão Gottwald [:dao] from comment #11)
> > Was perf determined to be a problem with SVG images?
> 
> Yes. Bug 764299 may help but that's not resolved yet.

My questions was more like whether somebody actually saw/measured performance problems when using SVG as images (not masks/clip paths) in chrome, but ok. Looks like bug 764299 is coming along? Not sure what it's currently waiting for...
At the last Australis meeting, we discussed that we likely wanted to at least have pngs in place for when we land on Nightly, as a not-insignificant number of our Nightly users are on Linux (moreso than on release). Therefore, I think this should be P1 rather than P2. Once a basic level of non-horrible Linux support is in place, we can downgrade back to P2 or lower if we decide that at that point SVG isn't super-important in the end.

Stephen, if I remember right you said it wouldn't be that hard for you to get some basic icons for us to use here, is that right? :-)
Flags: needinfo?(shorlander)
Whiteboard: [Australis:P2] → [Australis:P1]
Whiteboard: [Australis:P1] → [Australis:P1][Australis:M9]
We're gonna roll with Windows icons for the time-being.
Assignee: shorlander → jaws
Flags: needinfo?(shorlander)
Hardware: x86_64 → All
Keywords: icon
Attached patch Patch (obsolete) — Splinter Review
Comment on attachment 810075 [details] [diff] [review]
Patch

Hey Mike, can you check this out and see if I missed anything here? I tested it out by placing all of the toolbaritems in the navbar and all of the icons looked good (with the exception of the tabview icon but that one we appear to handle separately on Windows too).
Attachment #810075 - Attachment description: 874674.patch → Patch
Attachment #810075 - Flags: review?(mdeboer)
Comment on attachment 810075 [details] [diff] [review]
Patch

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

It seems you missed browser/themes/linux/downloads/indicator.css, because when you click the download button and it switches to "downloads-indicator", the icon is swapped to the generic, missing icon one.
In browser.css you also might want to check out the `.unified-nav-back[_moz-menuactive]` rules, because the navigation panel shows the GTK back/ forward icons. Do we want that?
You also might want to check out the `#urlbar-go-button`, `#urlbar-stop-button` and `#search-go-button`, which also show GTK icons while the rest is Australis themed. Do we want that?
Additionally, are we OK with the GTK-style icon for `.tabs-newtab-button`?
Attachment #810075 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> In browser.css you also might want to check out the
> `.unified-nav-back[_moz-menuactive]` rules, because the navigation panel
> shows the GTK back/ forward icons. Do we want that?
> You also might want to check out the `#urlbar-go-button`,
> `#urlbar-stop-button` and `#search-go-button`, which also show GTK icons
> while the rest is Australis themed. Do we want that?
> Additionally, are we OK with the GTK-style icon for `.tabs-newtab-button`?

Scope creep?
(In reply to Dão Gottwald [:dao] from comment #19)
> Scope creep?

Yes, might be. That's why I formulated them as questions, to at least have 'em discussed.
Attached patch Patch v1.1Splinter Review
(In reply to Mike de Boer [:mikedeboer] from comment #18)
> It seems you missed browser/themes/linux/downloads/indicator.css, because
> when you click the download button and it switches to "downloads-indicator",
> the icon is swapped to the generic, missing icon one.

Fixed.

> In browser.css you also might want to check out the
> `.unified-nav-back[_moz-menuactive]` rules, because the navigation panel
> shows the GTK back/ forward icons. Do we want that?

I don't see any issue with this. It is very similar to how we handle it for Windows, so I'd like to leave it as-is at least for this bug. We can handle any changes to these in a follow-up.

> You also might want to check out the `#urlbar-go-button`,
> `#urlbar-stop-button` and `#search-go-button`, which also show GTK icons
> while the rest is Australis themed. Do we want that?

> Additionally, are we OK with the GTK-style icon for `.tabs-newtab-button`?

I would like to handle these two issues in follow-up bugs.
Attachment #810075 - Attachment is obsolete: true
Attachment #811315 - Flags: review?(mdeboer)
Comment on attachment 811315 [details] [diff] [review]
Patch v1.1

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

Checked it out, r=me once you unbitrotted the patch (did this locally, was quite easy).
Attachment #811315 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/projects/ux/rev/4bc44de6cdd2
Whiteboard: [Australis:P1][Australis:M9] → [Australis:P1][Australis:M9][fixed-in-ux]
This seems to have made all the mochitest-bc go orange on Linux. :-(
(In reply to :Gijs Kruitbosch from comment #24)
> This seems to have made all the mochitest-bc go orange on Linux. :-(

Bustage fix pushed: https://hg.mozilla.org/projects/ux/rev/b46dbad96b50

Buttons with no styling that were added in tests got a full toolbar spritesheet. This always happened, but the full toolbar spritesheet is way bigger now, which meant the buttons overflowed, which broke the tests. Other OSes don't have this problem because they don't have the blanket rule that Linux has (see the cset above).

This fixes the issue for me locally. I'll file a followup shortly about removing this rule entirely and/or figuring out a proper solution.
Depends on: 923543
Blocks: 923543
No longer depends on: 923543
Blocks: 923548
Summary: Clean up menu panel icons for Linux GTK theme → Clean up toolbar icons for Linux GTK theme
No longer blocks: 923548
Depends on: 923548
Depends on: 925710
https://hg.mozilla.org/mozilla-central/rev/4bc44de6cdd2
https://hg.mozilla.org/mozilla-central/rev/b46dbad96b50
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][Australis:M9][fixed-in-ux] → [Australis:P1][Australis:M9]
Target Milestone: --- → Firefox 28
Depends on: 969125
Depends on: 947356
You need to log in before you can comment on or make changes to this bug.