The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Theme
--
trivial
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jaws, Assigned: dwendorf)

Tracking

unspecified
Firefox 15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [switch-to-tab][good first bug][mentor=jaws][lang=css])

Attachments

(5 attachments, 3 obsolete attachments)

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.

Updated

6 years ago
Component: Location Bar → Theme
QA Contact: location.bar → theme

Comment 1

6 years ago
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.
(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.
(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); }
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Created attachment 557286 [details]
Screenshot of patch

Screenshot of applied patch.
Attachment #557286 - Flags: feedback?(shorlander)
Created attachment 557287 [details] [diff] [review]
Patch for bug 683390
Attachment #557287 - Flags: review?(dao)
Attachment #557286 - Attachment is patch: false
Attachment #557286 - Attachment mime type: text/plain → image/png
Attachment #557286 - Attachment is patch: false
Attachment #557286 - Attachment mime type: text/plain → image/png
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.
Attachment #557287 - Flags: review?(dao) → review-

Updated

6 years ago
Whiteboard: [switch-to-tab]

Updated

6 years ago
Duplicate of this bug: 558328
Yeah it really only applies to OSX. The other tabs are always attached bottom.

It needs a new graphic.
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."
Attachment #557287 - Attachment is obsolete: true
Attachment #557355 - Flags: review?(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.
Attachment #557355 - Flags: review?(dao) → review-
Assignee: jwein → margaret.leibovic

Comment 11

6 years ago
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? :)
Created attachment 557749 [details] [diff] [review]
Patch with just updated actionicon-tab.png graphics
Attachment #557286 - Flags: feedback?(shorlander)
Assignee: margaret.leibovic → jwein
Assignee: jwein → mwein2009
Assignee: mwein2009 → nobody
Status: ASSIGNED → NEW
Whiteboard: [switch-to-tab] → [switch-to-tab][good first bug][mentor=jwein][lang=css]

Updated

5 years ago
Assignee: nobody → jan.bambach
Assignee: jan.bambach → info
Status: NEW → ASSIGNED
Whiteboard: [switch-to-tab][good first bug][mentor=jwein][lang=css] → [switch-to-tab][good first bug][mentor=jaws][lang=css]
Assignee: info → dan
(Assignee)

Comment 13

5 years ago
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.
Attachment #619838 - Flags: review?(jwein)

Updated

5 years ago
Attachment #619838 - Flags: review?(jwein) → review?(dao)

Updated

5 years ago
Attachment #619838 - Flags: feedback?(jwein)
(Assignee)

Comment 14

5 years ago
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.
Attachment #619909 - Flags: review?(dao)
Attachment #619909 - Flags: feedback?(jwein)
(Assignee)

Updated

5 years ago
Attachment #619909 - Attachment is patch: true
Attachment #557355 - Attachment is obsolete: true
Attachment #619838 - Attachment is obsolete: true
Attachment #619838 - Flags: review?(dao)
Attachment #619838 - Flags: feedback?(jwein)
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?
Attachment #619909 - Flags: feedback?(jwein)
Attachment #619909 - Flags: feedback?(fryn)
Attachment #619909 - Flags: feedback+
Bug 690191 shows a grey switch to tab icon.

Comment 17

5 years ago
Created attachment 620566 [details]
Screenshot of patch on OS X

Comment 18

5 years ago
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+.
Attachment #619909 - Flags: feedback?(fryn) → feedback+

Updated

5 years ago
Attachment #619909 - Flags: review?(dao) → review+
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
Flags: in-testsuite-
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/185af659e9b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Duplicate of this bug: 752899
You need to log in before you can comment on or make changes to this bug.