Closed Bug 594958 Opened 14 years ago Closed 13 years ago

Tab thumbnails sometimes have gray (now black) bars at the bottom

Categories

(Firefox Graveyard :: Panorama, defect, P4)

Product:
Firefox Graveyard
Component:
Panorama
Type:
defect

Tracking

(blocking2.0 final+)

Status:
VERIFIED FIXED
Milestone:
Firefox 4.0b12
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: smartell, Assigned: ttaubert)

References

Details

(Whiteboard: [b9][visual][good first bug][softblocker][fx4-fixed-bugday])

Attachments

(9 files, 6 obsolete files)

Screenshot
10.46 KB, image/png
Details
Another screenshot
51.12 KB, image/png
Details
Screenshot with position: fixed element
42.36 KB, image/png
Details
Yet another screenshot, but this time on Mac!
399.59 KB, image/png
Details
testcase html
131 bytes, text/html
Details
Video with Tim's WIP v1
294.67 KB, video/ogg
Details
patch for checkin
10.72 KB, patch
Details | Diff | Splinter Review
black bar at bottom of thumbnails still present in FF4beta11 for empty tabs
111.46 KB, image/png
Details
case of black bar at bottom of tab thumbnail also for tab with content - FF4-beta-11
117.94 KB, image/png
Details 
http://mozilla.seanmartell.com/tabcandy.png

As seen in the above screen grab, there are grey bars that appear at the bottom or sides when a thumbnail doesn't resize properly. The tab's grey background shows through.
Assignee: nobody → aza
Target Milestone: --- → Firefox 4.0
Priority: -- → P3
I believe this is because there is a discrepancy in the calculations for how tall the body of the page and what gets represented when we take the screenshot.
blocking2.0: --- → ?
OS: Mac OS X → All
Priority: P3 → P1
Hardware: x86 → All
Summary: Tab thumbnails are not resizing properly → Tab thumbnails sometimes have gray bars at the bottom
It's an aspect ratio issue. A bit difficult to fix since TabCandy items are all supposed to have the same AR. I'd say that we make the thumbails sized like the thumbnail + whatever is needed to accommodate the AR, then clip them back to the AR we want. That way the corrupt area is gone and we just shrink the clipped area while we're zooming.
And I'd say this is definitely BLOCKING.
Slated for b9.
Whiteboard: [b9]
Blocks: 598154
Agreed that this is important visual polish to get right. We should diagnose and size the fix sooner rather than later.
blocking2.0: ? → final+
Assignee: aza → nobody
Whiteboard: [b9] → [b9][visual][good first bug]
I don't know if it's the same bug or a similar, but the TabCandy thumbnail for [1] has a white bar at the bottom.

[1]
data:text/html,<html><head></head><body%20style="margin:0px"><div%20style="width:100%;height:100%;background-color:red;"></div></body></html>
Attached image Screenshot — 
Assignee: nobody → seanedunn

    
    

    
  
This appears to be fixed; can you reproduce it?
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME

    
    

    
  
Sorry for the late answer.

The problem I mentioned in comment 6 still exists.
Should this bug be reopened or should a new bug be filed?

    
    

    
  
