Last Comment Bug 713008 - Tabs shouldn't flicker when hovered
: Tabs shouldn't flicker when hovered
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Thunderbird 12.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
Depends on:
Blocks: tb-tabsontop
  Show dependency treegraph
 
Reported: 2011-12-22 10:03 PST by Florian Quèze [:florian] [:flo]
Modified: 2012-01-12 07:03 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Hack (845 bytes, patch)
2011-12-22 10:03 PST, Florian Quèze [:florian] [:flo]
mconley: review+
Details | Diff | Splinter Review
Patch v2 (813 bytes, patch)
2012-01-09 02:40 PST, Florian Quèze [:florian] [:flo]
mconley: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2011-12-22 10:03:05 PST
Created attachment 583837 [details] [diff] [review]
Hack

The first time a non-selected tab is hovered, it flickers because the tab-hover-active.png image isn't loaded yet.

The attached patch reproduces the hack Firefox uses to work-around this: http://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser.css#1885
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2011-12-22 10:36:30 PST
Comment on attachment 583837 [details] [diff] [review]
Hack

Review of attachment 583837 [details] [diff] [review]:
-----------------------------------------------------------------

Florian:

This looks fine to me - and thanks for filing / tackling these issues!

Does this apply to gnomestripe / qute as well?  If so, it's an r+, but you should update the patch to stretch across the other two platforms as well.

Thanks,

-Mike
Comment 2 Florian Quèze [:florian] [:flo] 2011-12-22 10:50:04 PST
(In reply to Mike Conley (:mconley) from comment #1)

> Does this apply to gnomestripe / qute as well?  If so, it's an r+, but you
> should update the patch to stretch across the other two platforms as well.

I hadn't checked at the time I attached the patch (I was mostly interested in getting rid of the flickering that kept attracting my eyes today while frequently restarting Thunderbird to debug something else), but this is the only place where your Tabs-on-top patch adds an image that's visible only on hover.
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-01-06 12:44:48 PST
Florian:

Is this patch supposed to fix the black-box flash thing in the tab selector on start up?  Because I'm still seeing it with this patch...

Also, the patch has bitrotted slightly.

-Mike
Comment 4 Florian Quèze [:florian] [:flo] 2012-01-09 02:40:49 PST
Created attachment 586938 [details] [diff] [review]
Patch v2

(In reply to Mike Conley (:mconley) from comment #3)
> Florian:
> 
> Is this patch supposed to fix the black-box flash thing in the tab selector
> on start up?  Because I'm still seeing it with this patch...
> 
> Also, the patch has bitrotted slightly.

I don't see the the black-box on startup, but only on unselected tabs. My previous patch fixed only the flashing when hovering an unselected tab for the first time, but I've noticed it also happens when opening a tab in the background (during my initial testing, I already had 2 tabs at startup), this new patch fixes this case too, and also unbittrots.
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-01-09 10:43:29 PST
Comment on attachment 586938 [details] [diff] [review]
Patch v2

Review of attachment 586938 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, Florian!

Just two nits.  Modulo those, r=me.

Thanks,

-Mike

::: mail/themes/pinstripe/mail/tabmail.css
@@ -107,4 +106,5 @@
> >  .tabmail-tab:not([selected="true"]):hover {
> >    -moz-border-image: url(tabs/tab-hover-active.png) 4 5 3 6 fill repeat stretch;
> >  }
> >  
> > +/* preloading hack */

Could you quickly document what the preloading hack does, and why we need it?

@@ +108,5 @@
>  }
>  
> +/* preloading hack */
> +#tabs-toolbar::after {
> +   content: '';

The contents of this block should be 2 spaces in, not 3.
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-01-09 10:47:10 PST
Comment on attachment 586938 [details] [diff] [review]
Patch v2

We'll want this for TB 11.
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-01-10 07:03:22 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/439e89ac3f1d
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-01-11 10:08:38 PST
So, I was a bit silly, and I committed this to trunk without letting Florian fix the nits I found.

I've made the corrections I pointed out, and committed to comm-aurora here:

http://hg.mozilla.org/releases/comm-aurora/rev/f03fea92e7fc

and we'll need a follow-up bug to fix the nits in comm-central.  Forthcoming.
Comment 9 Mike Conley (:mconley) - (needinfo me!) 2012-01-11 10:43:54 PST
So it turns out that Florian *did* fix the nits before they were committed as http://hg.mozilla.org/comm-central/rev/439e89ac3f1d.  I think we're good then.

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