753742 - Content at hotmail.com is overlapped when panning

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: screenshot from Firefox Beta

Nightly/15.0a1 (2012-05-09) / Firefox Native 14.0b1 build 2
Device: HTC Desire
OS: Android 2.2.2

Steps to reproduce:
1. Go to Hotmail.com and Sign In with a valid user account. Use the account provided in the Note section if needed.
2. From the top menu choose "Hotmail" to view your inbox.
3. Pan around the page.

Expected results:
Content remains correctly displayed.

Actual results:
Content is overlapped with other parts of the page which seem to be low-resolution screenshots - please see the attached screenshot.
On Nightly 15.0a1 2012-05-09 there are white areas which are white covering the content which may be due to Bug 752910 - low res checkerboard is broken. 

Notes:
Test account credentials:
user: [e-mail]
password: firsttest

Comment 1

[Adrian Tamas (:AdrianT)]

Attachment: logs

Comment 2

[Aaron Train [:aaronmt]]

This looks like some incorrect invalidation or painting issue. Thanks for the test-account.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Still happens on nightly 5/10/2012 multilocale.  Samsung Galaxy S II

Comment 4

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

This looks exactly like bug 751690 which should be fixed. Can we re-test the STR in bug 751690 too and see if the patches just didn't fix the bug?

Comment 5

[Adrian Tamas (:AdrianT)]

This is still an issue on Nightly 15.0a1 2012-05-14. Tested on HTC Desire Z running Android 2.3.3.

Comment 6

[Chris Lord [:cwiiis]]

This is producing a similar effect for me as bug 749741 and bug 750211 - it kind of looks like we're getting the viewport coordinates of the rendered document wrong somehow...

As you scroll, when it re-renders, it appears as though the re-rendered content has scrolled twice the distance it should - You can see this by just scrolling the list of mails slowly downwards (moving slowly allows it to re-render quickly enough for async scrolling to not be too visible), and seeing how the position under your finger moves.

Interestingly, events are still delivered to the right area of the page.

Comment 7

[Chris Lord [:cwiiis]]

Had a look via the inspector, this site is actually a full-screen iframe.

What could be happening here is that the root scrollable frame code is finding the iframe, but the code we use to actually scroll will be trying to scroll the document body node.

In fact, I'm pretty certain this is what's happening - we're scrolling the iframe fine - but the document isn't scrolling because it can't... The compositor picks up the iframe and thinks it did scroll, when in fact, it didn't, so we end up with the scroll applied twice - once from the iframe scrolling, then again by the async scroll code. Or something along these lines at least.

Somehow, we need to have GetPrimaryScrollableLayer not be tricked by a scrollable iframe occupying an entire unscrollable document. I'll know more when this build finishes...

Comment 8

[Chris Lord [:cwiiis]]

So I was right, but the fix isn't so simple - Ideally, we could just use FrameMetrics::IsRootScrollable instead of IsScrollable in CompositorParent::GetPrimaryScrollableLayer, but because our 'root scrollable' is not at the root of frame tree, it doesn't get marked as such.

Perhaps we want to change this, or perhaps we could add another ID for nodes with a display-port set, which we could use to fetch the layer representing our 'root scrollable'. I'm going to have a shot at doing the latter, as it sounds easier to me.

Ccing roc (sorry!) for comment, as this is related to layout.

Comment 9

[Chris Lord [:cwiiis]]

Oh, of course, FrameMetrics already has the display-port set, so presumably we could just look for the first layer with a display-port set? Trying this out.

Comment 10

[Chris Lord [:cwiiis]]

Well, that does get us the same behaviour as looking for the first scrollable layer (and is perhaps more reliably what we want?), but this bug still manifests.

I'll start looking at the dumped display list and generated layers, but I'm starting to think that the iframe layer is being optimised into the root layer... Or that might just be nonsense. Will report back.

Comment 11

[Timothy Nikkel (:tnikkel)]

We could just change the meaning of the root scroll ID to be for the root content document scroll frame. This matches was e10s Fennec does, and get us what we want for Native Fennec. I don't think it would require much code change.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #10)
> I'll start looking at the dumped display list and generated layers, but I'm
> starting to think that the iframe layer is being optimised into the root
> layer... Or that might just be nonsense. Will report back.

If the parent document (the one that contains the iframe) is overflow hidden then this code http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2278 means we won't create a scroll layer display item and consequently won't create a scroll layer.

Comment 13

[Chris Lord [:cwiiis]]

