Open Bug 368046 Opened 18 years ago Updated 12 years ago

Tab preview again: some bugs and fixes

Categories

(SeaMonkey :: General, defect)

Product:
SeaMonkey
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

People

(Reporter: chalx, Unassigned)

Details

(Keywords: polish)

Attachments

(12 files, 2 obsolete files)

tab preview: zoom in on narrow window
31.14 KB, image/png
Details
tab preview: empty on zero-height window
17.60 KB, image/png
Details
tab preview: small image
15.79 KB, image/png
Details
tab preview: resized image
22.40 KB, image/png
Details
tab preview: zoom in on narrow window [FIXED]
25.47 KB, image/png
Details
tab preview: empty on zero-height window [FIXED]
27.54 KB, image/png
Details
tab preview: small image [FIXED]
15.04 KB, image/png
Details
tab preview: resized image [FIXED]
21.18 KB, image/png
Details
tab preview: no content [FIXED]
12.12 KB, image/png
Details
doPreview replacement for tabbrowser.xml
6.06 KB, application/vnd.mozilla.xul+xml
Details
make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)
1.65 KB, patch
neil
: review+
neil
: superreview+
kairo
: approval-seamonkey1.1.1+
Details | Diff | Splinter Review
doPreview replacement for tabbrowser.xml [PATCH format]
8.12 KB, patch
Details | Diff | Splinter Review 
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

    
    

    
  


  

    
    

    
  


  

    
    

    
  


  

    
    

    
  
And some screenshots with fixed tabbrowser.xml:

    
    

    
  


  
Using fixed tabbrowser.xml

    
    

    
  


  
Using fixed tabbrowser.xml

    
    

    
  

      
      
      
      

        Attached image
          
        tab preview: small image [FIXED] (obsolete)
        — 
      

    


  
Using fixed tabbrowser.xml

    
    

    
  


  
Using fixed tabbrowser.xml

        Attachment #252602 -
        Attachment is obsolete: true

    
    

    
  


  
Using fixed tabbrowser.xml

    
    

    
  


  
Using fixed tabbrowser.xml

    
    

    
  
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.

    
    

    
  
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>


    
    

    
  


  
chrome/toolkit.jar
content/global/bindings/tabbrowser.xml
line 200 or so

    
    

    
  
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..

Component: General → XP Apps: GUI Features

    
    

    
  
Can you please attach that in the form of a diff so I can see what you actually changed?
Component: XP Apps: GUI Features → General

    
    

    
  
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.

    
    

    
  


  
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.

        Attachment #252713 -
        Flags: superreview?(neil)

        Attachment #252713 -
        Flags: review?(neil)

    
    

    
  


  
Oops.  Fixed.

        Attachment #252713 -
        Attachment is obsolete: true

        Attachment #252714 -
        Flags: superreview?(neil)

        Attachment #252714 -
        Flags: review?(neil)

        Attachment #252713 -
        Flags: superreview?(neil)

        Attachment #252713 -
        Flags: review?(neil)

    
    

    
  
> 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.

    
    

    
  


  
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

    
    

    
  
> 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 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.

        Attachment #252714 -
        Flags: superreview?(neil)

        Attachment #252714 -
        Flags: superreview+

        Attachment #252714 -
        Flags: review?(neil)

        Attachment #252714 -
        Flags: review+

    
    

    
  
Lots of activity for an unconfirmed bug. :-p

    
    

    
  
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.

        Attachment #252714 -
        Attachment description: make 0px-tall window act almost like 1px-tall window → make 0px-tall window act almost like 1px-tall window (checked in on trunk)

        Attachment #252714 -
        Flags: approval-seamonkey1.1.1?

    
    

    
  
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 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

        Attachment #252714 -
        Flags: approval-seamonkey1.1.1? → approval-seamonkey1.1.1+

        Attachment #252714 -
        Attachment description: make 0px-tall window act almost like 1px-tall window (checked in on trunk) → make 0px-tall window act almost like 1px-tall window (checked in on trunk and branch)

    
    

    
  
(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.

    
  
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
    

    
  
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?).

    
    

    
  
anything left to do here for trunk?
Version: unspecified → Trunk

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.
Keywords: polish

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: