Closed Bug 640997 Opened 9 years ago Closed 9 years ago

Lots of checkerboarding on etsy.com shopping cart

Categories

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

ARM
Android
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: aakashd, Assigned: mbrubeck)

References

()

Details

(Keywords: regression, Whiteboard: [needs new patch])

Attachments

(3 files, 2 obsolete files)

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.
Priority: -- → P2
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.
Assignee: nobody → mbrubeck
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
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.
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).
(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)
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.
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 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.
Attachment #518879 - Flags: review+
tracking-fennec: ? → 2.0+
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out for 135ms Ts regression:
http://hg.mozilla.org/mobile-browser/rev/3869704a33c7

We could try the suggestion in comment 7 next.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This works; I have not done any perf testing.
Attachment #518944 - Flags: review?(mark.finkle)
Whiteboard: [has patch][needs review]
Comment on attachment 518944 [details] [diff] [review]
(backed out) update dimensions on location change

Looks safe. Let's test for Ts regressions.
Attachment #518944 - Flags: review?(mark.finkle) → review+
Whiteboard: [has patch][needs review] → [has patch]
Pushed: http://hg.mozilla.org/mobile-browser/rev/9b20fa85a5cb
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Backed out the second patch too.  The Ts regression was not as large, but still present:  http://hg.mozilla.org/mobile-browser/rev/e782a2ed6b7d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has patch] → [needs new patch]
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.
Attachment #518879 - Attachment description: patch: always call setCSSViewport → (backed out) always call setCSSViewport
Attachment #518944 - Attachment description: patch: update dimensions on location change → (backed out) update dimensions on location change
I'm not sure we should block on this -- are we seeing it on other sites?
tracking-fennec: 2.0+ → ?
(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
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.
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.)
Attachment #519012 - Flags: review?(mark.finkle)
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.
Attachment #519012 - Flags: review?(mark.finkle) → review+
(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.
What is the correct way to fix this bug?
(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.
This does appear to be a regression since 2011-01-19.  Bisecting to find a regression range.
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.
This fixes the remaining cases, and (unlike the backed-out patches) does not increase the number of setCSSViewport calls during startup.
Attachment #518879 - Attachment is obsolete: true
Attachment #518944 - Attachment is obsolete: true
Attachment #519172 - Flags: review?(mark.finkle)
Attachment #519172 - Flags: approval2.0?
Attachment #519012 - Flags: approval2.0?
Attachment #519012 - Flags: approval2.0? → approval2.0+
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.
Attachment #519172 - Flags: review?(mark.finkle)
Attachment #519172 - Flags: review+
Attachment #519172 - Flags: approval2.0?
Attachment #519172 - Flags: approval2.0+
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attached patch patchSplinter Review
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).
Attachment #519416 - Flags: review?(mark.finkle)
Attachment #519416 - Flags: approval2.0?
Blocks: 641855
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
Attachment #519416 - Flags: review?(mark.finkle)
Attachment #519416 - Flags: review-
Attachment #519416 - Flags: approval2.0?
Verified:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110317
Firefox/4.0b13pre Fennec/4.0b6pre
Status: RESOLVED → VERIFIED
Depends on: 642835
Depends on: 644117
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.