(In reply to comment #9)
> Sorry for the late answer.
> 
> The problem I mentioned in comment 6 still exists.
> Should this bug be reopened or should a new bug be filed?

Andreas, if it's still there, we should reopen this bug.

It doesn't happen for me… can you tell me how you're getting it? Is it always there, or do you have to do something special? What platform are you on?

    
    

    
  
I can't reproduce the originally mentioned problem, i.e. grey bars (on new tabs, about:blank), maybe I wasn't clear enough about that.

I still can see white bars on some tab thumbnails though.
One example would be the data-URL I posted in comment 6; just copy the data URL in the location bar and press enter. You should get a completely red tab. Now switch to TabCandy. The thumbnail of the tab isn't completely red, there is a white bar at the bottom. (see attachment 483773 [details])

Another example would be error pages (about:neterror), they have a colored background (color depends on the OS), but in TabCandy there appears a white bar at the bottom of the thumbnail.

I'm on Windows XP SP3, screen resolution is 1280x1024.

    
    

    
  
Huh... I can't repro this issue with the data url or the error url; in both of them the thumbnail goes to the bottom of the tab in Panorama. I've tried on Mac and Win7.

    
    

    
  

      
      
      
      

        Attached image
          
        Another screenshot
        — 
      

    


  
That is the result I got on
Windows XP SP3
screen resolution: 1280x1024
new profile
Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre

    
    

    
  
Well, seems like a real thing... I wonder how many people it affects? XP only perhaps? 

I'm reopening, but I recommend we pull its blocking status.
Assignee: seanedunn → nobody
Status: RESOLVED → REOPENED
Priority: P1 → P4
Resolution: WORKSFORME → ---
Target Milestone: Firefox 4.0 → Future

    
    

    
  
I reported it from OSX. 

It seems to be an issue with sites that have a short height to them. Almost as if the thumbnail generated has a minimum page height that is taller than those sites causing the issue.
Whiteboard: [b9][visual][good first bug] → [b9][visual][good first bug][softblocker]

    
    

    
  
I don't know how these thumbnails are created, but they seem to try to render an area that is supposed to be invisible.
I noticed this when looking at a thumbnail of a youtube video page.
If there is an element with style="position: fixed; top: 100%" it should be invisible, but it shows up in the thumbnail.

I only tried it on Windows XP, but if you want I can also try it on Windows Vista.

What are factors that can affect the rendering of the thumbnails? 
OS? Hardware Acceleration? Other factors?

    
    

    
  


  
data:text/html,<html><head></head><body style="font-size: 200%"><div style="position: fixed; top: 50%"><a>This text should be visible!<br>style="position: fixed; top: 50%"</a></div><div style="width: 100%; background-color: orange; position: fixed; top: 100%"><a>This text should not be visible!<br>style="width: 100%; background-color: orange; position: fixed; top: 100%"</a></div></body></html>

    
    

    
  
While playing with the testcase in comment 17 I noticed that it depends on the window size if the bug is visible or not.

(Values are window.outerWidth x window.outerHeight)

Clearly visible:
1078x648

Partly visible:
709x651

Not visible:
794x953

    
    

    
  


  
Uploading an image from Mac. Sean's description is correct.

I think there's a general question about what we want to do when the screen's aspect ratio is very different from that of the tabs', which is at the root of this problem. We're too late in the game for drastic (but potentially interesting) options like making the tab thumbnails always mirror the screen in aspect. This leaves two options, I think:

1. Try to identify the bottom/background color from the contents of the tab itself, and fill some background of the thumbnails with that. (This may be the moment that we're lucky that these things are in canvas.)
2. Try to display as large a rectangle with the aspect ratio of the thumbnails as possible, while staying within the drawn area of the window. I.e., for this Bing tab, take a rect from the top left, so that the content window's height covers that of the tab thumbnail's vertical dimension.

    
    

    
  
Assigning to faaborg for ux decision.
Assignee: nobody → faaborg
Keywords: ux-feedback

    
    

    
  

      
      
      
      

        Attached file
          
        testcase html
        — 
      

    


  
[STR]
1. Start Minefield with new profile
2. Open testcase
3. Maximized browser
4. Enter Panorama

[Actual result]
 Blank rectangle area appears in thumbnail.

    
    

    
  
softblocker = critical
Severity: normal → critical

    
  
Severity: critical → normal

    
    

    
  
Are you sure that the black bars are the same issue? They are appearing only  since 0118 build.

    
    

    
  
(In reply to comment #25)
> Are you sure that the black bars are the same issue? They are appearing only 
> since 0118 build.

The landing of bug 624101 made the bars black instead of gray.
Summary: Tab thumbnails sometimes have gray bars at the bottom → Tab thumbnails sometimes have gray (now black) bars at the bottom

    
    

    
  
Faaborg, can you comment on the two options suggested in Comment 19?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1, WIP (obsolete)
        — 
          — Splinter Review
      

    


  
Mitcho's second suggestion was very easy to implement so I tried that out. This works quite well with most of the pages I had problems with. It shows some weakness for pages that are way to small (in height) - e.g. 'about:home'.

What do you think?

    
    

    
  

      
      
      
      

        Attached video
          
        Video with Tim's WIP v1
        — 
      

    


  
Tim, I think this approach was indeed a good idea. Playing with it, though, I ran into places where it go back to adding the black bars, which would get replaced over time... it must have something to do with the cached versions of the thumbnails that we're holding on to, and when we do that swapping.

    
    

    
  
Ok, so you think that this is basically two bugs? The one causing the big black/gray bars at the bottom and the other one thin bars at the right?

    
    

    
  
(In reply to comment #32)
> Ok, so you think that this is basically two bugs? The one causing the big
> black/gray bars at the bottom and the other one thin bars at the right?

Well, the black bar on the right is one issue. But that's not what I was referring to in that video... you'll see in that video that when you do a full refresh of tab thumbs, there's no bottom black bar, due to your new code. But when you resize those tab thumbs, you can get black bars once in a while, still.

    
    

    
  
Comment on attachment 505087 [details] [diff] [review]
patch v1, WIP

As per previous comments.

        Attachment #505087 -
        Flags: feedback-

    
    

    
  
(In reply to comment #33)
> Well, the black bar on the right is one issue. But that's not what I was
> referring to in that video... you'll see in that video that when you do a full
> refresh of tab thumbs, there's no bottom black bar, due to your new code. But
> when you resize those tab thumbs, you can get black bars once in a while,
> still.

Ah! That's an easy one. I forgot to modify the thumbnail generation for thumbs
< 150x150.

    
    

    
  
(In reply to comment #29)
> Created attachment 505087 [details] [diff] [review]
> patch v1, WIP
> 
> Mitcho's second suggestion was very easy to implement so I tried that out. This
> works quite well with most of the pages I had problems with. It shows some
> weakness for pages that are way to small (in height) - e.g. 'about:home'.
> 
> What do you think?

Maybe for those extreme pages, allow a little extra space at the bottom and just fill it with white?
Assignee: faaborg → tim.taubert
Blocks: 627096
No longer blocks: 598154
Target Milestone: Future → ---

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #36)
> Maybe for those extreme pages, allow a little extra space at the bottom and
> just fill it with white?

I modified the aspect ratio to be 3:1 at max. The rest is filled with the background color. This works quite well for me.

        Attachment #505087 -
        Attachment is obsolete: true

        Attachment #505917 -
        Flags: feedback?(mitcho)

    
    

    
  
Comment on attachment 505917 [details] [diff] [review]
patch v2

>-    let fromWin = this.tab.linkedBrowser.contentWindow;
>-    if (fromWin == null) {
>+    if (this.tab.linkedBrowser.contentWindow == null) {
>       Utils.log('null fromWin in paint');
>       return;
>     }

We should use assertThrow here, I believe. (You can check with Ian.) Also, change the message particularly if a search for "fromWin" in this code isn't going to actually find a reference, except in the message.

>+    let ctx = this.canvas.getContext("2d");
>     let tempCanvas = TabItems.tempCanvas;
>+
>     if (w < tempCanvas.width) {
>       // Small draw case where nearest-neighbor algorithm breaks down in Windows
>       // First draw to a larger canvas (150px wide), and then draw that image
>       // to the destination canvas.

Wait, if this is just an issue for Windows, why do we run through this code for all platforms? Should we have a platform switch here? ☆

Can you tell that I've never looked at this part of our code before? :)

(Also, is this related to bug 627838?)

>+        // the tempcanvas is square, so draw it as a square.
>+        this._fillCanvasBackground(ctx, w, h);
>+        ctx.drawImage(tempCanvas, 0, 0, w, w);

Wait wait wait, this doesn't make sense. All our canvases will have the same aspect ratio. We should use that for the tmpCanvas aspect ratio, which seems to be set in TabItems.init, and reference that h here. ☆

>+  _getWindowBackgroundColor: function () {

Is it possible that this will return something that's not a color, like "none" or null or something? If so, we should check for that in _fillCanvasBackground and use a white instead. I just don't know.

Oh, ps: all your new functions need our standard function comment header. :)

>+  _drawWindowToCanvas: function (ctx, width, height) {
>+    this._fillCanvasBackground(ctx, width, height);
>+
>+    let rect = this._calculateClippingRectangle(width, height);
>+    let scaler = width/rect.width;
>+
>+    ctx.save();
>+    ctx.scale(scaler, scaler);

So this process, in case the scaler is nontrivial, can make the zoom animation "jump" a little when transitioning between the largest flying frame and the browser itself. Please file a follow up bug, though I don't think it's noticeable often enough to warrant working on for fx4.

Also, is there a cost to this scale operation? What about not doing it if it's 1 or very close?

>+      let bgColor = this._getWindowBackgroundColor();

We already did this once in _fillCanvasBackground. Is it possible to cache or carry around this value?

>+      ctx.drawWindow(win, rect.left, rect.top, width/scaler, height/scaler,
>+                     bgColor, ctx.DRAWWINDOW_DO_NOT_FLUSH);

Do we know whether there's an advantage to (scale the canvas first, then drawWindow to that scaled size), which you have here, vs. (drawWindow to a large canvas, then scale down the canvas)? Just wondering. If we don't know, file a follow up bug to check, post-fx4.

>+  _calculateClippingRectangle: function (origWidth, origHeight) {

>+    let maxWidth = Math.max(body.clientWidth, body.scrollWidth - win.scrollX);
>+    let maxHeight = Math.max(body.clientHeight, body.scrollHeight - win.scrollY);

You want win.innerWidth and win.innerHeight. If a page's body's contents are very small, but the page is larger than that, you'll see a discrepancy and you want the inner values.

>+    if (3 < maxWidth / maxHeight)
>+      maxHeight = Math.round(maxWidth/3);
>+    else if (0.3 > maxHeight / maxWidth)
>+      maxWidth = Math.round(maxHeight*3);

This is special casing for very lopsided (very wide or very narrow) browsers, right? Please add a comment.

Adding Ian for additional feedback as I may be suggesting some crazy things (just the starred ☆ thoughts above). ;)

        Attachment #505917 -
        Flags: feedback?(mitcho)

        Attachment #505917 -
        Flags: feedback?(ian)

        Attachment #505917 -
        Flags: feedback-

    
    

    
  
(In reply to comment #38)
> Comment on attachment 505917 [details] [diff] [review]
> patch v2

Oh, ps: in general it's great! :D

    
    

    
  
(In reply to comment #38)
> >-    let fromWin = this.tab.linkedBrowser.contentWindow;
> >-    if (fromWin == null) {
> >+    if (this.tab.linkedBrowser.contentWindow == null) {
> >       Utils.log('null fromWin in paint');
> >       return;
> >     }
> 
> We should use assertThrow here, I believe. (You can check with Ian.) 

Depends on whether we want to derail the caller when things go wrong here. In this case I don't think we do, so the return is appropriate. 

> Wait, if this is just an issue for Windows, why do we run through this code for
> all platforms? Should we have a platform switch here? ☆

I wonder if it makes a difference on linux? At any rate, if there's a clean way to make it Windows only, that's cool. On the other hand, there's something to be said for having the code the same on each platform as much as possible... helps us avoid having weird issues that only show up on Windows. 

> Wait wait wait, this doesn't make sense. All our canvases will have the same
> aspect ratio. We should use that for the tmpCanvas aspect ratio, which seems to
> be set in TabItems.init, and reference that h here. ☆

I believe the idea was to give us flexibility on tab shape, but clearly we're not using it, so I suppose giving it the correct aspect ratio would be more efficient; seems like a fine idea.

        Attachment #505917 -
        Flags: feedback?(ian)

    
  
Blocks: 628402

    
  
Blocks: 628411

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #38)
> Wait, if this is just an issue for Windows, why do we run through this code for
> all platforms? Should we have a platform switch here? ☆
> 
> (Also, is this related to bug 627838?)

I rather think bug 627838 could be a bug of drawWindow()?!

> 
> >+        // the tempcanvas is square, so draw it as a square.
> >+        this._fillCanvasBackground(ctx, w, h);
> >+        ctx.drawImage(tempCanvas, 0, 0, w, w);
> 
> Wait wait wait, this doesn't make sense. All our canvases will have the same
> aspect ratio. We should use that for the tmpCanvas aspect ratio, which seems to
> be set in TabItems.init, and reference that h here.

Done.

> 
> >+  _getWindowBackgroundColor: function () {
> 
> Is it possible that this will return something that's not a color, like "none"
> or null or something? If so, we should check for that in _fillCanvasBackground
> and use a white instead. I just don't know.

This function does now try to determine the color of the <html> and <body> element. The topmost element with bgColor != 'transparent' holds the background color to be drawn. If everything fails we just take #fff.

> >+  _drawWindowToCanvas: function (ctx, width, height) {
> >+    this._fillCanvasBackground(ctx, width, height);
> >+
> >+    let rect = this._calculateClippingRectangle(width, height);
> >+    let scaler = width/rect.width;
> >+
> >+    ctx.save();
> >+    ctx.scale(scaler, scaler);
> 
> So this process, in case the scaler is nontrivial, can make the zoom animation
> "jump" a little when transitioning between the largest flying frame and the
> browser itself. Please file a follow up bug, though I don't think it's
> noticeable often enough to warrant working on for fx4.

Filed bug 628402.

> >+      let bgColor = this._getWindowBackgroundColor();
> 
> We already did this once in _fillCanvasBackground. Is it possible to cache or
> carry around this value?

Done.

> >+      ctx.drawWindow(win, rect.left, rect.top, width/scaler, height/scaler,
> >+                     bgColor, ctx.DRAWWINDOW_DO_NOT_FLUSH);
> 
> Do we know whether there's an advantage to (scale the canvas first, then
> drawWindow to that scaled size), which you have here, vs. (drawWindow to a
> large canvas, then scale down the canvas)? Just wondering. If we don't know,
> file a follow up bug to check, post-fx4.

Filed bug 628411.

> >+  _calculateClippingRectangle: function (origWidth, origHeight) {
> 
> >+    let maxWidth = Math.max(body.clientWidth, body.scrollWidth - win.scrollX);
> >+    let maxHeight = Math.max(body.clientHeight, body.scrollHeight - win.scrollY);
> 
> You want win.innerWidth and win.innerHeight. If a page's body's contents are
> very small, but the page is larger than that, you'll see a discrepancy and you
> want the inner values.

I now use document.documentElement because .clientWidth/Height and scrollX/Y are not available on 'window'. clientWidth should be the inner width omitting the scrollbar.

> >+    if (3 < maxWidth / maxHeight)
> >+      maxHeight = Math.round(maxWidth/3);
> >+    else if (0.3 > maxHeight / maxWidth)
> >+      maxWidth = Math.round(maxHeight*3);
> 
> This is special casing for very lopsided (very wide or very narrow) browsers,
> right? Please add a comment.

Done.

        Attachment #505917 -
        Attachment is obsolete: true

        Attachment #506532 -
        Flags: review?(ian)

    
    

    
  
>Faaborg, can you comment on the two options suggested in Comment 19?

yikes, this feel off my radar because of the keyword ux-feedback (for user feedback), as opposed to uiwanted (the bat symbol that causes us to show up).  Filed bug 628442 to get that renamed.

>1. Try to identify the bottom/background color from the contents of the tab
>itself, and fill some background of the thumbnails with that. (This may be the
>moment that we're lucky that these things are in canvas.)

>2. Try to display as large a rectangle with the aspect ratio of the thumbnails
>as possible, while staying within the drawn area of the window. I.e., for this
>Bing tab, take a rect from the top left, so that the content window's height
>covers that of the tab thumbnail's vertical dimension.

The best solution is probably a combination, where we zoom in to a certain threshold but then in extreme cases leverage the background color to fill areas (super narrow window etc).  My apologies for not reading the patches, if that's what we are already doing.

quick version: as Ian suggests, just use white
I can't remember the exact figures, but web pages with a white background accounted for a sizeable majority of the Web (I think it was like 80%)

    
    

    
  
(In reply to comment #42)
> The best solution is probably a combination, where we zoom in to a certain
> threshold but then in extreme cases leverage the background color to fill areas
> (super narrow window etc).  My apologies for not reading the patches, if that's
> what we are already doing.
> 
> quick version: as Ian suggests, just use white
> I can't remember the exact figures, but web pages with a white background
> accounted for a sizeable majority of the Web (I think it was like 80%)

Good choice, Alex! :) This is exactly what the patch is doing now and it also tries to determine the background color when the page is too wide or too narrow.

    
    

    
  
Passed try.

    
    

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

>+    // if we have a page that is very wide or very narrow we should correct
>+    // its max-values to ensure we have a rather normal aspect ratio
>+    if (3 < maxWidth / maxHeight)
>+      maxHeight = Math.round(maxWidth / 3);
>+    else if (0.3 > maxHeight / maxWidth)
>+      maxWidth = Math.round(maxHeight * 3);

Shouldn't the else if use "maxWidth / maxHeight" instead? Otherwise you're roughly repeating the same test as the if above it, right?

Otherwise looks good.

I don't think we have a test yet for thumbnail drawing... seems possible though; make some tabs specific html test pages (that we've created ourselves) and spot check a few pixels. What do you think?

r- for test.

        Attachment #506532 -
        Flags: review?(ian) → review-

    
    

    
  
(In reply to comment #45)
> >+    // if we have a page that is very wide or very narrow we should correct
> >+    // its max-values to ensure we have a rather normal aspect ratio
> >+    if (3 < maxWidth / maxHeight)
> >+      maxHeight = Math.round(maxWidth / 3);
> >+    else if (0.3 > maxHeight / maxWidth)
> >+      maxWidth = Math.round(maxHeight * 3);
> 
> Shouldn't the else if use "maxWidth / maxHeight" instead? Otherwise you're
> roughly repeating the same test as the if above it, right?

Oops :)

> I don't think we have a test yet for thumbnail drawing... seems possible
> though; make some tabs specific html test pages (that we've created ourselves)
> and spot check a few pixels. What do you think?

Sounds feasible, so let's do it!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #506532 -
        Attachment is obsolete: true

        Attachment #508371 -
        Flags: review?(ian)

    
    

    
  
Erm... The idea of trying to determine the background color seems a bit sketchy. And who's going to rewrite it to work in a multi-process world? :( We should try to avoid messing with content like this.

I have only skimmed this bug. Why are you not just using white?

    
    

    
  
The method used is in no way failure-proof but if it fails to determine the color it just falls back to white. Isn't this better than not trying to determine the color at all and always use white?

Another option is to calculate the biggest possible crop window (keeping the thumbnail's aspect ratio) and render that to the canvas. But this yields very ugly thumbnails for pages like google, about:home and bing that are way too short...

    
    

    
  
(In reply to comment #50)
> The method used is in no way failure-proof but if it fails to determine the
> color it just falls back to white. Isn't this better than not trying to
> determine the color at all and always use white?

It won't even know that it failed to determine the color if a page sets the dominant color in a child node of <body>. Anyway, I don't doubt that your method would get more edge cases right than just using white. But this doesn't automatically make it worthwhile.

    
    

    
  
(In reply to comment #51)
> (In reply to comment #50)
> > The method used is in no way failure-proof but if it fails to determine the
> > color it just falls back to white. Isn't this better than not trying to
> > determine the color at all and always use white?
> 
> It won't even know that it failed to determine the color if a page sets the
> dominant color in a child node of <body>. Anyway, I don't doubt that your
> method would get more edge cases right than just using white. But this doesn't
> automatically make it worthwhile.

I say, at this point in time, let's follow Dāo's advice and just use white here. As Alex said too, it's probably a safe bet on most websites. Just file a follow up for post-fx4 to do something a tad smarter... but let's get this landed asap.

(In reply to comment #50)
> Another option is to calculate the biggest possible crop window (keeping the
> thumbnail's aspect ratio) and render that to the canvas. But this yields very
> ugly thumbnails for pages like google, about:home and bing that are way too
> short...

Let's not do this.

    
    

    
  
2 comments:
1. why is the black band always equally high, it does not seem to depend on the exact page layout ?
2. why not render the band transparent i/o using a color ?

    
  

        Attachment #508371 -
        Flags: review?(ian)

    
    

    
  
(In reply to comment #53)
> 1. why is the black band always equally high, it does not seem to depend on the
> exact page layout ?

The height of the browser window is decisive when determining the height - not the height of the body contents. So every page that is not taller than the browser window (like google, about:home, about:blank, bing) have an equally-sized black bar.

> 2. why not render the band transparent i/o using a color ?

We disabled this for performance reasons (bug 624101).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #52)
> I say, at this point in time, let's follow Dāo's advice and just use white
> here. As Alex said too, it's probably a safe bet on most websites. Just file a
> follow up for post-fx4 to do something a tad smarter... but let's get this
> landed asap.

Agreed, landing this one should have priority. I removed any background color related stuff and use white for now.

> > Another option is to calculate the biggest possible crop window (keeping the
> > thumbnail's aspect ratio) and render that to the canvas. But this yields very
> > ugly thumbnails for pages like google, about:home and bing that are way too
> > short...
> 
> Let's not do this.

I noticed that I was actually still using this and was blaming the wrong part for those bad results. In fact we have pretty good results for browser windows with an aspect of 1:1, 4:3, 16:9, etc. When the browser window is way wider than tall the current patch reduces the cropping rectangle's width and thus adds some (white) space around the thumbnail. Works really good for me, you should give that a try ;)

        Attachment #508371 -
        Attachment is obsolete: true

        Attachment #508562 -
        Flags: review?(ian)

    
    

    
  
(In reply to comment #54)
> (In reply to comment #53)
> > 1. why is the black band always equally high, it does not seem to depend on the
> > exact page layout ?
> 
> The height of the browser window is decisive when determining the height - not
> the height of the body contents. So every page that is not taller than the
> browser window (like google, about:home, about:blank, bing) have an
> equally-sized black bar.
> 
 thanks Tim

I see this bar even in pages that are multiple screens high.
e.g. when I scroll all the way down on this bugzilla page I get the black band at the bottom. This band is approx 25% of the hight of the thumbnail.

my screen is 1440x900 px

Am I seeing something different than this bug ?

    
    

    
  
(In reply to comment #56)
> I see this bar even in pages that are multiple screens high.
> e.g. when I scroll all the way down on this bugzilla page I get the black band
> at the bottom. This band is approx 25% of the hight of the thumbnail.

Oh yes, of course. We also take win.scrollY into consideration when drawing the thumbnail. So we try to show you the thumbnail of what you're currently seeing in the viewport. When scrolling to the bottom there is unfortunately nothing to display anymore :)

    
    

    
  
(In reply to comment #55)
> Created attachment 508562 [details] [diff] [review]
> patch v5

Passed try.

    
    

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

Looks good

        Attachment #508562 -
        Flags: review?(ian) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v6 (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry Ian, I completely forgot to test thumbnail generation for item width < 150. Added that to the test and pushed to try again.

        Attachment #508562 -
        Attachment is obsolete: true

        Attachment #508940 -
        Flags: review?(ian)

    
    

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

Looks good.

        Attachment #508940 -
        Flags: review?(ian) → review+

    
    

    
  
(In reply to comment #60)
> Created attachment 508940 [details] [diff] [review]
> patch v6

Passed try.

    
  

        Attachment #508940 -
        Flags: approval2.0?

    
  

        Attachment #508940 -
        Flags: approval2.0?

    
  
Version: unspecified → Trunk

    
    

    
  


  

        Attachment #508940 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/54da8f75df7f
Status: REOPENED → RESOLVED
Closed: 14 years ago → 13 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12

    
  
Depends on: 625561

    
  
Depends on: 631530

    
  
Status: RESOLVED → VERIFIED
Whiteboard: [b9][visual][good first bug][softblocker] → [b9][visual][good first bug][softblocker][fx4-fixed-bugday]
No longer depends on: 631530

    
    

    
  
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

    
  
Depends on: 631530

    
    

    
  
The bug is still there (FF4-beta-11, Feb 12, 2011) but for empty tabs only. I will attach a screenshot. Here is why I know that this is the same bug as the one described here: 
1) I had empty tabs and tabs with content open. 2) I opened Panorama. 3) ALL tab thumbnails had black bars at the bottom. 4) After a fraction of a second the tabs with content got re-rendered and the black bar disappeared. The empty tab thumbnail did not get "finished" and the black bar stayed.

I hope this description and the following screenshot help.

    
    

    
  
On my computer (Windows XP SP3) the bug is still present, sometimes also for tabs with content. See following attachment. This time also the tab with content did not get re-rendered and the black bar stayed.

    
    

    
  
(In reply to comment #67)
> The bug is still there (FF4-beta-11, Feb 12, 2011) but for empty tabs only. 

I don't believe this fix made it into beta11; it should be in beta12.  Can you please test with the latest nightly? If you can reproduce on the latest nightly, feel free to reopen.

    
    

    
  
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

Was unable to reproduce issue anymore.
Product: Firefox → Firefox Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: