640997 - Lots of checkerboarding on etsy.com shopping cart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre Fennec/4.0b6pre

NOte: I think this has something to do with the bug 624454

Steps to Reproduce:
1. Go to www.etsy.com
2. Tap on any of the items on the front page
3. Tap on "Add to cart"

Actual Results:
The shopping cart page will have lots of checkerboarding on the bottom of the page

Expected Results:
There should be any checkerboarding on the page.

Comment 1

[Matt Brubeck (:mbrubeck)]

I can reproduce this too.  Just to clarify: This isn't checkerboard that appears during panning and then disappears; this is checkerboard that is always present because the document is not as tall as the browser.  Another case of this was previously fixed in bug 614353.

Comment 2

[Aakash Desai [:aakashd]]

Other scenarios with this issue:

Twitter.com
1. Go to www.twitter.com for their desktop site

Linkedin.com
1. Go to www.linkedin.com
2. Sign In
3. On the home page, click on "Add an Application" button on the bottom right part of the page.
4. Click on the "GitHub" option

Comment 3

[Matt Brubeck (:mbrubeck)]

On desktop, values in updateViewportSize before the Math.max call added in bug 614353:

    viewportH = 1333.3333333333335
    viewportW = 980
    screenH = 800
    screenW = 480

After the Math.max call:

    viewportH: 1650

That ought to be enough to fill the entire height of the screen, but I still see checkerboard over the bottom 50% or so.

Resizing the window (e.g. switching orientation) fixes the problem, but reloading the page makes it appear again.

Comment 4

[Matt Brubeck (:mbrubeck)]

This is happening when moving from one page to another with the same calculated viewport width and height.  We don't try to set the viewport size again, because Fennec thinks it is already set to the correct value.  It looks as though something is changing the viewport size behind our backs.  If I comment out the "if (browser.contentWindowWidth != viewportW ..." at the end of updateViewportSize, then this bug is fixed (probably with a perf penalty).

Comment 5

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

