Closed
Bug 594958
Opened 14 years ago
Closed 14 years ago
Tab thumbnails sometimes have gray (now black) bars at the bottom
Categories
(Firefox Graveyard :: Panorama, defect, P4)
Firefox Graveyard
Panorama
Tracking
(blocking2.0 final+)
VERIFIED
FIXED
Firefox 4.0b12
| 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
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•14 years ago
|
Assignee: nobody → aza
|
||
Updated•14 years ago
|
Target Milestone: --- → Firefox 4.0
|
||
Updated•14 years ago
|
Priority: -- → P3
|
|
||
Comment 1•14 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•14 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•14 years ago
|
||
And I'd say this is definitely BLOCKING.
|
||
Comment 5•14 years ago
|
||
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•14 years ago
|
Assignee: aza → nobody
|
|
||
Updated•14 years ago
|
Whiteboard: [b9] → [b9][visual][good first bug]
|
||
Comment 6•14 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•14 years ago
|
||
|
||
Updated•14 years ago
|
Assignee: nobody → seanedunn
|
|
||
Comment 8•14 years ago
|
||
This appears to be fixed; can you reproduce it?
|
|
||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
|
|
||
Comment 9•14 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?
|
|
||
Comment 10•14 years ago
|
||
(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•14 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.
|
|
||
Comment 12•14 years ago
|
||
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•14 years ago
|
||
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
|
|
||
Comment 14•14 years ago
|
||
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•14 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.
|
|
||
Updated•14 years ago
|
Whiteboard: [b9][visual][good first bug] → [b9][visual][good first bug][softblocker]
|
|
||
Comment 16•14 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•14 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•14 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
|
|
||
Comment 19•14 years ago
|
||
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.
|
|
||
Comment 20•14 years ago
|
||
Assigning to faaborg for ux decision.
Assignee: nobody → faaborg
Keywords: ux-feedback
|
||
Comment 21•14 years ago
|
||
[STR]
1. Start Minefield with new profile
2. Open testcase
3. Maximized browser
4. Enter Panorama
[Actual result]
Blank rectangle area appears in thumbnail.
|
||
Updated•14 years ago
|
Severity: critical → normal
|
||
Comment 25•14 years ago
|
||
Are you sure that the black bars are the same issue? They are appearing only since 0118 build.
|
|
||
Comment 27•14 years ago
|
||
(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
|
|
||
Comment 28•14 years ago
|
||
Faaborg, can you comment on the two options suggested in Comment 19?
|
Assignee | |
Comment 29•14 years ago
|
||
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?
|
|
||
Comment 31•14 years ago
|
||
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.
|
|
Assignee | |
Comment 32•14 years ago
|
||
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?
|
|
||
Comment 33•14 years ago
|
||
(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 34•14 years ago
|
||
Comment on attachment 505087 [details] [diff] [review]
patch v1, WIP
As per previous comments.
Attachment #505087 -
Flags: feedback-
|
|
Assignee | |
Comment 35•14 years ago
|
||
(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.
|
|
||
Comment 36•14 years ago
|
||
(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 | |
Comment 37•14 years ago
|
||
(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 38•14 years ago
|
||
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-
|
|
||
Comment 39•14 years ago
|
||
(In reply to comment #38)
> Comment on attachment 505917 [details] [diff] [review]
> patch v2
Oh, ps: in general it's great! :D
|
|
||
Comment 40•14 years ago
|
||
(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.
|
|
||
Updated•14 years ago
|
Attachment #505917 -
Flags: feedback?(ian)
|
|
Assignee | |
Comment 41•14 years ago
|
||
(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)
|
||
Comment 42•14 years ago
|
||
>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%)
|
|
||
Updated•14 years ago
|
Keywords: ux-feedback
|
|
Assignee | |
Comment 43•14 years ago
|
||
(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.
|
|
Assignee | |
Comment 44•14 years ago
|
||
Passed try.
|
|
||
Comment 45•14 years ago
|
||
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-
|
|
Assignee | |
Comment 46•14 years ago
|
||
(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 | |
Comment 48•14 years ago
|
||
Attachment #506532 -
Attachment is obsolete: true
Attachment #508371 -
Flags: review?(ian)
|
|
||
Comment 49•14 years ago
|
||
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?
|
|
Assignee | |
Comment 50•14 years ago
|
||
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...
|
|
||
Comment 51•14 years ago
|
||
(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.
|
|
||
Comment 52•14 years ago
|
||
(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.
|
||
Comment 53•14 years ago
|
||
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•14 years ago
|
Attachment #508371 -
Flags: review?(ian)
|
|
Assignee | |
Comment 54•14 years ago
|
||
(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).
|
|
Assignee | |
Comment 55•14 years ago
|
||
(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)
|
|
||
Comment 56•14 years ago
|
||
(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 ?
|
|
Assignee | |
Comment 57•14 years ago
|
||
(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 :)
|
|
Assignee | |
Comment 58•14 years ago
|
||
(In reply to comment #55)
> Created attachment 508562 [details] [diff] [review]
> patch v5
Passed try.
|
|
||
Comment 59•14 years ago
|
||
Comment on attachment 508562 [details] [diff] [review]
patch v5
Looks good
Attachment #508562 -
Flags: review?(ian) → review+
|
|
Assignee | |
Comment 60•14 years ago
|
||
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 61•14 years ago
|
||
Comment on attachment 508940 [details] [diff] [review]
patch v6
Looks good.
Attachment #508940 -
Flags: review?(ian) → review+
|
|
Assignee | |
Comment 63•14 years ago
|
||
(In reply to comment #60)
> Created attachment 508940 [details] [diff] [review]
> patch v6
Passed try.
|
|
Assignee | |
Updated•14 years ago
|
Attachment #508940 -
Flags: approval2.0?
|
|
Assignee | |
Updated•14 years ago
|
Attachment #508940 -
Flags: approval2.0?
|
|
Assignee | |
Updated•14 years ago
|
Version: unspecified → Trunk
|
|
Assignee | |
Comment 64•14 years ago
|
||
Attachment #508940 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•14 years ago
|
Keywords: checkin-needed
|
||
Comment 65•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Status: RESOLVED → VERIFIED
Whiteboard: [b9][visual][good first bug][softblocker] → [b9][visual][good first bug][softblocker][fx4-fixed-bugday]
|
||
Comment 66•14 years ago
|
||
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
|
||
Comment 67•14 years ago
|
||
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.
|
|
||
Comment 68•14 years ago
|
||
related to comment 67
|
|
||
Comment 69•14 years ago
|
||
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.
|
|
||
Comment 70•14 years ago
|
||
related to comment 69
|
||
Comment 71•14 years ago
|
||
(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•14 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre
Was unable to reproduce issue anymore.
|
||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•