315207 - Show tab preview in tab tooltips

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

See http://ted.mielczarek.org/code/mozilla/tabpreview/

When hovering over a tab, the tooltip should show a preview of the tab's contents.

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch

Comment 2

[Stefan [:stefanh]]

Can we make this a pref (off by default)? It's nice, but how many people will find this useful (and not annoying)?

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v2

Adds a label showing the tab's title.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v3

Don't break in the case of a non-canvas build.

Comment 5

[[e-mail]]

Comment on attachment 202173 [details] [diff] [review]
patch v3

>+          <xul:tooltip onpopupshowing="return this.parentNode.parentNode.parentNode.doPreview(this, event);">
>+            <xul:vbox>
You don't need a vbox here, just use orient="vertical" on the tooltip
>+              <xul:label class="tooltip-label" crop="right" flex="1"/>
Flex is meaningless here.
>+              <html:canvas id="tabpreviewcanvas" flex="0"/>
id??

>+          var tab = document.tooltipNode;
>+          var b = this.getBrowserForTab(tab);
You should check that tab is actually a tab...

>+          var label = aNode.firstChild.childNodes[0];
Don't mix firstChild and childNodes[0]; as you're only dealing with two nodes, use firstChild and lastChild appropriately.

>+          var canvasH = canvasW*(h/w);
Nits: no spaces; ()s don't gain you here. Should canvasH be an integer?

>+          aNode.firstChild.style.width = canvasW+"px";
>+
>+          var canvas = aNode.firstChild.childNodes[1];
>+          canvas.style.width=canvasW+"px";
>+          canvas.style.height=canvasH+"px";
>+          canvas.width=canvasW;
>+          canvas.height=canvasH;
Do the height and width really need to be set so many times?

