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

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: marcia, Assigned: (not reading, please use seth@sspitzer.org instead))

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta2
PowerPC
Mac OS X
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx2 theme change])

Attachments

(2 attachments, 7 obsolete attachments)

17.60 KB, patch
mano
: review+
Details | Diff | Splinter Review
780 bytes, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Blocks: 347454
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)
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2 beta2
> 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...
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.

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.
Assignee: nobody → sspitzer
Whiteboard: [Fx2 theme change]
Status: NEW → ASSIGNED
Summary: [mac] Tab listing bar loses styling after tab overflow → [mac] Tab listing bar loses styling after tab overflow background animation

Updated

12 years ago
Flags: blocking-firefox2? → blocking-firefox2+
> 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

12 years ago
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?
> 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.

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?)
Created attachment 233433 [details] [diff] [review]
fix to fix this bug and to bring back the animation on the mac
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...
> 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.
> I'm going to spin this off to another bug, so I can work on it separately.

see bug #348496
Created attachment 233502 [details] [diff] [review]
patch, css still needs some cleanup
Attachment #233433 - Attachment is obsolete: true
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
Created attachment 233584 [details] [diff] [review]
patch, includes fix for bug #348293 (over on all tabs)
Attachment #233502 - Attachment is obsolete: true
Blocks: 348293
Created attachment 233586 [details] [diff] [review]
patch, includes fix for bug #348293 (over on all tabs)
Attachment #233584 - Attachment is obsolete: true
Attachment #233586 - Flags: review?(mconnor)
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.
Blocks: 348674
Created 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]
Attachment #233586 - Attachment is obsolete: true
Attachment #233695 - Flags: review?(mconnor)
Attachment #233586 - Flags: review?(mconnor)
Created attachment 233711 [details] [diff] [review]
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)
Attachment #233711 - Flags: review?(mconnor)
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.
Attachment #233695 - Flags: review?(mconnor) → review?(bugs.mano)
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.
Attachment #233711 - Flags: review?(mconnor) → review?(bugs.mano)
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
Created attachment 233880 [details] [diff] [review]
updated patch, per mano's comments
Attachment #233695 - Attachment is obsolete: true
Attachment #233711 - Attachment is obsolete: true
Attachment #233695 - Flags: review?(bugs.mano)
Attachment #233711 - Flags: review?(bugs.mano)
Attachment #233880 - Flags: review?(bugs.mano)
Comment on attachment 233880 [details] [diff] [review]
updated patch, per mano's comments

failed hunks in tabbrowser.xml
Attachment #233880 - Flags: review?(bugs.mano)
Created attachment 233895 [details] [diff] [review]
patch again
Attachment #233880 - Attachment is obsolete: true
Created attachment 233899 [details] [diff] [review]
a supplimental patch for winstripe
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.
Attachment #233895 - Flags: review+
Blocks: 346581
> 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 on attachment 233899 [details] [diff] [review]
a supplimental patch for winstripe

this supplimental patch has an implied r= from asaf
Attachment #233899 - Flags: review+

Updated

12 years ago
Keywords: fixed1.8.1

Comment 30

12 years ago
Can this be RESOLVED FIXED, or are there still open trunk bugs here?
> 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.