683390 - Switch to Tab graphic is displayed in the wrong vertical orientation with tabs-on-top enabled

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

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

Attachment: Screenshot of bug

The icon for Switch to Tab shows a tab coming from the top of a tab strip, even when tabs-on-top are enabled.

Comment 1

[:Margaret Leibovic]

We could use -moz-transform: scaleY(-1); to flip the image based on whether or not tabs-on-top are enabled, but I'm not sure how we would make a selector for that, since the richlistitem is in a part of the DOM tree that knows nothing about tabs.

Alternately, we could just replace the image with a flipped version, so that we're at least making it fit with the default tabs setting.

Comment 2

[Stephen Horlander [:shorlander]]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> Alternately, we could just replace the image with a flipped version, so that
> we're at least making it fit with the default tabs setting.

We should probably just do this.

Comment 3

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

(In reply to Margaret Leibovic [:margaret] from comment #1)
> We could use -moz-transform: scaleY(-1); to flip the image based on whether
> or not tabs-on-top are enabled, but I'm not sure how we would make a
> selector for that, since the richlistitem is in a part of the DOM tree that
> knows nothing about tabs.

We can do a CSS selector for this because we store the tabsontop attribute on window. For example:

window[tabsontop="true"] .ac-action-icon { -moz-transform: scaleY(-1); }

Comment 4

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

Attachment: Screenshot of patch

Screenshot of applied patch.

Comment 5

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

Attachment: Patch for bug 683390

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 557287 [details] [diff] [review]
Patch for bug 683390

>--- a/browser/themes/winstripe/browser/browser.css
>+++ b/browser/themes/winstripe/browser/browser.css

> richlistitem[type~="action"][actiontype="switchtab"] > .ac-url-box > .ac-action-icon {
>   list-style-image: url("chrome://browser/skin/actionicon-tab.png");
> }
> 
>+window[tabsontop="true"] richlistitem[type~="action"][actiontype="switchtab"] > .ac-url-box > .ac-action-icon {
>+  -moz-transform: scaleY(-1);
>+}

The icon is still wrong for tabs on bottom... It was, in fact, never right for Windows and Linux.

Comment 8

[Stephen Horlander [:shorlander]]

Yeah it really only applies to OSX. The other tabs are always attached bottom.

It needs a new graphic.

Comment 9

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

Attachment: Patch for bug 683390

Updated the patch to be correct on gnomestripe and winstripe.

I understand that we may want a new graphic, but I think we can land this patch in the meantime. As the saying goes, "perfect is the enemy of good."

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 557355 [details] [diff] [review]
Patch for bug 683390

If we don't get a newly designed icon, the obvious solution is to just open the existing one in a graphics editing tool and flip it over.

Comment 11

[:Margaret Leibovic]

I think this will be a good first bug for our new intern. Stephen, could you have a new image ready before next week so that he can whip up a patch by the end of his first day? :)

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: Patch with just updated actionicon-tab.png graphics

Comment 13

[Dan Wendorf [:dwendorf]]

Attachment: Patch with hover state and padding

In this patch, I'm using the PNGs from Stephen Horlander and also using the hover-state graphic.

Additionally, the width of the graphic changed from 23 to 16 (getting rid of whitespace on both sides), so I set a left and right padding of 3px to maintain the correct whitespace.

Comment 14

[Dan Wendorf [:dwendorf]]

Attachment: Include Stephen's PNGs and remove Gnomestripe hover

My second patch includes the changed PNGs, and fixes Gnomestripe. I did not initially notice that the new Gnomestripe PNG had no hoverstate (it is only 16x16), so I have removed the CSS support for it.

Comment 15

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

Comment on attachment 619909 [details] [diff] [review]
Include Stephen's PNGs and remove Gnomestripe hover

Looks good to me on Windows & Linux. I didn't test it out on Mac, but the CSS looks fine.

Frank, can you test this patch out on Mac?

Comment 16

[u428464]

Bug 690191 shows a grey switch to tab icon.

Comment 17

[Frank Yan (:fryn)]

Attachment: Screenshot of patch on OS X

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 619909 [details] [diff] [review]
Include Stephen's PNGs and remove Gnomestripe hover

If the UI is supposed to look like this with the patch, then feedback+.

Comment 19

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

Thanks for the patch Dan! I've now pushed this to mozilla-inbound where it will stay for a day or two before getting merged to mozilla-central. When it is merged to mozilla-central it should show up in the next day's Nightly build.

https://hg.mozilla.org/integration/mozilla-inbound/rev/185af659e9b1

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/185af659e9b1