Last Comment Bug 368046 - Tab preview again: some bugs and fixes
: Tab preview again: some bugs and fixes
Status: NEW
: polish
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-24 06:03 PST by chAlx
Modified: 2012-06-04 19:33 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
tab preview: zoom in on narrow window (31.14 KB, image/png)
2007-01-24 06:07 PST, chAlx
no flags Details
tab preview: empty on zero-height window (17.60 KB, image/png)
2007-01-24 06:09 PST, chAlx
no flags Details
tab preview: small image (15.79 KB, image/png)
2007-01-24 06:10 PST, chAlx
no flags Details
tab preview: resized image (22.40 KB, image/png)
2007-01-24 06:11 PST, chAlx
no flags Details
tab preview: zoom in on narrow window [FIXED] (25.47 KB, image/png)
2007-01-24 06:16 PST, chAlx
no flags Details
tab preview: empty on zero-height window [FIXED] (27.54 KB, image/png)
2007-01-24 06:17 PST, chAlx
no flags Details
tab preview: small image [FIXED] (15.79 KB, image/png)
2007-01-24 06:19 PST, chAlx
no flags Details
tab preview: small image [FIXED] (15.04 KB, image/png)
2007-01-24 06:21 PST, chAlx
no flags Details
tab preview: resized image [FIXED] (21.18 KB, image/png)
2007-01-24 06:23 PST, chAlx
no flags Details
tab preview: no content [FIXED] (12.12 KB, image/png)
2007-01-24 06:24 PST, chAlx
no flags Details
doPreview replacement for tabbrowser.xml (6.06 KB, application/vnd.mozilla.xul+xml)
2007-01-24 06:40 PST, chAlx
no flags Details
make 0px-tall window act almost like 1px-tall window (1.73 KB, patch)
2007-01-24 17:35 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch) (1.65 KB, patch)
2007-01-24 17:36 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
neil: superreview+
kairo: approval‑seamonkey1.1.1+
Details | Diff | Review
doPreview replacement for tabbrowser.xml [PATCH format] (8.12 KB, patch)
2007-01-25 00:30 PST, chAlx
no flags Details | Diff | Review