(In reply to comment #4)
> This is happening when moving from one page to another with the same calculated
> viewport width and height.  We don't try to set the viewport size again,
> because Fennec thinks it is already set to the correct value.  It looks as
> though something is changing the viewport size behind our backs.  If I comment
> out the "if (browser.contentWindowWidth != viewportW ..." at the end of
> updateViewportSize, then this bug is fixed (probably with a perf penalty).

I have added this check while looking to find a Ts regression on the n900 if I remember correctly. Adding it was without effect, so I think removing it won't hurt much (for reference this was the followup in bug 627293)

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: (backed out) always call setCSSViewport

It would also be good to test Tp with this patch.

I'd like to understand the bug better though, to see if there's a better solution.

Comment 7

[Wesley Johnston (:wesj)]

Alternatively, you could reset contentDocumentWidth and Height on Content:LocationChange (pushing innerWidth and height from the child to the parent). But I agree it would be nice to know where they are being reset in the first place.

Comment 8

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

Comment on attachment 518879 [details] [diff] [review]
(backed out) always call setCSSViewport

If we can't find the underlying explanation before RC spin on Monday, we should take this patch. In fact, we should land it well ahead of the RC spin so we can see the Tp, Ts affects.

Comment 9

[Matt Brubeck (:mbrubeck)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/907bc2950d31

I'll watch Talos over the weekend, and continue to investigate to see what the "right" fix is.

Comment 10

[Matt Brubeck (:mbrubeck)]

Backed out for 135ms Ts regression:
http://hg.mozilla.org/mobile-browser/rev/3869704a33c7

We could try the suggestion in comment 7 next.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: (backed out) update dimensions on location change

This works; I have not done any perf testing.

Comment 12

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

Comment on attachment 518944 [details] [diff] [review]
(backed out) update dimensions on location change

Looks safe. Let's test for Ts regressions.

Comment 13

[Matt Brubeck (:mbrubeck)]

Pushed: http://hg.mozilla.org/mobile-browser/rev/9b20fa85a5cb

Comment 14

[Matt Brubeck (:mbrubeck)]

Backed out the second patch too.  The Ts regression was not as large, but still present:  http://hg.mozilla.org/mobile-browser/rev/e782a2ed6b7d

Comment 15

[Matt Brubeck (:mbrubeck)]

Let's talk about the root cause of this bug, since papering over it seems to come with too much performance penalty.

The intent of bug 596969 is that, once we have called setCSSViewport, the viewport size should remain the same and ignore any changes.  Clearly something is still changing it, though, since we observe innerHeight/innerWidth changes.  (These should be the same as the size passed to setCSSViewport, because of bug 602580.)

If we can get the viewport to stop changing except when we call setCSSViewport, then we win.

Comment 16

[Stuart Parmenter]

I'm not sure we should block on this -- are we seeing it on other sites?

Comment 17

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

(In reply to comment #16)
> I'm not sure we should block on this -- are we seeing it on other sites?

I see it on twitter.com

Comment 18

[Matt Brubeck (:mbrubeck)]

I've seen it on just a few other sites - it can happen on any page that's shorter than it is tall (but it depends on the navigation path you take to that page).  Last night while browsing, I noticed it on this page:
http://snorp.net/2011/02/10/android-is-linux.html

I agree that this shouldn't be a hardblocker - I'd rather ship with this bug than take a risky patch.

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: handle "autosize" pages that are too wide/short

Actually the bug on snorp.net is slightly different, and is not fixed by the other patches here.  snorp.net sets width=device-width ("autoSize" in the code), but its content ends up wider than the viewport.  We currently adjust the viewport height only for non-autoSize pages.

This patch just applies the same fix to autoSize pages.  This fixes the problem on snorp.net (and probably some other pages), but not on etsy or twitter.  This one should not have any Ts impact.  (Unlike the others, this does not increase the number of resizes.)

Comment 20

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

Comment on attachment 519012 [details] [diff] [review]
handle "autosize" pages that are too wide/short

I assume this does not break any existing tests and do we have tests for this now? If not, I'd like to add some.

Comment 21

[Matt Brubeck (:mbrubeck)]

(In reply to comment #20)
> I assume this does not break any existing tests and do we have tests for this
> now? If not, I'd like to add some.

The patch doesn't break any tests (though I'm getting some new timeouts in our browser-chrome tests, apparently unrelated).  I can add a test for it tomorrow.

Comment 22

[Stuart Parmenter]

What is the correct way to fix this bug?

Comment 23

[Matt Brubeck (:mbrubeck)]

(In reply to comment #22)
> What is the correct way to fix this bug?

There are two causes for the bug - one on "width=device-width" sites like snorp.net, where the correct fix is the patch in attachment 519012 [details] [diff] [review].

There's a different cause for other sites.  For that, the ideal fix is to prevent the viewport from resizing once we have overridden it.  I'm not sure if this is a regression or not.  My next step in this bug is to do test past nightlies to see if it is a regression since bug 614353 was fixed, and find a regression range if possible.

Comment 24

[Matt Brubeck (:mbrubeck)]

This does appear to be a regression since 2011-01-19.  Bisecting to find a regression range.

Comment 25

[Matt Brubeck (:mbrubeck)]

Last good nightly is 2011-02-11, first bad is 2011-02-12.  Range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8013e7a30984&tochange=95feb77222f2

http://hg.mozilla.org/mobile-browser/pushloghtml?startdate=2011-02-11&enddate=2011-02-12

Comment 26

[Matt Brubeck (:mbrubeck)]

The first bad changeset is http://hg.mozilla.org/mobile-browser/rev/c0f28968a38e (bug 631038).

Comment 27

[Matt Brubeck (:mbrubeck)]

So, it looks like the underlying issue with viewports changing was there all along, but it was previously masked by bug 631038.  We can't just revert that change, since it fixes a much worse bug than this one.

As a last try at papering over the problem, we might try a patch like attachment 518944 [details] [diff] [review] but using the pagehide message instead of the LocationChange message.  I'll test this locally to see if it avoids the problem with extra resizes during startup.

If that doesn't work, I suggest that we wait until .next for a "real" fix, rather than trying to fix the underlying behavior in layout/graphics/whatever for this release.

Comment 28

[Matt Brubeck (:mbrubeck)]

Attachment: update dimensions on pagehide

This fixes the remaining cases, and (unlike the backed-out patches) does not increase the number of setCSSViewport calls during startup.

Comment 29

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

Comment on attachment 519172 [details] [diff] [review]
update dimensions on pagehide

This makes no sense to me. The "pagehide" event should never affect the new page being loaded.

But I understand that this bug is a regression from the removal of such code from a "pagehide" handler. This patch _fixes_ the current problem and does not add a regression to Ts, but likely does regression Tp - we'll see.

I'm afraid we'll have to live with some pref regression because the rendering issue out-weighs the current pref regression size.

We definitely need to file a followup bug to fix the root cause of this issue.

Comment 30

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mobile-browser/rev/3d3966df201b
http://hg.mozilla.org/mobile-browser/rev/2ee8a79c06e7

I'm working on tests today.  I'll attach those as a separate patch, and file a followup bug on the root cause.

Comment 31

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This fixes an apparently harmless error message ("aMessage.json is undefined") when navigating from one local tab to another.  This is a minimally risky fix aimed at 4.0, which does not touch any code except what was added for this bug.

I'll file a post-4.0 followup to fix the underlying issue (we should remove the message listeners that are causing extra calls to this method in local tabs).

Comment 32

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

Comment on attachment 519416 [details] [diff] [review]
patch

Since this only appears to happen during browser-chrome test and it seems to be harmless, I don't think we should even try to patch it for 4.0

Let's just get the better fix in for post-4.0

Comment 33

[Ayan Shah[:ashah]]

Verified:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110317
Firefox/4.0b13pre Fennec/4.0b6pre