>+          try {
>+            var ctx = canvas.getContext("2d");
This should go earlier to save computing the other stuff.

>+            ctx.drawWindow(win, 0, 0, w, h+win.scrollMaxY, "rgb(255,255,255)");
What about horizontal scrolling? (Or what is scrollMaxY for?) Why rgb(255,255,255)?

>+            canvas.style.display="none";
Nit: space around = (again)

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Patch in a moment.

(In reply to comment #5)
> You don't need a vbox here, just use orient="vertical" on the tooltip
Fixed

> >+              <xul:label class="tooltip-label" crop="right" flex="1"/>
> Flex is meaningless here.
Fixed

> >+              <html:canvas id="tabpreviewcanvas" flex="0"/>
> id??
Fixed

> >+          var tab = document.tooltipNode;
> >+          var b = this.getBrowserForTab(tab);
> You should check that tab is actually a tab...
Fixed

> >+          var label = aNode.firstChild.childNodes[0];
> Don't mix firstChild and childNodes[0]; as you're only dealing with two nodes,
> use firstChild and lastChild appropriately.
Fixed

> >+          var canvasH = canvasW*(h/w);
> Nits: no spaces; ()s don't gain you here. Should canvasH be an integer?
Yes, fixed


> >+          aNode.firstChild.style.width = canvasW+"px";
> >+
> >+          var canvas = aNode.firstChild.childNodes[1];
> >+          canvas.style.width=canvasW+"px";
> >+          canvas.style.height=canvasH+"px";
> >+          canvas.width=canvasW;
> >+          canvas.height=canvasH;
> Do the height and width really need to be set so many times?
Yes.  The style affects the display, and the attributes / properties / whatever they are affect the canvas itself.  Leave out the style and the canvas is the wrong size, leave out the other two and the scaling doesn't work.


> >+          try {
> >+            var ctx = canvas.getContext("2d");
> This should go earlier to save computing the other stuff.
Fixed

> >+            ctx.drawWindow(win, 0, 0, w, h+win.scrollMaxY, "rgb(255,255,255)");
> What about horizontal scrolling? (Or what is scrollMaxY for?) Why
> rgb(255,255,255)?
Fixed horizontal scrolling.  RGB(255,255,255) for white background (if the page is transparent(?)) and not RGBA because I want it to be opaque.

> >+            canvas.style.display="none";
> Nit: space around = (again)
Fixed

Comment 7

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v4

Comment 8

[[e-mail]]

Comment on attachment 203491 [details] [diff] [review]
patch v4

>+            <html:canvas flex="0"/>
Don't need this flex either.

>+          var tab = document.tooltipNode;
>+          if (tab.localName != "tab")
>+            return false;
>+          var b = this.getBrowserForTab(tab);
>+          if (!b)
>+            return false;
Hmm, I'm thinking too much of the tab dnd patch (and that also explains the lack of a tab check, which you don't actually need without tab dnd), I was wondering why you hadn't used tab.linkedBrower :-/

>+          var canvasW = 300;
const?

>+          label.style.width = canvasW+"px";
label.width = canvasW; should suffice. It also belongs after the try/catch so on systems without canvas the tooltip will have its default variable width.

>+            canvas.style.display = "none";
Surely there's nothing to hide if there's no canvas support?

>+          canvas.style.width = canvasW+"px";
>+          canvas.style.height = canvasH+"px";
Nit: spaces around +

>+          ctx.drawWindow(win, 0, 0, w + win.scrollMaxX, h + win.scrollMaxY, "rgb(255,255,255)");
I had two suggestions for alternatives: a) the tooltip's background colour b) the default background colour in preferences.

sr=me with these issues addressed.

Comment 9

[Peter Weilbacher]

I just tried this, it's really cool. But can I echo Stefan's comment 2 to add a pref to your patch? The window is damn big... Personally I would like it at 150px width instead of 300, so my idea is to make a hidden integer pref which gives the size of the preview width, and if it's 0 don't show a preview at all.

The Tab Preview extension for Firefox has more prefs (sizing to tab width, in percentage of browser screen size, a preview delay) but a width (or height) setting should be the minimum.

Comment 10

[Bruno 'Aqualon' Escherl]

Contrary to comment #9 I think 300px is to small for users with higher resolutions. The best would be a user defined percentage value.

To tab preview itself. I tested it a while now and It didn't work all the time. Sometimes the preview is available in the same time as the old tooltip, sometimes it needs much longer and sometimes it doesn't show up at all (changing between some tabs makes it work again with these tabs).

Even if it works like expected I can't see how it should improve usability of SeaMonkey. It's a nice gimmick, but in my opinion switching through all tabs with Ctrl + Tab is much faster when you search a certain tab. I have to admit that I don't use the text tool tips at all, but if someone finds this useful, I'm not against shipping SeaMonkey with it (afaik if you don't want to use it, you will never see it at all).

Bruno

P.S. I tested it with a self compiled Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051205 SeaMonkey/1.0b. 

Buildconfig was: --enable-application=suite '--enable-optimize=-O2 -G7 -GL -arch:SSE2' --disable-debug --disable-tests --enable-svg --enable-canvas

Comment 11

[Peter Weilbacher]

I think I found another problem with this feature. On many long pages (most often when using lxr) I get crashes when the tooltip with the preview is supposed to show, both under Linux and OS/2 from 1.8 branch. I could not get a stack for this for some reason, so I don't know anything about the cause.
This should probably go into a new bug, the reason for posting this here now is that even though this is probably not the fault of this feature/patch but somewhere in the backend, enabling this feature (even defaulting to off) would make SeaMonkey unstable for several users.

(In reply to comment #10)
> I tested it a while now and It didn't work all the time.
> Sometimes the preview is available in the same time as the old tooltip,
> sometimes it needs much longer and sometimes it doesn't show up at all
> (changing between some tabs makes it work again with these tabs).

This I see here as well but I couldn't come up with a reproducible scenario, either.

Comment 12

[Bruno 'Aqualon' Escherl]

Another thing. The preview always shows the top of a page and not the real viewport, so if you scrolled down a page, you probably won't recognize it, cause it looks different.

Would it be possible to show the real viewport of the tab?

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #11)
> I think I found another problem with this feature. On many long pages (most
> often when using lxr) I get crashes when the tooltip with the preview is
> supposed to show, both under Linux and OS/2 from 1.8 branch. I could not get a
> stack for this for some reason, so I don't know anything about the cause.
> This should probably go into a new bug, the reason for posting this here now is
> that even though this is probably not the fault of this feature/patch but
> somewhere in the backend, enabling this feature (even defaulting to off) would
> make SeaMonkey unstable for several users.
> 

The program 'Gecko' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadAlloc (insufficient resources for operation)'.
  (Details: serial 48734 error_code 11 request_code 53 minor_code 0)
  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)



> (In reply to comment #10)
> > I tested it a while now and It didn't work all the time.
> > Sometimes the preview is available in the same time as the old tooltip,
> > sometimes it needs much longer and sometimes it doesn't show up at all
> > (changing between some tabs makes it work again with these tabs).
> 
> This I see here as well but I couldn't come up with a reproducible scenario,
> either.

I've seen that behavior sometimes when hacking tooltip-related stuff (regardless of what exactly I'm doing), and don't know what exactly causes it or how to reproduce it.

Comment 14

[Bruno 'Aqualon' Escherl]

- ctx.drawWindow(win, 0, 0, w + win.scrollMaxX, h + win.scrollMaxY, "rgb(255,255,255)");

+ ctx.drawWindow(win, win.pageXOffset, win.pageYOffset, w + win.pageXOffset, h + win.pageYOffset, "rgb(255,255,255)");

This would fix the wrong viewport problem, but there are still hangs of a few second sometimes for bigger pages (but no crash so far for me in windows and actual branch build with that change).

Comment 15

[Bruno 'Aqualon' Escherl]

(In reply to comment #14)
> + ctx.drawWindow(win, win.pageXOffset, win.pageYOffset, w + win.pageXOffset, h
> + win.pageYOffset, "rgb(255,255,255)");
I have to correct myself, the following line works like expected:

ctx.drawWindow(win, win.pageXOffset, win.pageYOffset, w, h, "rgb(255,255,255)");

I tested it with nsCSSFrameConstructor.cpp and SeaMonkey always showed the correct viewport without any bad effects on mem usage.

I don't know if page<coord>Offset or scroll<coord> are to be preferred here (seems to be the same according to Gecko DOM Reference).

Comment 16

[(not currently active) Ted Mielczarek]

I've seen drawWindow cause hangs if you use that technique and try to render the WHAT-WG WebApps spec: http://whatwg.org/specs/web-apps/current-work/

drawWindow throws an OOM exception for me.

Comment 17

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #16)
> I've seen drawWindow cause hangs if you use that technique and try to render
> the WHAT-WG WebApps spec: http://whatwg.org/specs/web-apps/current-work/
> 
> drawWindow throws an OOM exception for me.
> 

Works fine for me, and it's *much* faster in general on big pages.

Comment 18

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v5

1. Display the viewport as the user sees it (i.e. scrolled).  This seems to solve ridiculous memory use and horrible slowdowns for large pages.
2. Support a pref to disable the preview.  On by default.
2b. If you don't have canvas or the pref is off, the tooltip is no longer truncated unnecessarily
3. Support a pref to change the width from the default of 300px to another size.
4. Draw against the user's background color.

Comment 19

[Daniel Brooks [:db48x]]

Comment on attachment 207730 [details] [diff] [review]
patch v5

r=db48x

Comment 20

[Bruno 'Aqualon' Escherl]

(In reply to comment #18)
> 3. Support a pref to change the width from the default of 300px to another
> size.
Personally I would prefer a percentage value for this (e.g. default 1/4). This would make it easier to use a profile on different screen resolutions. Maybe an extreme example: 300px is big enough for a notebook with 1024x768, but on the desktop with screen width 1900 or above and high dpi, it would be quite small. On the other hand, adjusting it to the big resolution would make it way too big for the little one.

Would be nice, if you could consider that before comitting the patch.

Comment 21

[Jayson King]

Attachment: Screenshot of too-large tooltip when tab preview is disabled.

A few things I noticed when testing this:

1) If browser.tabs.tooltippreview.disable is set to true (preview is disabled), then the tab tooltip is much too large. See the attached screenshot of what this looks like. I would expect the "old" tooltips to be used in this case (where the box is only big enough for the text, the page title). 

2) The tab preview does not show anything at all if tooltips are disabled in the browser, regardless of the earlier setting (by un-checking "Show Tooltips" under Appearance). This does not seem appropriate. I personally don't consider the tab preview to be a "tooltip" (though others may disagree), and a user should be able to use tab preview even if tooltips are disabled (as they are two different things to me, and tooltips are useless to experienced users). 

My suggestions:

Can we make tab previews work even if browser.chrome.toolbar_tips ("Show Tooltips") is false? At the least, make a note somewhere that the tooltips option will also affect tab previews, as this can really be confusing. 

Tooltip needs to be fixed when tab previews are disabled, and not be too large. See the attached screenshot.

Comment 22

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #21)
> Created an attachment (id=208144) [edit]
> Screenshot of too-large tooltip when tab preview is disabled. 
> 
> A few things I noticed when testing this:
> 
> 1) If browser.tabs.tooltippreview.disable is set to true (preview is disabled),
> then the tab tooltip is much too large. See the attached screenshot of what
> this looks like. I would expect the "old" tooltips to be used in this case
> (where the box is only big enough for the text, the page title). 

Had you previously had the preview enabled?  If so, you need to restart the browser.  I could easily make it behave properly, but it would be another few lines of code.  If it's decided that we want it, I'll include them.

The changes required would be:
a) move "var canvas..." above the preceeding try {}
b) set canvas.style.display to be none before the "return true;" in the try{}

