347783 - [mac] Tab listing bar loses styling after tab overflow background animation

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Marcia Knous [:marcia]]

Seen using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b1) Gecko/20060807 BonEcho/2.0b1 on my machine, Asa has seen on his Intel mac as well.

STR:

1. Make sure to have pref checked for "Always show the tab bar."
2. Open the Latest Headlines RSS feed in tabs.
3. After the feeds load, click on the first tab and then select close other tabs.
4. Observe that the styling for the tab listing bar has now disappeared.

Comment 1

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

I've seen this as well.

from the looks of it, it appears like the collaped attribute (on the tabbar) is not changing?

I'll use DOMi to confirm that theory (and get a screen shot)

Comment 2

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

> from the looks of it, it appears like the collaped attribute (on the tabbar) is
> not changing?

sorry, a brain hiccup.  the overflow attribute on the tab bar is what I was thinking of, and it appears to be correct.

using DOMi now to figure this one out...

Comment 3

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

the problem is this:

when you overflow, the animation will finish by setting the opacity of the alltabs-box to 0.0;

when the opacity is 0.0, you hit this bug.

Comment 4

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

from pinstripe's browser.css (see http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/global/browser.css#302)

Were using a background image now and the opacity varies between .65 and 1.0, depending on the state.

298 .tabs-alltabs-box{
299   width: 16px;
300   height: 25px;
301   background-image: url("chrome://global/skin/alltabs-box-bkgnd.png");
302   opacity: 0.65;
303 }
304 
305 .tabs-alltabs-box:hover{
306   opacity: 1.0;
307 }

This shouldn't be here at all, it must have been a merge conflict.

309 .tabs-alltabs-box[flash="true"] {
310   background-color: orange;
311   opacity: 0.0;
312 }

on a related note, because of these .css changes, the overflow animation is broken on the mac.

I'll check in with mike to see what he wants done, and then I can fix this bug.

Comment 5

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

> I'll check in with mike to see what he wants done, and then I can fix this bug.

mike wants:

1) orange on Windows, blue on OS X (use the blue we're using elsewhere in the them, like the home button's door.)
2) New effect should flash the arrow and menu.
3) It shouldn't break the style of the button"

I'm not sure if #2 applies to both mac and windows. 

Comment 6

[Jay Goldman]

Two additional thoughts:

1) We might be changing the Home button's icon (see https://bugzilla.mozilla.org/show_bug.cgi?id=347602), so maybe grab a blue from the Reload button instead.

2) There's a hover state defined on the alltabs-box but it no longer works because of the reorganization of the box and button into a stack. I'm guessing that the button is now absorbing the mouseover event and so the box never gets it. Not sure if there's a way to resolve that?

Comment 7

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

> 2) There's a hover state defined on the alltabs-box but it no longer works
> because of the reorganization of the box and button into a stack. I'm guessing
> that the button is now absorbing the mouseover event and so the box never gets
> it. Not sure if there's a way to resolve that?

I see this, and I'll spin off a bug and work on it.

Comment 8

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

I chatted with dbaron and mconnor about how to do this, and here's the consensus:

I need additional "blue" (and "orange", for windows xp) versions of the following images:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/global/alltabs-box-overflow-bkgnd.png
http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/global/tab-arrow-end-bkgnd.png
http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/global/tab-arrow-start-bkgnd.png

right now, we're using the image (and the .65 opacity) on the background-image of the scrollbutton and the all-tabs box.

the plan is to have stack with the button, a box with the blue version as the background image, a box with the normal version as the background image.

note, we currently have the opacity as .65 unless we are hovering, when it is 1.0.

when animating, I would set the opacity of layer in the stack with the blue version between 0.0 and 1.0

thanks to dbaron for suggesting this approach.

I'll email jay and mconnor directly for the image request.

asaf, to accomplish this, I'll need to add a stack to the arrow scrollbox binding in scrollbox.xml.  (thoughts?)

Comment 9

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

Attachment: fix to fix this bug and to bring back the animation on the mac

Comment 10

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

I've checked in the new images (for pinstripe only) and the only thing I need to do to keep winstripe from breaking is this change to winstripe/global/browser.css

< .tabs-alltabs-box {
---
> .tabs-alltabs-box-animate {

soon, winstripe will have a new theme refresh, and it will have this same bug.

when it does land, I can port the rest of the pinstripe/global/browser.css changes for this bug (and land the new images for winstripe that I received from jay) on the branch.

working on the hover issue now...

Comment 11

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

> 2) There's a hover state defined on the alltabs-box but it no longer works
> because of the reorganization of the box and button into a stack. I'm guessing
> that the button is now absorbing the mouseover event and so the box never gets
> it. Not sure if there's a way to resolve that?

I'm going to spin this off to another bug, so I can work on it separately.

I think to fix this I am going to need create a new binding for the alltabs-button stack (of box,box,toolbarbutton), where the toolbarbutton has handlers for mouseover and mouseout.  then, on mouseover and mouseout of the toolbarbutton, I'd set fakehover to "true" on the first box (which has opacity of .65, except when in the "fakehover state", when it would be 1.0) and remove the fakehover attribute on mouseout.

the reason this has to be a binding is so I can use it from pinstripe/global/globalBindings.xml.  themes are scriptable, so to get mouseover / mouseout to work, it needs to be a new binding in tabbrowser.xml

I'll discuss with mano, to see if this is a good idea or not.

Comment 12

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

> I'm going to spin this off to another bug, so I can work on it separately.

see bug #348496

Comment 13

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

Attachment: patch, css still needs some cleanup

Comment 14

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

this patch fixes this bug, animates both the scrolldown button and the alltabs button, and adds back hover to the alltabs button.

to animate the scrolldown button, I needed to extend the existing scrollbox "click to scroll" binding so that I could use a stack (box1,box2,toolbarbutton) instead of a simple toolbarbutton for the scrolldown button.

this patch includes the css changes in order to keep the hover effect on the scrolldown button (but that might be going away, see bug #347399)

I need to clean up, simplify the css in pinstripe/global/browser.css, as well as fix a couple nits that my patch introduces:

1)  hover images for the scrolldown button and the alltabs button slightly off
2)  dropdown for alltabs is no longer centered

