348929 - Tab strip right scroll button and All Tabs menu draw bevels on hover

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Peter Kasting]

After the landing of bug 345466, the two buttons on the right of the tab strip both draw bevels on hover on Linux, which looks bizarre.  The left scroll button does not do this.

Weirder, the buttons apparently aren't the same height, as the All Tabs menu's bevel extends down 1 pixel lower than the scroll button's bevel.

Screenshots to follow.

Comment 1

[Peter Kasting]

Attachment: Left scroll button on hover (OK)

Comment 2

[Peter Kasting]

Attachment: Right scroll button on hover (broken)

Comment 3

[Peter Kasting]

Attachment: All Tabs menu on hover (broken)

Comment 4

[Adam Kowalczyk]

I can see this on Windows as well, so I am adjusting the summary and OS.

Comment 5

[Mike Schroepfer]

Setting TM to final - small enough issue to not block b2

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

If it helps, the bevel on the right-scoll button only appears when there are actually tabs to scroll to off the right side of the tabstrip.

Comment 7

[ronin.achilles]

(In reply to comment #5)
> Setting TM to final - small enough issue to not block b2
> 

I agree this is a small issue, but considering that Beta2 is going to be out there for almost 2 months before the final release and will be reviewed widely across major publications, IMHO this bug is too 'visible' and 'sloppy' to be left as it is. I would sincerely request you to reconsider changing the TM to Fx2b2.

Comment 8

[Michael Wu [:mwu]]

*** Bug 348937 has been marked as a duplicate of this bug. ***

Comment 9

[Michael Wu [:mwu]]

Attachment: Remove bevels on tabstrip buttons

Is there any reason why -moz-appearance: none is commented out?

Comment 10

[(not reading, please use [e-mail] instead)]

I can't remember why I did either of these commenting out:

 //  -moz-appearance: none !important;
 //  background: transparent !important;

but it was me, and I handed off a patch to mconnor with those changes (sorry mike).

Comment 11

[Peter Kasting]

(In reply to comment #10)
> I can't remember why I did either of these commenting out:
> 
>  //  -moz-appearance: none !important;
>  //  background: transparent !important;
> 
> but it was me, and I handed off a patch to mconnor with those changes (sorry
> mike).

I wonder if removing the comment on that "background" thing would fix bug 348939.  Maybe both these should be uncommented.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: patch, for michael to test and review

Comment 13

[(not reading, please use [e-mail] instead)]

from michaels patch, adding "-moz-appearance: none !important;" fixes issues with the scrollbutton-down (aka, right scroll button for LTR) and the all tabs button.

The following three rules fix one of the issues covered in bug #348950 (see
https://bugzilla.mozilla.org/attachment.cgi?id=234138 for the issue)

+  width: 18px !important;"
+  border: 0px !important;
+  margin-top: 2px;


I removed:

// background: transparent !important;

we can't set that, because if we do, the alltabs button will not respond on hover.  for the hover effect on alltabs, we use a background image on the button.

michael, was this change for another bug?

+.tabbrowser-tab > .tab-image-left,
+.tabbrowser-tab > .tab-image-right {
+  padding-bottom: 1px;
+  padding-top: 0;
+  margin-bottom: 1px;
+}

can you test and review my patch?

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 234293 [details] [diff] [review]
Remove bevels on tabstrip buttons

clearing review for now.

(I'm sure michael added the tab-image-left and tab-image right for a reason, but waiting to hear why.)

Comment 15

[Michael Wu [:mwu]]

(In reply to comment #14)
> (From update of attachment 234293 [details] [diff] [review] [edit])
> clearing review for now.
> 
> (I'm sure michael added the tab-image-left and tab-image right for a reason,
> but waiting to hear why.)
> 
Oh, sorry, that was for bug 349038. Please ignore.

Comment 16

[Michael Wu [:mwu]]

Looks like this was partially fixed by bug 349122, but the right overflow button is one px too high on hover now.

Comment 17

[(not reading, please use [e-mail] instead)]

> Looks like this was partially fixed by bug 349122, but the right overflow
> button is one px too high on hover now.

I'll update and attach a new patch (post bug #349122)

Comment 18

[Michael Wu [:mwu]]

Attachment: unbitrotted version of seth's patch

I had to unbitrot it to test, so I might as well upload it. Seems to work well.

Comment 19

[(not reading, please use [e-mail] instead)]

thanks for the new patch, michael.  i

your new, unbitrotted patch works like a charm for me, after updating and rebuilding.

it fixes that part of bug #348950

once we get approval, I can land.

Comment 20

[Michael Wu [:mwu]]

Checking in toolkit/themes/winstripe/global/browser.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/browser.css,v  <--  browser.css
new revision: 1.9.4.16; previous revision: 1.9.4.15
done

Comment 21

[(not reading, please use [e-mail] instead)]

I've updated and rebuilt, and on windows, michael's patch (along with other recent checkins) have improved this part of the tabstrip, including fixing this issue:

https://bugzilla.mozilla.org/attachment.cgi?id=234138

thanks michael!