Last Comment Bug 370832 - Null Tab Preview shown in SeaMonkey 1.1.1 with Modern theme
: Null Tab Preview shown in SeaMonkey 1.1.1 with Modern theme
Status: RESOLVED FIXED
: fixed-seamonkey1.1.1
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: 1.8 Branch
: x86 Windows 2000
: -- normal (vote)
: mozilla1.8.1
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
:
Mentors:
Depends on:
Blocks: 345178
  Show dependency treegraph
 
Reported: 2007-02-18 10:00 PST by Steve Snyder
Modified: 2007-02-21 16:34 PST (History)
3 users (show)
kairo: blocking‑seamonkey1.1.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (sort of...) (638 bytes, patch)
2007-02-18 12:20 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: approval‑seamonkey1.1.1+
Details | Diff | Splinter Review
this seems to actually work (2.73 KB, patch)
2007-02-21 06:25 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: superreview+
kairo: approval‑seamonkey1.1.1+
Details | Diff | Splinter Review

Description Steve Snyder 2007-02-18 10:00:50 PST
In the 18 Feb 2007 nightly of SeaMonkey v1.1.1 (ftp://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-mozilla1.8/seamonkey-1.1.1.en-US.win32.installer.exe), the Tab Preview of the current tab consists of an empty tooltip when the Modern theme is in use.

The Tab Previews show correctly with the default Classic theme(SVG thumbnail with non-current tab, tooltip colored text with current tab).  It is only when the Modern theme is in use that the the current tab displays a tootip with no text.  The empty tooltip displays as a small rectangular box hovering above the tab.  The rectangle has no text and contains no color.  Non-current tabs are shown in Modern with the correct SVG thumbnail.
Comment 1 Andrew Schultz 2007-02-18 10:12:58 PST
linux too
Comment 2 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-18 11:50:19 PST
Oops.  This is fallout from Bug 34517
Comment 3 Steve Snyder 2007-02-18 11:54:32 PST
(In reply to comment #2)
> Oops.  This is fallout from Bug 34517

This is fallout from a bug fixed 7 years ago?
Comment 4 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-18 11:58:42 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Oops.  This is fallout from Bug 34517
> 
> This is fallout from a bug fixed 7 years ago?
> 

Yes.  Or maybe I meant bug 345178 ;).
Comment 5 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-18 12:20:11 PST
Created attachment 255604 [details] [diff] [review]
patch (sort of...)

This helps somewhat.  Unfortunately it doesn't fix it 100% of the time.
Comment 6 neil@parkwaycc.co.uk 2007-02-18 12:29:18 PST
Comment on attachment 255604 [details] [diff] [review]
patch (sort of...)

Well, it's a start...
Comment 7 Robert Kaiser 2007-02-19 06:10:00 PST
Is this enough for 1.1.1?
If so, we should get it in fast and respin builds - we want to release really soon, if possible...
Comment 8 neil@parkwaycc.co.uk 2007-02-20 05:32:46 PST
Comment on attachment 255604 [details] [diff] [review]
patch (sort of...)

I think we can go with this at least as a stopgap, it seems to resolve the issue for me even if you don't find it 100% effective.
Comment 9 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-20 05:40:30 PST
(In reply to comment #8)
> (From update of attachment 255604 [details] [diff] [review])
> I think we can go with this at least as a stopgap, it seems to resolve the
> issue for me even if you don't find it 100% effective.
> 

Checked in on branch and trunk.
Comment 10 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-20 05:56:02 PST
Reopening.  On further examination, I can't believe the patch fixes anything.
Comment 11 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-21 06:25:21 PST
Created attachment 255895 [details] [diff] [review]
this seems to actually work

...makes me think this is gecko's fault.
Comment 12 neil@parkwaycc.co.uk 2007-02-21 06:55:47 PST
Comment on attachment 255895 [details] [diff] [review]
this seems to actually work

>+                       onpopuphiding="return this.parentNode.parentNode.parentNode.resetPreview(this);" orient="vertical">
Don't "return", the return value is ignored here.

>+      <!-- This should not be needed, but it seems that the tooltip sizing is
>+           happening too early when we want to stop showing the preview.  This
>+           clears the label's width early (i.e. when the previous preview
>+           disappears) so that when the next tooltip appears, it doesn't start
>+           with a bad size.  For now, I blame Gecko. -->
>+      <method name="resetPreview">
>+        <parameter name="aPopup"/>
>+        <body>
>+        <![CDATA[
>+          var label = aPopup.firstChild;
>+          label.removeAttribute("width");
>+          return true;
>+        ]]>
>+        </body>
>+      </method>
IMHO you should remove the tabpreview attribute too.
Either way, you don't need to remove it/them in doPreview any more.
r+sr=me with these fixed.
Comment 13 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-21 16:24:17 PST
Backed out the old fix and checked in the new one on trunk, with Neil's comments addressed.
Comment 14 Robert Kaiser 2007-02-21 16:24:49 PST
Comment on attachment 255895 [details] [diff] [review]
this seems to actually work

Let's get 1.1.1 rolling... a=me
Comment 15 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-21 16:34:43 PST
Checked in on branch.

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