> 2) The tab preview does not show anything at all if tooltips are disabled in
> the browser, regardless of the earlier setting (by un-checking "Show Tooltips"
> under Appearance).

They're definitely tooltips, and I don't know what would be required to make them not be.

Comment 23

[Jason Bassford]

> I could easily make it behave properly, but it would be another few
> lines of code.  If it's decided that we want it, I'll include them.

FWIW: My opinion is that if this is disabled, things should behave just as they did prior to this code being introduced - otherwise I'd consider it to be a regression.

Comment 24

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #23)
> > I could easily make it behave properly, but it would be another few
> > lines of code.  If it's decided that we want it, I'll include them.
> 
> FWIW: My opinion is that if this is disabled, things should behave just as they
> did prior to this code being introduced - otherwise I'd consider it to be a
> regression.
> 

It does, if you close the browser window and open a new one.

Comment 25

[Jayson King]

(In reply to comment #22)
> Had you previously had the preview enabled?  If so, you need to restart the
> browser.  I could easily make it behave properly, but it would be another few
> lines of code.  If it's decided that we want it, I'll include them.

Restarting the browser does not fix it for me, I still get the too-large tooltips when preview is disabled even after restarting. So adding the extra few lines that you mentioned would be good if it helps. 

> They're definitely tooltips, and I don't know what would be required to make
> them not be.

Alright. If its decided that they're treated as tooltips, then can we refer to them as "Tab Preview Tooltips" or something similar? Because the wording "Tab Preview" alone doesn't indicate that they're tooltips, and tooltips are generally just rectangles with words in them, not pictures of webpages (I'm saying this from the perspective of every other application that uses tooltips). 