Comment 15

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

Attachment: patch, includes fix for bug #348293 (over on all tabs)

Comment 16

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

Attachment: patch, includes fix for bug #348293 (over on all tabs)

Comment 17

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

to elaborate on the patch:

1)  

there is a new binding, arrowscrollbox-clicktoscroll-animate, which extends arrowscrollbox-clicktoscroll binding.  
this new binding is in tabbrowser.xml, and not scrollbox.xml, as it is special and only really applies to tab browsing.

the new binding replaces the scrollbutton-down with a stack that contains {hbox, hbox, scrollbutton-down}.  The desired 
background image for the scrollbutton-down is on the first hbox, and the second hbox contains the background to show
when animating.  

as with the arrowscrollbox-clicktoscroll binding, I collapse (and uncollapse) the boxes on underflow / overflow.

2)

I'm doing a similar "use stack for animation" trick {hbox, hbox, scrollbutton-down} for the alltabs button.

3) when animating, I animate both the scrollbutton-down and the alltabs button, per mconnor.

4) to fix bug #348293, have hover rules which set the background image on the toolbarbuttons (both scrollbutton-down and the alltabs)

5) I also removed the bogus ".tabs-alltabs-box[flash="true"]" rule, which I think came back with the new theme, as a result of a merge.

note, these changes don't break winstripe, even though winstripe is not refreshed yet.

I'll make a trunk version of the patch that should keep the animation working on trunk (for winstripe and pinstripe) until we land the refresh there.

Comment 18

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

Attachment: patch, includes fix for bug #348293 (over on all tabs) [merged with recent changes to branch for all tabs tooltip fix]

Comment 19

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

Attachment: here's a trunk version of the patch

notice, the trunk does not include all the pinstripe/global/browser.css changes, as those will need to be back ported to the trunk, along with the rest of the new theme.

this works on win32 trunk.  things work on the mac trunk, as much as can be expected.  for example, the animation color is disabled on the mac, due to bug #346738 (aka #325296)

Comment 20

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

Comment on attachment 233695 [details] [diff] [review]
patch, includes fix for bug #348293 (over on all tabs) [merged with recent changes to branch for all tabs tooltip fix]

mconnor is so doomed it is not funny.  switching to asaf for review.

Comment 21

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

Comment on attachment 233711 [details] [diff] [review]
here's a trunk version of the patch

mconnor is so doomed it is not funny.  switching to asaf for review.

Comment 22

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

asaf says over irc:

<Mano> please use a class instead of another attribute and make the binding tabbrowser specific, something like
<Mano> .tabbrowser-tabs > .tabbrowser-arrowscrollbox
<Mano> other than that, i thought you don't need the extra hboes on trunk
<sspitzer> I thought so too, since that fix landed, but I haven't verified and tested yet.
<Mano> also, mDownBoxAnimate can be simplified
<Mano> this.mTabstrip._scrollButtonDownBoxAnimate

Comment 23

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

Attachment: updated patch, per mano's comments

Comment 24

[Asaf Romano (gone)]

Comment on attachment 233880 [details] [diff] [review]
updated patch, per mano's comments

failed hunks in tabbrowser.xml

Comment 25

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

Attachment: patch again

Comment 26

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

Attachment: a supplimental patch for winstripe

Comment 27

[Asaf Romano (gone)]

Comment on attachment 233895 [details] [diff] [review]
patch again

This is still almost invisible IMO, but we have a bug filed on this, I think.

Also file a follow-up on stopping the animation when the arrow button is clicked.

r=mano (branch only) with the winstripe change mentioned on IRC.

Comment 28

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

> This is still almost invisible IMO, but we have a bug filed on this, I think.

yes, see bug #348674

> Also file a follow-up on stopping the animation when the arrow button is
> clicked.

filed it a while ago, but it is a bigger issue now that we animate that button.  see bug #346581

thanks for the review, asaf.

Comment 29

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

Comment on attachment 233899 [details] [diff] [review]
a supplimental patch for winstripe

this supplimental patch has an implied r= from asaf

Comment 30

[Peter Kasting]

Can this be RESOLVED FIXED, or are there still open trunk bugs here?

Comment 31

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

> Can this be RESOLVED FIXED, or are there still open trunk bugs here?

marking as fixed, as this was a theme refresh issue.

once the new theme is back ported to the trunk, if there are issues, we'll deal with them.