Description chAlx 2007-01-24 06:03:38 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1
Build Identifier: Mozilla/5.0 (en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1 (checked with 1.5a too)

There are a lot of issues with current tab preview implementation. Here are some of them documented and the refactored code to fix most of them and add some features. I don't exactly know how to write/document Mozilla code but I believe my style is easy to understand and will help others to maintain the code.

Here are most of differences (see attached pics):

* Empty page has original simple tooltip instead of empty square.
* Small window not zoomed in; small content has 1:1 preview.
* Resized to zero-height window has normal preview instead of empty square.
* Image is always previewed in a whole independing of zoom/scroll.
* Non-proportional content has smaller preview canvas without unused white space.
* Large document is [probably] correctly resized and scrolled using screen dimensions as max preview area.
* Max preview height added (and tooltippreview.width is used as max width).
* May be a little more..

The main idea was:
 * To preview the data, not the window.
 * To output only the data, not empty space.


Reproducible: Always
Comment 1 chAlx 2007-01-24 06:07:41 PST
Created attachment 252596 [details]
tab preview: zoom in on narrow window
Comment 2 chAlx 2007-01-24 06:09:36 PST
Created attachment 252597 [details]
tab preview: empty on zero-height window
Comment 3 chAlx 2007-01-24 06:10:47 PST
Created attachment 252598 [details]
tab preview: small image
Comment 4 chAlx 2007-01-24 06:11:35 PST
Created attachment 252599 [details]
tab preview: resized image
Comment 5 chAlx 2007-01-24 06:14:15 PST
And some screenshots with fixed tabbrowser.xml:
Comment 6 chAlx 2007-01-24 06:16:28 PST
Created attachment 252600 [details]
tab preview: zoom in on narrow window [FIXED]

Using fixed tabbrowser.xml
Comment 7 chAlx 2007-01-24 06:17:59 PST
Created attachment 252601 [details]
tab preview: empty on zero-height window [FIXED]

Using fixed tabbrowser.xml
Comment 8 chAlx 2007-01-24 06:19:21 PST
Created attachment 252602 [details]
tab preview: small image [FIXED]

Using fixed tabbrowser.xml
Comment 9 chAlx 2007-01-24 06:21:30 PST
Created attachment 252603 [details]
tab preview: small image [FIXED]

Using fixed tabbrowser.xml
Comment 10 chAlx 2007-01-24 06:23:39 PST
Created attachment 252604 [details]
tab preview: resized image [FIXED]

Using fixed tabbrowser.xml
Comment 11 chAlx 2007-01-24 06:24:59 PST
Created attachment 252606 [details]
tab preview: no content [FIXED]

Using fixed tabbrowser.xml
Comment 12 chAlx 2007-01-24 06:34:34 PST
I'm sorry for skipping reproduce steps but screenshots are rather obviously.
Ones with [FIXED] in description belongs to fixed tabbrowser.xml with new
version of doPreview method.
Comment 13 chAlx 2007-01-24 06:36:58 PST
chrome/toolkit.jar: 
content/global/bindings/tabbrowser.xml:
line 200 or so:

      <!-- Tab Preview handler: -->
      <method name="doPreview">
        <parameter name="aPopup"/>
        <body>
        <![CDATA[
          var tab = document.tooltipNode;
          if (tab.localName != "tab") {
            return false;
          }
          var b = tab.linkedBrowser;
          if (!b) {
            return false;
          }

          var label = aPopup.firstChild;
          label.setAttribute("value", tab.getAttribute("label"));

          var canvas = aPopup.lastChild.firstChild;
          canvas.parentNode.hidden = true;

          var errorFound = false; // Flag to rerurn with regular tooltip

          if ((tab == this.mCurrentTab) ||
              !this.mPrefs.getBoolPref("browser.tabs.tooltippreview.enable")) {
            errorFound = true;
          }

          if (!errorFound) {

            var ctx;
            try {
              ctx = canvas.getContext("2d");
            } catch (e) {
              errorFound = true;
            }
          } // if !errorFound

          if (!errorFound) {
            label.width = 0;
            aPopup.setAttribute("tabpreview", "true");
            var win = b.contentWindow; // document.tooltipNode.linkedBrowser.contentWindow

            var MaxPreviewWidth = this.mPrefs.getIntPref("browser.tabs.tooltippreview.width");
            var MaxPreviewHeight = 300; //TODO: this.mPrefs.getIntPref("browser.tabs.tooltippreview.height");

            var bgColor = this.mPrefs.getCharPref("browser.display.background_color");

            var winWidth = 0; // Previewed Content size
            var winHeight = 0;

            // check for ImageDocument:
            var isImageDocument = false;
            if (b.contentDocument instanceof ImageDocument) {
              if(b.contentDocument.imageRequest.imageStatus & Components.interfaces.imgIRequest.STATUS_ERROR) {
                // Error in image -- return simple tooltip:
                errorFound = true;
              }
              else {
                var img = b.contentDocument.body.firstChild; // HTMLImageElement
                // Preview images using original size without scroll/zoom:
                winWidth = img.naturalWidth;
                winHeight = img.naturalHeight;
                isImageDocument = true;
              }
            } // is ImageDocument
            else { // not ImageDocument:
              var contentDoc = win.top.document;

              var winMaxWidth = win.screen.availWidth;
              var winMaxHeight = win.screen.availHeight;
              var previewScrollX = 0;
              var previewScrollY = 0;

              if (win.scrollMaxX > 0) { // Scrolling found
                winWidth = win.innerWidth + win.scrollMaxX; // Full content+space size
                if (winWidth > winMaxWidth) {
                  previewScrollX = win.pageXOffset;
                  // Check for overscrolling (due to increased preview area):
                  if (previewScrollX > winWidth - winMaxWidth) {
                    previewScrollX = winWidth - winMaxWidth;
                  }
                  winWidth = winMaxWidth; // Set desktop size for preview area
                }
              } // Horizontal scrolling
              else {
                winWidth = contentDoc.width; // Skip unused window space
              }

              if (win.scrollMaxY > 0) { // Vertical scrolling
                winHeight = win.innerHeight + win.scrollMaxY;
                if (winHeight > winMaxHeight) {
                  previewScrollY = win.pageYOffset;
                  if (previewScrollY > winHeight - winMaxHeight) {
                    previewScrollY = winHeight - winMaxHeight;
                  }
                  winHeight = winMaxHeight;
                }
              } // Vertical scrolling
              else {
                winHeight = contentDoc.height;
              }

            } // not ImageDocument
          } // if !errorFound

          if (!errorFound) {
            if((winWidth < 1) || (winHeight < 1)) {
              // Error in preview src -- return simple tooltip:
              errorFound = true;
            }
          } // if !errorFound

          if (!errorFound) {
            // Calculate canvas dimensions:
            var canvasW = winWidth; // Preassign original size
            var canvasH = winHeight;

            // Avoid zoom-in and overheight issues:
            if (canvasW > MaxPreviewWidth) { // Width is too large:
              canvasW = MaxPreviewWidth;
              canvasH = Math.round(winHeight * MaxPreviewWidth / winWidth);
            }
            if (canvasH > MaxPreviewHeight) { // Original or calculated-by-width height is too large:
              canvasW = Math.round(winWidth * (MaxPreviewHeight / winHeight));
              canvasH = MaxPreviewHeight;
            }

            // Check errors (zero calculated size) and return long tooltip:
            if(canvasW < 1 || canvasH < 1) { // Error in preview calculations -- return simple tooltip:
              errorFound = true;
            }
          } // if !errorFound

          if (errorFound) { // Return simple tooltip on error:
            label.removeAttribute("width");
            aPopup.removeAttribute("tabpreview");
            return true;
          } // else:

          // Apply calculated dimensions:
          canvas.width = canvasW;
          canvas.height = canvasH;
          canvas.parentNode.hidden = false;

          if (isImageDocument) {
            // Draw calculated image:
            ctx.fillStyle = bgColor;
            ctx.fillRect(0, 0, canvasW, canvasH);
            ctx.drawImage(img, 0, 0, canvasW, canvasH);
          } // is ImageDocument
          else { // not ImageDocument:
            ctx.save();
            ctx.scale(canvasW / winWidth, canvasH / winHeight);
            ctx.drawWindow(win, previewScrollX, previewScrollY, winWidth, winHeight, bgColor);
            ctx.restore();
          } // not ImageDocument

          return true;
        ]]>
        </body>
      </method>
Comment 14 chAlx 2007-01-24 06:40:53 PST
Created attachment 252611 [details]
doPreview replacement for tabbrowser.xml

chrome/toolkit.jar
content/global/bindings/tabbrowser.xml
line 200 or so
Comment 15 chAlx 2007-01-24 07:41:59 PST
Known issues (With some original script is affected too):

* Some sites (most with frames) are appears to be empty: document.tooltipNode.linkedBrowser.contentWindow has no vital params such as innerWidth.
Example: http://www.thetiny.net

* Very small content (such as 5x2px) will generate unusable small preview. Possible solution: add minWidth and minHeight for preview to config.

* Small preview has truncated page title -- maybe label and preview should be 2 different windows?

* Shrinked pages are really looks ugly (bug 352412) because of weak resize algorithm. I still can't find this code to fix it..
Comment 16 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-24 16:19:41 PST
Can you please attach that in the form of a diff so I can see what you actually changed?
Comment 17 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-24 17:14:45 PST
For a zero-height window, I believe we should disable the preview.  The only reason that doesn't currently happen is that the code divides by zero and doesn't zero out the canvas height (I think it's getting set to "NaN" based on the error in the JS console).  Displaying the whole document doesn't really make sense to me; the choice of scaling there is somewhat arbitrary.

I also think having a maximum height is reasonable; what it should be, I don't know.  Just restricting it to width or 2*width certainly wouldn't work (if I have a narrow window because I'm reading a web page with instructions for some other app that also needs to fit on screen, the tooltip would legitimately be fairly tall).

For small images, if we shrink the preview, you truncate the tooltip's text.  Consider an image in a background tab when your tabs are narrow - if you were hovering to get the tooltip text, you wouldn't actually be able to read it.

> * Image is always previewed in a whole independing of zoom/scroll.

Doesn't that happen with the current code?  It was a design decision to always preview the full image - look for the "instanceof ImageDocument" check in the code.

> * Non-proportional content has smaller preview canvas without unused white
space.

This was intentional.  I think I didn't like seeing the tooltips sometimes sized differently, but I forget if there was another reason.

I'd be ok with centering small images, and drawing them against a clear color rather than a white background, but other than that...

Regarding blank documents, what's blank?  Is a page with white text on a white background blank?  Is <body bgcolor="red"></body> blank?  There's no reasonable way to detect blank.
Comment 18 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-24 17:35:12 PST
Created attachment 252713 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window

Neil, *if* we want to show something for zero-height for some reason, what proportions do we assume?

Centered images don't look good against a white background... I think with Classic (or any native-like theme), finding the tooltip's background color would be a problem.
Comment 19 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-24 17:36:49 PST
Created attachment 252714 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

Oops.  Fixed.
Comment 20 chAlx 2007-01-24 23:38:25 PST
> Can you please attach that in the form of a diff so I can see what you actually
> changed?

Sure.. But I've decided to make the whole code rewritten (attach id=252611) after some tries to patch original code. There were too many things to add and the code wasn't "ready" to.

I'll make the diff if it is the way to attract someone's attention ;) but it doesn't seems to be handy.
Comment 21 chAlx 2007-01-25 00:30:27 PST
Created attachment 252752 [details] [diff] [review]
doPreview replacement for tabbrowser.xml [PATCH format]

diff with tabbrowser.xml (2007-01-21 9:36:58 from Seamonkey 1.5a)

For full patched code see source of https://bugzilla.mozilla.org/attachment.cgi?id=252611
Comment 22 chAlx 2007-01-25 02:10:22 PST
> For a zero-height window, I believe we should disable the preview.

But windows are resized with useful content -- why to disable preview this content?

> For small images, if we shrink the preview, you truncate the tooltip's text.

For small image we usually have full picture in preview -- tooltip text is redundant.
NB: Tooltip text for image is URL, it's usually longer than 300px and truncated anyway.

> It was a design decision to always preview the full image

Yes, but there were some issues (errors were treated as document; canvasH depends on win.innerHeight). And I've just made this feature to work not implying it wasn't ;)

> I'd be ok with centering small images

But this full-sized tooltip overlaps some interface and content data -- for what?

> There's no reasonable way to detect blank.

I don't think so. My way is simple: content height (win.innerHeight) must be positive. Others are backgrounds and may be some CSS layers -- not much to worry (but good to think) about.

Anyway blank pages detection seems to be the good config option.


Neil, Chris and all:

I think we should talk about special situations (such as zero-height windows, shrunk preview, blank pages etc) after some basic rules defined. My version of rules:

* Output only the data, not empty space (scroll preview area or shrink tooltip if needed; return to normal tooltip if no data).
* Output max amount of data for (maxW x maxH) tooltip (ignore window size if more data is available).
* Preview the full image; don't make document previewed area more than desktop size.
Comment 23 neil@parkwaycc.co.uk 2007-01-29 06:05:32 PST
Comment on attachment 252714 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

>+          if (!this.mPrefs.getBoolPref("browser.tabs.tooltippreview.enable")
>+              || tab == this.mCurrentTab
>+              || h == 0) {
You made the same mistakes as you did in bug 367461. r+sr=me with that fixed.
Comment 24 Felix Miata 2007-01-29 13:02:31 PST
Lots of activity for an unconfirmed bug. :-p
Comment 25 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-29 16:13:59 PST
Comment on attachment 252714 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

Checked in my patch for zero-height windows.
Comment 26 Ian Neal 2007-02-13 15:51:11 PST
Comment on attachment 252714 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

a=me for SM1.1.1 (one more needed)
Comment 27 Robert Kaiser (not working on stability any more) 2007-02-14 05:50:37 PST
Comment on attachment 252714 [details] [diff] [review]
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

a=me for 1.1.1
Comment 28 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-02-14 17:12:01 PST
(In reply to comment #27)
> (From update of attachment 252714 [details] [diff] [review])
> a=me for 1.1.1
> 

KaiRo, I checked it in on branch.
Comment 29 chAlx 2007-02-21 03:36:29 PST
Just checked v1.1.1 (20070217). What can I say? Too sad that great Mozilla developers can't review or rewrite hundred lines of simple script.

Look: there are ERRORS, not just not-enhanced-features. Narrow windows still has zoomed-in and overheighted preview (attachment 252596 [details]). Empty tab has empty tooltip on both active and inactive tabs (attachment 252597 [details]). Even current tab has empty text tooltip (open the bug and one more tab and play switching tabs with mouse: current tab tooltips are "randomly" replaced with empty square). Division by zero is still available in all variables with division operator (never seen images or pages with zero width?).
Comment 30 Robert Kaiser (not working on stability any more) 2008-02-26 12:06:05 PST
anything left to do here for trunk?
Comment 31 Volkmar Kostka 2008-07-29 03:10:41 PDT
Does this bug also include that for some pages only an empty preview is shown?
For me e.g. http://www.oxymoronical.com/web/firefox/nightly shows as an empty preview.
Comment 32 Volkmar Kostka 2008-07-29 03:14:52 PDT
ADD: The previews are empty if i have not visited them before. If i visit them using CTRL-TAB i need to switch two times before they are shown as preview.
Comment 33 Robert Kaiser (not working on stability any more) 2009-05-10 09:09:35 PDT
Neil, there's a patch here that apparently fell through the cracks as it has no review request. Could you look into this and tell if this is still something we want?
If so, we should get it reviewed and into the tree, else we should close this as fixed due to the patch that actually was done and checked in here.

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