I'm not fighting over them working like tooltips, it just needs to be made clear that they're tooltips in the browser, so experienced users who have disabled tooltips wouldn't be confused when Tab Previews don't work. (The default setting is for tooltips to be enabled, so this won't matter as much for new users). 

Just my suggestions. 

Comment 26

[Jayson King]

(In reply to comment #24)
> (In reply to comment #23)
> > > I could easily make it behave properly, but it would be another few
> > > lines of code.  If it's decided that we want it, I'll include them.
> > 
> > FWIW: My opinion is that if this is disabled, things should behave just as they
> > did prior to this code being introduced - otherwise I'd consider it to be a
> > regression.
> > 
> 
> It does, if you close the browser window and open a new one.
> 

On my system, if I disable the tooltips and restart the browser, I get the behavior of the screenshot I posted earlier. This is with using the latest patch (v5). I haven't tried any of the earlier versions. 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060107 SeaMonkey/1.0b

Windows XP SP2

Comment 27

[Jayson King]

Attachment: Preferences patch

Proposed patch for how I envision preferences to be set for Tab Preview. Hoping I'm doing this right, this is my first Mozilla patch. 

This adds default settings for Tab Preview. The feature is disabled by default, and has a 300px width (from the v5 patch). I haven't accounted for a possible percentage setting if one is needed. 

The "Appearance" prefs window seemed like the most suitable place to put this. I also considered the "Navigator-> Tabbed Browsing" window, but it seemed like this preference was best grouped with the Tooltips options instead since that's what they are. 

Tab Preview prefs can be set if and only if Tooltips are checked. This should avoid any confusion about the availability of Tab Previews which depends on whether Tooltips are turned on or off. 

The "pref-appearance.js" is a new file that was modified from a copy of pref-keynav.js to enable/disable the preferences for Tab Preview based on the Tooltips setting. 

Diffed against trunk.

Comment 28

[Jayson King]

Attachment: Preferences patch ver 2

Updated to disable width setting if tab preview setting is unchecked.

Comment 29

[Jayson King]

Attachment: Preferences patch ver 2.01

Oops. Disregard the ver 2 patch, I accidentally changed the default to enable previews. This one changes the default back to disable previews as it was originally.

Comment 30

[Peter Weilbacher]

Do we really need a prefs UI at this point if this feature is disabled by default? Well, in the Appearance panel there is still enough free space... About the patch itself: I don't know why, but for me it doesn't work that the preview related items are disabled when tooltips are deselected.

If the two lines to browser-prefs.js from the pref UI patch are taken, do we then need the hardcoded 300px in the backend patch?

About the "backend" patch: I also see the same problem that when I disable the previews I still get the large tooltips (even after switching windows or restarting), so adding the extra lines might indeed be a good idea.

Comment 31

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #30)
> Do we really need a prefs UI at this point if this feature is disabled by
> default? Well, in the Appearance panel there is still enough free space...
> About the patch itself: I don't know why, but for me it doesn't work that the
> preview related items are disabled when tooltips are deselected.

I'm not sure a UI for these prefs would belong under Appearance, since it's browser-only...

> If the two lines to browser-prefs.js from the pref UI patch are taken, do we
> then need the hardcoded 300px in the backend patch?

I'm not sure - if we can safely assume we'll get a value, no.  If we still have to read prefs safely and use a reasonable default, yes.

> About the "backend" patch: I also see the same problem that when I disable the
> previews I still get the large tooltips (even after switching windows or
> restarting), so adding the extra lines might indeed be a good idea.

I still can't reproduce that.  Can you look in DOM Inspector and see what's causing it to be tall?

Comment 32

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

It looks even better with a 1px solid gray border around the canvas.

Comment 33

[Peter Weilbacher]

(In reply to comment #31)
> I still can't reproduce that.  Can you look in DOM Inspector and see what's
> causing it to be tall?

Of course now that I want to investigate a browser restart cures the problem. But with just a new window I still saw a html:canvas node in the DOM tree below xul:tooltip that had height of 168px and width of 300px. After the restart it has height=width=0.

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

imagine the patch had a comment before
+          canvas.style.width = canvasW+"px";
which said...
//XXXcst - working around a bug here, the .style. lines shouldn't be needed

Comment 35

[[e-mail]]

Comment on attachment 208181 [details] [diff] [review]
Preferences patch ver 2.01

>+pref("browser.tabs.tooltippreview.disable", true);
Why not "browser.tabs.tooltippreview.enable", false ?

>Index: mozilla/xpfe/components/prefwindow/resources/content/pref-appearance.js
Wrong place to add a new file, at least on the trunk; jar.mn wasn't updated either. Alternatively, you might decide that the function is small enough to inline.

>+ * Contributor(s): Aaron Leventhal
>+ *                 Asaf Romano
>+ *                 Ian Neal
I doubt that ;-) Also, is the licence the correct format?

>+    document.getElementById('previewWidth').disabled = !isTooltipEnabled || !isTabPreviewEnabled;
>+    document.getElementById('previewWidthLabel').disabled = !isTooltipEnabled || !isTabPreviewEnabled;
>+    document.getElementById('previewWidthUnitLabel').disabled = !isTooltipEnabled || !isTabPreviewEnabled;
You could consolidate these using a broadcaster or chained assignment.

>+<!ENTITY previewWidth.label                     "Width of preview">
Missing :

Note that this makes the appearance preferences way too big anyway.

Comment 36

[[e-mail]]

Comment on attachment 207730 [details] [diff] [review]
patch v5

>-          <xul:tooltip onpopupshowing="event.preventBubble(); if (document.tooltipNode.hasAttribute('label')) { this.setAttribute('label', document.tooltipNode.getAttribute('label')); return true; } return false;"/>
Might be worthwhile keeping the stopPropagation() (was preventBubble).

>+          <xul:tooltip onpopupshowing="return this.parentNode.parentNode.parentNode.doPreview(this, event);" orient="vertical">
doPreview only takes one parameter.

>+            <html:canvas flex="0"/>
Remove the flex="0" as it is the default.

>+      <method name="doPreview">
>+        <parameter name="aNode"/>
It would be more meaningful if you named this aPopup.

>+          try {
>+            if (this.mPrefs.getBoolPref("browser.tabs.tooltippreview.disable")) {
>+              return true;
>+            }
>+          } catch (e) {
>+          }
I'd prefer you to add the prefs to browser-prefs.js to avoid the try/catch (also consider renaming it .enable).

>+          label.style.width = canvasW+"px";
I'd prefer you set the width property rather than the style width. Also note that this breaks dynamically turning off the preference.

>+          aNode.style.maxWidth = label.style.width;
Unnecessary and incorrect.

>+          canvas.style.width = canvasW+"px";
>+          canvas.style.height = canvasH+"px";
>+          canvas.width = canvasW;
>+          canvas.height = canvasH;
I seem to recall that you did this to work around the non-visible area bug. Now that you're only drawing the visible area you only needs to set the properties, right?

>+          var bgColor = "#FFFFFF";
>+          try {
>+            bgColor = this.mPrefs.getCharPref("browser.display.background_color");
>+          } catch (e) {
>+          }
Again, this shouldn't need try/catch.

Setting sr- so I can see what your ideas are regarding dynamically turning off tab preview.

Comment 37

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #35)
> (From update of attachment 208181 [details] [diff] [review] [edit])
> Note that this makes the appearance preferences way too big anyway.
> 

It also really doesn't belong there - it belongs on the Navigator->Tabbed Browsing page.

Comment 38

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #36)
> Setting sr- so I can see what your ideas are regarding dynamically turning off
> tab preview.

I can't figure it out.

Comment 39

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: patch v6

Fixes review issues and now works properly as you turn the previews on and off.

Comment 40

[[e-mail]]

Comment on attachment 225187 [details] [diff] [review]
patch v6

>+            <xul:label class="tooltip-label" style="display: none;"><html:canvas/></xul:label>
Should use hidden="true" on XUL elements.

>+          var canvas = aPopup.lastChild.firstChild;
>+
>+          if (!this.mPrefs.getBoolPref("browser.tabs.tooltippreview.enable")) {
>+            canvas.parentNode.style.display="none";
Should use .hidden; also unsure whether you should use canvas.parentNode or aPopup.lastChild (in which case you could declare canvas later).

>+          canvas.parentNode.style.display="";
.hidden again.

>+      <method name="getBrowserAtIndex">
>+        <parameter name="aIndex"/>
>+        <body>
>+        <![CDATA[
>+          return this.mTabs[aIndex].linkedBrowser;
>+        ]]>
>+        </body>
>+      </method>
>+
Delete.

Comment 41

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: what I checked in

Checked in on trunk after a fix discussed with neil on IRC

Comment 42

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060625 Minefield/3.0a1 ID:2006062518 [cairo]

this was fixed/landed ?
cannot see preview.

Comment 43

[Henrik Gemal]

(In reply to comment #42)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060625
> Minefield/3.0a1 ID:2006062518 [cairo]
> 
> this was fixed/landed ?
> cannot see preview.
> 

I dont think this has landed for Minefield, correct? It's only for xpfe (seamonkey)

Comment 44

[Frederic Bezies]

I have a little problem. I built a seamonkey trunk with this patch added, and I got only a white popup box.

Is this normal or not ? 

Comment 45

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #44)
> I have a little problem. I built a seamonkey trunk with this patch added, and I
> got only a white popup box.
> 
> Is this normal or not ? 

Does you build have canvas enabled?

Comment 46

[Frederic Bezies]

Yes. It is Canvas enabled.

And here is the list of options I am using : (pasted from about:buildconfig)

--enable-application=suite 
--enable-default-toolkit=cairo-gtk2 
--disable-freetype2 --enable-pango 
--enable-svg 
--enable-canvas 
--disable-debug 
--disable-tests 
--enable-optimize='-O3 -pipe -w -march=pentium4' 
--enable-strip 
--disable-pedantic 
--enable-static 
--disable-shared

Any idea ?

And the popup is a yellow box, with the title, and in the middle of it, a white box, instead of page preview.

Comment 47

[Dave Townsend [:mossop]]

This won't work on cairo builds due to bug 324803

Comment 48

[Frederic Bezies]

Thanks for the tip. Looking at bug 324803 activity I think I won't have this tooltip fully working before next thanksgiving or so.

I will live with it ;)

Comment 49

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

jasonb pointed out on mozillazine that 300px doesn't scale with font size.  Maybe the width should be changed to some number of "em"s?  I'm not in favor of a percent of the window size.

Comment 50

[Justin Wood (:Callek)]

CTho, I'd be ok with em's and min-width specified as xyz pixels, and max-width as a % of the window.

I'm not sure how easy that would be to achieve though.

Comment 51

[Robert Kaiser]

Comment on attachment 227016 [details] [diff] [review]
what I checked in

a=me for 1.1

Comment 52

[Peter Weilbacher]

(In reply to comment #49)
> jasonb pointed out on mozillazine that 300px doesn't scale with font size. 
> Maybe the width should be changed to some number of "em"s?  I'm not in favor of
> a percent of the window size.

I don't really see what is not scaling. If I increase the font size a lot the font size in the preview also scales. With small font the font in the preview is of course unreadable but why should it be?

Comment 53

[seamonkey]

Mmmmm. Feature does not work in Linux-gtk1 builds. Does it only work with gtk2? 

Comment 54

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #53)
> Mmmmm. Feature does not work in Linux-gtk1 builds. Does it only work with gtk2? 
> 

I'm told that the mozconfig I used for the official GTK1 build results in no canvas.  This feature depends on canvas.  I don't know if you can enable canvas in GTK1 builds.

Comment 55

[TT]

I just installed Seamonkey 1.1a, and this feature is enabled by default.  Not only that, but the only way to turn it off is to disable tool tips.  Is this how it's supposed to be?

IMHO, the default should be disabled.  The pref turn turn it on/off should be independent of tool tips (since a preview is not a tool tip).  The pref should appear in the Navigator->Tabbed Browsing page.

Comment 56

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #55)
> I just installed Seamonkey 1.1a, and this feature is enabled by default.  Not
> only that, but the only way to turn it off is to disable tool tips.  Is this
> how it's supposed to be?
> 
> IMHO, the default should be disabled.  The pref turn turn it on/off should be
> independent of tool tips (since a preview is not a tool tip).  The pref should
> appear in the Navigator->Tabbed Browsing page.
> 

Use it for a week, then see if you still dislike it.  You may find it's actually useful.  You can turn them off with browser.tabs.tooltippreview.enable if you really don't like it.

Comment 57

[Anonymous]

Using 1.1b...

Tab previews are shown upside-down-mirrored in NT 4.0 SP6a, probably because the canvas (I'm assuming Cairo) ends up using a DIB (origin on the _bottom_-left, instead of top-left), instead of whatever it normally uses.


NT only has a sort-of DirectX 3, at the very latest.  I do have the "beta" DirectX 5 installed, but it's still very ancient.

Comment 58

[Anonymous]

Using 1.1b...

Tab previews are shown upside-down-mirrored in NT 4.0 SP6a, probably because the canvas (presumably through Cairo) ends up falling back to using a DIB (origin on the _bottom_-left, instead of top-left), instead of whatever it normally uses.

I do have the "beta" DirectX 5 installed, incidentally.

Comment 59

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #57)
> Using 1.1b...
> 
> Tab previews are shown upside-down-mirrored in NT 4.0 SP6a, probably because
> the canvas (I'm assuming Cairo) ends up using a DIB (origin on the
> _bottom_-left, instead of top-left), instead of whatever it normally uses.
> 
> 
> NT only has a sort-of DirectX 3, at the very latest.  I do have the "beta"
> DirectX 5 installed, but it's still very ancient.
> 

roc, vlad, comments?

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No clue sorry

Comment 61

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No idea; DirectX version is irrelevant, and cairo does the right thing as far as creating top-down DIBs vs DDBs.  The code shouldn't be any different than on XP, so it sounds like it's an NT4 bug that could be worked around, or some unsupported thing on NT4.

Comment 62

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #58)
> Using 1.1b...
> 
> Tab previews are shown upside-down-mirrored in NT 4.0 SP6a, probably because
> the canvas (presumably through Cairo) ends up falling back to using a DIB
> (origin on the _bottom_-left, instead of top-left), instead of whatever it
> normally uses.
> 
> I do have the "beta" DirectX 5 installed, incidentally.
> 

Neil reported WFM on NT4.  If you can figure out exactly what configuration causes the results you're seeing, we can consider a workaround...

Comment 63

[Anonymous]

Same results with different video (ATI Rage 128 -> NVIDIA Riva TNT2 M64, and/or VGA fallback) and with DirectX removed (for the sake of doing so).

The post at <http://blog.vlad1.com/index.php?tag=stretchdibits> leads me to wonder about using StretchBlt, instead of StretchDIBits (since BitBlt, as opposed to SetDIBits, was being used anyway).  (My apologies, if this has nothing to do with the actual production code.)

Comment 64

[Rolf Sponsel]

JUST FOR THE RECORD, I'd like to mention that I have presented an alternative, more lightweigth and faster, but still intuitive implementation for finding and navigating tabbed browser documents. This compared to the tooltip based implementation presented here, and currently implemented for SeaMonkey 1.1.

It can be found in the mozilla.support.seamonkey news group,
news://news.mozilla.org/mozilla.support.seamonkey , by searching for the thread with subject "An alternative implementation of tab preview (w/o using tooltips) - is suggested here". Or through Google Groups here:
http://groups.google.com/group/mozilla.support.seamonkey/browse_thread/thread/b3ed97a61b5ab270/adcca3e76e4b5c3b#adcca3e76e4b5c3b

Please add Your comments about the alternative in that thread - NOT here!

Comment 65

[Anonymous]

By the by... aforementioned problem with NT 4.0 (tab preview images flipped vertically) was due to it finding and using _msimg32.dll_ (GDIEXT, for GradientFill and TransparentBlt functions, included in Windows 98 and later).

I had 5.00.1693.1 from Windows 98, apparently (the 2000/XP version looks like a stub)...

Tab previews are fine, now.

Implementing a check comes to mind, though, yes, it's probably quite pointless.  It does, however, remind me of <http://toastytech.com/guis/misc351newshell2.png> (in context: Bottom of <http://toastytech.com/guis/misc4.html>), where the Opera 6 ad bar is shown flipped vertically (probably because DIBSections weren't implemented then?).


Yeah... just letting you know.  Move along, nothing to see here...