Last Comment Bug 683390 - Switch to Tab graphic is displayed in the wrong vertical orientation with tabs-on-top enabled
: Switch to Tab graphic is displayed in the wrong vertical orientation with tab...
Status: RESOLVED FIXED
[switch-to-tab][good first bug][mento...
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All All
: -- trivial (vote)
: Firefox 15
Assigned To: Dan Wendorf [:dwendorf]
:
Mentors:
: 558328 752899 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-30 17:23 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2013-11-12 00:57 PST (History)
11 users (show)
jaws: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Screenshot of bug (15.84 KB, image/png)
2011-08-30 17:23 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details
Screenshot of patch (186.06 KB, image/png)
2011-08-31 13:03 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details
Patch for bug 683390 (2.72 KB, patch)
2011-08-31 13:06 PDT, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug 683390 (2.50 KB, patch)
2011-08-31 16:05 PDT, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch with just updated actionicon-tab.png graphics (2.57 KB, patch)
2011-09-01 19:41 PDT, Stephen Horlander [:shorlander]
no flags Details | Diff | Splinter Review
Patch with hover state and padding (2.90 KB, patch)
2012-04-30 22:38 PDT, Dan Wendorf [:dwendorf]
no flags Details | Diff | Splinter Review
Include Stephen's PNGs and remove Gnomestripe hover (5.07 KB, patch)
2012-05-01 06:34 PDT, Dan Wendorf [:dwendorf]
dao+bmo: review+
jaws: feedback+
fryn: feedback+
Details | Diff | Splinter Review
Screenshot of patch on OS X (11.37 KB, image/png)
2012-05-02 19:42 PDT, Frank Yan (:fryn)
no flags Details

Description Jared Wein [:jaws] (please needinfo? me) 2011-08-30 17:23:58 PDT
Created attachment 557043 [details]
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 2011-08-31 11:02:35 PDT
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] 2011-08-31 11:07:38 PDT
(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) 2011-08-31 12:49:49 PDT
(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) 2011-08-31 13:03:57 PDT
Created attachment 557286 [details]
Screenshot of patch

Screenshot of applied patch.
Comment 5 Jared Wein [:jaws] (please needinfo? me) 2011-08-31 13:06:01 PDT
Created attachment 557287 [details] [diff] [review]
Patch for bug 683390
Comment 6 Dão Gottwald [:dao] 2011-08-31 13:09:57 PDT
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 7 Dão Gottwald [:dao] 2011-08-31 13:12:01 PDT
*** Bug 558328 has been marked as a duplicate of this bug. ***
Comment 8 Stephen Horlander [:shorlander] 2011-08-31 13:12:58 PDT
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) 2011-08-31 16:05:55 PDT
Created attachment 557355 [details] [diff] [review]
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] 2011-09-01 00:39:05 PDT
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 2011-09-01 16:26:28 PDT
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] 2011-09-01 19:41:42 PDT
Created attachment 557749 [details] [diff] [review]
Patch with just updated actionicon-tab.png graphics
Comment 13 Dan Wendorf [:dwendorf] 2012-04-30 22:38:46 PDT
Created attachment 619838 [details] [diff] [review]
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] 2012-05-01 06:34:41 PDT
Created attachment 619909 [details] [diff] [review]
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) 2012-05-01 06:54:31 PDT
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 Guillaume C. [:ge3k0s] 2012-05-01 10:08:51 PDT
Bug 690191 shows a grey switch to tab icon.
Comment 17 Frank Yan (:fryn) 2012-05-02 19:42:24 PDT
Created attachment 620566 [details]
Screenshot of patch on OS X
Comment 18 Frank Yan (:fryn) 2012-05-02 19:43:54 PDT
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) 2012-05-04 09:17:56 PDT
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 (⌚ UTC+1/+2) 2012-05-05 03:38:56 PDT
https://hg.mozilla.org/mozilla-central/rev/185af659e9b1
Comment 21 Dão Gottwald [:dao] 2012-05-08 07:26:26 PDT
*** Bug 752899 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.