520074 - Thumbnail in left sidebar is bumped over by right sidebar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tyson Moore]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: 1.0b4

When one pulls over the right sidebar, loads a new page, and pulls over to the left sidebar, the screenshot in the left sidebar will have a white strip down the right side where the right sidebar was when the page was loaded.

Reproducible: Always

Steps to Reproduce:
1.Open Fennec (I used the OS X version 1.0b4)
2.Pull the screen to the left, displaying the right sidebar.
3.Go to a new page (preferably with a non-white background so the flaw is more visible)
4.Slide right to expose the left toolbar, showing the screenshot with a white bar where the right bar was when the page was loaded.
Actual Results:  
A white bar was rendered on the screenshot on the right side where the right toolbar was when the page was loaded.

Expected Results:  
Should have taken the screenshot of the page at full-width without the right-sidebar placeholder

MacBook Pro 3.1, 2.2GHz Intel Core 2 Duo, 2GB memory, OS X 10.5.8 (Leopard, up to date)

Comment 1

[Tyson Moore]

Attachment: Screenshot of problem.

Coincidentally on the Acid3 test, but happens on all other pages.

Comment 2

[Aakash Desai [:aakashd]]

This happens on slashdot.org on build:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091008
Fennec/1.0b4

Comment 3

[Aakash Desai [:aakashd]]

Ah, stuart this is still happening on build:

Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2b1pre) Gecko/20091023 Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091023
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091023
Fennec/1.0b5pre


You have to open the right sidebar, go to the acid 3 test. after page load, then go to the left sidebar

Comment 4

[Tyson Moore]

For me, it's not just on Acid3; it's on any webpage.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

The steps to repro has changed since now we hide the sidebars when a new page is loaded :
 * go to a page with a black background (e.g: http://vingtetun.org/todo/)
 * go to the awesome bar and load p.m.o
 * before it has finished loading pan to the left to show the right sidebar
 * wait until it finished loading and look at the tab thumbnail


I've just change one line in renderToCanvas since the only caller without a src parameter is tabs.xml.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Benjamin, if you prefer we could add use a custom parameter in tabs.xml but I'll be happy to avoid the use of Browser in the binding.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

> -      vr.x = cr.left;
> +      vr.x = 0;
>        vr.y = cr.top;
 
Should we set vr.t to zero too? Just to be safe?

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #7)
> > -      vr.x = cr.left;
> > +      vr.x = 0;
> >        vr.y = cr.top;
> 
> Should we set vr.t to zero too? Just to be safe?

Nope, otherwise the notifications are visible (or at least the size taken by the notifications)

Comment 9

[Benjamin Stover (:stechz)]

(In reply to comment #8)
> Nope, otherwise the notifications are visible (or at least the size taken by
> the notifications)

This looks to be false (Vivian and I have been testing).  Vivian is double checking, but the result matches my understanding of renderRectToCanvas.  srcRect is expected to be in viewport coordinates, where the origin is at the upper left corner of the upper left tile.  So 0, 0 should always start at the tiles and never at the notifications.

Using the critical rect is the wrong idea here because it factors in sidebars and notifications into its x and y.  If we want a thumbnail of where the viewport "camera" is located, we should use bvs's left and top coordinates.  If we just want the top of the page, use 0, 0.

I'm glad we are getting to the bottom of this, the inconsistent thumbnails have been annoying me for some time. :)

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

After seeing on IRC I've done it the wrong way! This patch should do the trick.

Comment 11

[Benjamin Stover (:stechz)]

Comment on attachment 415175 [details] [diff] [review]
Patch v0.2

I can't find any webpage where the left and top are not 0, 0 when the snapshot is taken (for instance http://people.mozilla.org/~bcombee/fennec/iframe-test.html#iframe-area should render thumbnail in the middle of page).

This was happening before and the patch fixes what it should. r+

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/85eb06d76c60

Comment 13

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; Window3sCE 5.2; en-US; rv:1.9.2b5pre) Gecko/20091201 Namoroka/3.6b5pre Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091201 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091201 Firefox/3.7a1pre Fennec/1.0b5