956130 - The tab groups button has the wrong icon when it is shown in non-overflow mode on Windows

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

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

Attachment: Screenshot of bug

See attachment.

Comment 1

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

Attachment: Patch

The CSS at http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser-aero.css#236 is what was setting the wrong icon here. Adding :not(#tabview-button) to this isn't right, because the primaryToolbarButtons is basically only used for setting the icon sprite but the tabview button doesn't use the sprite. Also, adding :not(#tabview-button) changes the specificity of the selector enough to make http://mxr.mozilla.org/mozilla-central/source/browser/themes/windows/browser.css#1619 no longer have the most precedence.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

Yes, I think this is the right course of action here.

Comment 3

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

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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57f666e0fdb9

Comment 7

[Girish Sharma [:Optimizer]]

The icon is still wrong. Although its the tab groups icon, but its the black version. not the white one.

Comment 8

[Paul Silaghi, QA [:pauly]]

(In reply to Girish Sharma [:Optimizer] from comment #7)
> The icon is still wrong. Although its the tab groups icon, but its the black
> version. not the white one.
Confirmed, don't know if it's good or bad.
Also, I don't see the panorama icon only after creating some groups first. What's the expected behavior here?

Comment 9

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

(In reply to Girish Sharma [:Optimizer] from comment #7)
> The icon is still wrong. Although its the tab groups icon, but its the black
> version. not the white one.

It should only show the inverted (white) icon if there is a lightweight theme applied that uses bright-text.

(In reply to Paul Silaghi, QA [:pauly] from comment #8)
> Also, I don't see the panorama icon only after creating some groups first.
> What's the expected behavior here?

I'm confused by what this is saying.

Did you use a new profile, enter the Tab Groups view (Ctrl+Shift+E), create some tab groups, exit the Tab Groups view, and not see the Tab Groups icon in the toolbar? I'm not sure if we did that before.

Or did you mean something different?

Comment 10

[Girish Sharma [:Optimizer]]

(In reply to Jared Wein [:jaws] from comment #9)
> (In reply to Girish Sharma [:Optimizer] from comment #7)
> > The icon is still wrong. Although its the tab groups icon, but its the black
> > version. not the white one.
> 
> It should only show the inverted (white) icon if there is a lightweight
> theme applied that uses bright-text.


No It should not. The downloads button, when put in tabs toolbar , is also white. This was the behavior prior to Australis too.

> (In reply to Paul Silaghi, QA [:pauly] from comment #8)
> > Also, I don't see the panorama icon only after creating some groups first.
> > What's the expected behavior here?
> 
> I'm confused by what this is saying.
> 
> Did you use a new profile, enter the Tab Groups view (Ctrl+Shift+E), create
> some tab groups, exit the Tab Groups view, and not see the Tab Groups icon
> in the toolbar? I'm not sure if we did that before.
> 
> Or did you mean something different?

I think he means that until we create atleast 2 tab groups, we do not see the tab groups icon

Comment 11

[Paul Silaghi, QA [:pauly]]

(In reply to Girish Sharma [:Optimizer] from comment #10)
> I think he means that until we create atleast 2 tab groups, we do not see
> the tab groups icon
Exactly. 

> enter the Tab Groups view (Ctrl+Shift+E), create
> some tab groups, exit the Tab Groups view
This isn't a very common scenario, people don't know this shortcut (that isn't posted anywhere actually).
Why not show the icon from the beginning ?

Comment 12

[:Gijs (he/him)]

(In reply to Girish Sharma [:Optimizer] from comment #10)
> (In reply to Jared Wein [:jaws] from comment #9)
> > (In reply to Girish Sharma [:Optimizer] from comment #7)
> > > The icon is still wrong. Although its the tab groups icon, but its the black
> > > version. not the white one.
> > 
> > It should only show the inverted (white) icon if there is a lightweight
> > theme applied that uses bright-text.
> 
> 
> No It should not. The downloads button, when put in tabs toolbar , is also
> white. This was the behavior prior to Australis too.

I agree with you, but different people disagree, see bug 956491. Stephen, could you make a call here, please?

Comment 13

[Girish Sharma [:Optimizer]]

Then the downloads icon should also be made dark, to be consistent.
Also, this means that we would be changing the default behavior which has been there since panorama (light themes, not the tab groups) existed.

Comment 14

[Girish Sharma [:Optimizer]]

Until Stephen decides on the final solution, can we at least have this to its original state before the regression due to Australis ?

Comment 15

[:Gijs (he/him)]

I think the current state is livable if not releasable, so I'd rather get a decision from Stephen before touching this code more.

Comment 16

[Stephen Horlander [:shorlander]]

Answer in bug 956491

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0
Build ID: 20140317004002

The tab groups button is now implemented accordingly to design.
Marking issue as verified.