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

VERIFIED FIXED in Firefox 4.0b12

Status

defect
P4
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: smartell, Assigned: ttaubert)

Tracking

Trunk
Firefox 4.0b12
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

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

Attachments

(9 attachments, 6 obsolete attachments)

10.46 KB, image/png
Details
51.12 KB, image/png
Details
42.36 KB, image/png
Details
399.59 KB, image/png
Details
131 bytes, text/html
Details
294.67 KB, video/ogg
Details
10.72 KB, patch
Details | Diff | Splinter Review
111.46 KB, image/png
Details
117.94 KB, image/png
Details
(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Assignee: nobody → aza

Updated

9 years ago
Target Milestone: --- → Firefox 4.0
Priority: -- → P3

Comment 1

9 years ago
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

Comment 2

9 years ago
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.

Comment 3

9 years ago
And I'd say this is definitely BLOCKING.

Comment 4

9 years ago
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+

Updated

9 years ago
Assignee: aza → nobody
Whiteboard: [b9] → [b9][visual][good first bug]

Comment 6

9 years ago
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>

Comment 7

9 years ago
Posted image Screenshot
Assignee: nobody → seanedunn
This appears to be fixed; can you reproduce it?
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WORKSFORME

Comment 9

8 years ago
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?

Comment 11

8 years ago
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.

Comment 13

8 years ago
Posted 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
(Reporter)

Comment 15

8 years ago
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]

Comment 16

8 years ago
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?

Comment 17

8 years ago
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>

Comment 18

8 years ago
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

Comment 21

8 years ago
Posted 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
Duplicate of this bug: 626804

Updated

8 years ago
Duplicate of this bug: 626945
Are you sure that the black bars are the same issue? They are appearing only  since 0118 build.
Duplicate of this bug: 626945
(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?
Posted 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?
Duplicate of this bug: 627109
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 → ---
Posted 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)
(Assignee)

Updated

8 years ago
Blocks: 628402
(Assignee)

Updated

8 years ago
Blocks: 628411
Posted 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!
(Assignee)

Updated

8 years ago
Duplicate of this bug: 629758
Posted 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 ?
(Assignee)

Updated

8 years ago
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).
Posted 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+
Posted 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+
Duplicate of this bug: 630808
(In reply to comment #60)
> Created attachment 508940 [details] [diff] [review]
> patch v6

Passed try.
(Assignee)

Updated

8 years ago
Attachment #508940 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Attachment #508940 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Version: unspecified → Trunk
Attachment #508940 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Comment 65

8 years ago
http://hg.mozilla.org/mozilla-central/rev/54da8f75df7f
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 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.

Comment 72

8 years ago
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.