(In reply to Timothy Nikkel (:tn) from comment #11)
> We could just change the meaning of the root scroll ID to be for the root
> content document scroll frame. This matches was e10s Fennec does, and get us
> what we want for Native Fennec. I don't think it would require much code
> change.

This sounds good to me.

(In reply to Timothy Nikkel (:tn) from comment #12)
> (In reply to Chris Lord [:cwiiis] from comment #10)
> > I'll start looking at the dumped display list and generated layers, but I'm
> > starting to think that the iframe layer is being optimised into the root
> > layer... Or that might just be nonsense. Will report back.
> 
> If the parent document (the one that contains the iframe) is overflow hidden
> then this code
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp#2278 means we won't create a scroll layer display item
> and consequently won't create a scroll layer.

Thanks, this has enabled me to boil it down into a simpler test-case: http://chrislord.net/files/mozilla/iframe-test.html - you need to zoom in a bit after it loads, but after doing so, scrolling down exhibits the same behaviour as hotmail.

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Timothy Nikkel (:tn) from comment #12)
> (In reply to Chris Lord [:cwiiis] from comment #10)
> If the parent document (the one that contains the iframe) is overflow hidden
> then this code
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/
> nsGfxScrollFrame.cpp#2278 means we won't create a scroll layer display item
> and consequently won't create a scroll layer.

So one thing that obviously fixes this is to force layer building on any scroll frame that has a display-port set. Would this be acceptable?

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Chris Lord [:cwiiis] from comment #14)
> So one thing that obviously fixes this is to force layer building on any
> scroll frame that has a display-port set. Would this be acceptable?

I would say forcing a scroll layer on the root scroll frame (mIsRoot) of the root content document (mOuter->PresContext()->IsRootContentDocument()) would work.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Force layer building on root scroll frames of root content documents

Patch does as suggested in comment #15.

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 624103 [details] [diff] [review]
Force layer building on root scroll frames of root content documents

Oh, I forgot one thing, we don't want to create scroll layers on desktop. I think this will create scroll layers on desktop.

Can we just ignore the overflow hidden check if we are usingDisplayport (ie always create a scroll layer if we are usingDisplayport)? I think we can.

Comment 20

[Chris Lord [:cwiiis]]

Attachment: Force layer building on scroll frames with displayports

Force layer building on scroll-frames with displayports, as suggested in comment #14 and comment #19.

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 624334 [details] [diff] [review]
Force layer building on scroll frames with displayports

This looks like the same patch. Wrong file or no qref or something?

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Force layer building on scroll frames with displayports

Upload the correct patch (sorry, VM fail)

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 624689 [details] [diff] [review]
Force layer building on scroll frames with displayports

Change the comment to say something like "so CompositorParent can always find the scrollable layer for the root content document".

Comment 26

[Chris Lord [:cwiiis]]

Pushed to inbound with fixed comment: http://hg.mozilla.org/integration/mozilla-inbound/rev/7d409147c4fd

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d409147c4fd

Comment 29

[Adrian Tamas (:AdrianT)]

Panning hotmail no longer displays the content overlapped on Nightly 15.0a1 2012-05-18 however long HTML emails ( Newsletters for e.g) are cut off at the edge of the viewport. Bug 756453 - Long HTML emails are cut off at the bottom at hotmail.com - covers the new issue.

Verified fix on:
Build: Nightly 15.0a1 2012-05-17
Device: HTC Desire Z
OS: Android 2.3.3

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Given the number of duplicates, should this have some sort of test?

Comment 36

[Alfredos-Panagiotis Damkalis [:fredy]]

Verify that this bug is fixed on nightly (v15 - 18 May).

The bug still affects beta (v14.0b2)

Tested on sgs2 cyanogenmod 9 (ics)

Tested url: Hotmail.com after login

Comment 37

[Chris Lord [:cwiiis]]

Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/2a3a10b7fc25

Comment 38

[Scoobidiver (away)]

The Nightly version where it landed is 15.

Comment 39

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 624689 [details] [diff] [review]
Force layer building on scroll frames with displayports

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Hotmail and other sites will show the bug described.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 40

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This bug may show the first time w/ Adreno devices when landing on hotmail, it will correct itself after some time (several seconds) and does not crash.  Probably best to log as a separate bug as it seems only to affect adreno devices.

On LG Revolution, it will show a caught exception:
05-22 08:52:42.895: W/System.err(9221): java.lang.IndexOutOfBoundsException: setSpan (0 ... 11340) ends beyond length 22
05-22 08:52:42.895: W/System.err(9221): 	at android.text.SpannableStringBuilder.checkRange(SpannableStringBuilder.java:948)
05-22 08:52:42.895: W/System.err(9221): 	at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:527)
05-22 08:52:42.905: W/System.err(9221): 	at android.text.SpannableStringBuilder.setSpan(SpannableStringBuilder.java:519)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoInputConnection.setEditable(GeckoInputConnection.java:1042)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoInputConnection.notifyTextChange(GeckoInputConnection.java:457)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoInputConnection.notifyIMEChange(GeckoInputConnection.java:978)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoAppShell.notifyIMEChange(GeckoAppShell.java:527)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoAppShell.nativeRun(Native Method)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:472)
05-22 08:52:42.905: W/System.err(9221): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:82)

On HTC Desire HD:
05-22 08:23:27.131: E/GeckoConsole(4187): [JavaScript Error: "NS_ERROR_INVALID_POINTER: Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIEventListenerService.getListenerInfoFor]" {file: "chrome://browser/content/browser.js" line: 3041}]
05-22 08:23:27.161: D/PowerManagerService(1377): New lightsensor value:160, lcdValue:163
05-22 08:23:27.161: D/PowerManagerService(1377): lightSensorChangedLocked, buttonValue >= 0, mPowerState = 3
05-22 08:23:27.732: E/GeckoConsole(4187): [JavaScript Warning: "Empty string passed to getElementById()." {file: "http://js.wlxrs.com/mxFacnptZSBDQMbNrEZZ7w/wlive.js" line: 1}]
05-22 08:23:27.982: E/GeckoConsole(4187): [JavaScript Error: "[Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIEventListenerService.getListenerInfoFor]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://browser/content/browser.js :: _hasMouseListener :: line 3041"  data: no]" {file: "chrome://browser/content/browser.js" line: 2666}]

Comment 41

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Created bug 757472; leaving this bug closed/verified.

Comment 42

[Alex Keybl [:akeybl]]

Comment on attachment 624689 [details] [diff] [review]
Force layer building on scroll frames with displayports

[Triage Comment]
It's not clear why 13 would need a FN fix since we're not shipping the product off that version.

Comment 45

[Catalin Suciu [:csuciu]]

Verified fixed on Aurora 14.0a2 (2012-05-24)
                  Beta 14.0b3
Samsung Galaxy SII (2.3.4)