747493 - Craiglist Forums Unusable / pages with framesets not fully visible

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Damon Sicore (:damons)]

Fennec Native 14.01a, Nightly (2012-04-20)

STR: Visit https://forums.craigslist.org/?forumID=76&areaID=1 to get some support because you are getting a divorce.

Expected results:

Be able to view the entire page, be able to log in, be able to search forums, be able to scroll around.

Actual results:

Galaxy S2:  Only the left frame message tree is visible, no way to zoom out to get the login and search functionality.

Galaxy Nexus: Both frames are visible, but half of the search text boxes are cut off, and you can't zoom out to view the entire page.  Also, the login links in the upper right hand side are inaccessible.

Comment 1

[Damon Sicore (:damons)]

Works perfectly on Chrome on Android.

Comment 2

[Damon Sicore (:damons)]

Attachment: video of the damage

Comment 3

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

I guess bug 746681 might be a dup of this bug then?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

this doesn't look like font inflation, can we try turning it off and seeing if it still reproduces?

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

You should see a green, yellow and a red block with this testcase

This seems to work in the latest Aurora Fennec Native build 13.0a2 2012-04-23 (if you don't switch orientation at least).

I guess this might be a regression from the Maple branch landing.

Comment 6

[Catalin Suciu [:csuciu]]

This issue doesn't occur on:
Build ID : Nightly (2012-03-14)20120314031139
http://hg.mozilla.org/mozilla-central/rev/c71845b3b2a6

but it occurs on:
Build ID : Nightly (2012-03-15)20120315031154
http://hg.mozilla.org/mozilla-central/rev/d0d13f09be44

Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c71845b3b2a6&tochange=d0d13f09be44

Comment 7

[Damon Sicore (:damons)]

Looks like that range includes a 351 change set merge from maple for OMTC.  Should we also look back in the maple branch pre-merge and narrow a range there?

Comment 8

[Martijn Wargers (dead)]

This might have just not worked on the Maple branch.
Benoit, Ali, is there anything QA can do more here?

Comment 9

[Ali Juma [:ajuma]]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #8)
> This might have just not worked on the Maple branch.
> Benoit, Ali, is there anything QA can do more here?

It'd be helpful to check if this works in the earliest usable Maple branch build, and if so, to find a regression range there.

Comment 10

[Jet Villegas (inactive)]

To Mats for a look.

Comment 11

[Catalin Suciu [:csuciu]]

This is what i found on Maple:

good build:
Maple (2012-03-21)20120221040247
http://hg.mozilla.org/projects/maple/rev/d644bee23551

bad build:
Maple (2012-03-22)20120222040208
http://hg.mozilla.org/projects/maple/rev/b520f34d78f1

Possible range:
http://hg.mozilla.org/projects/maple/pushloghtml?fromchange=d644bee23551&tochange=b520f34d78f1

Comment 12

[JP Rosevear [:jpr]]

Adding in people who might be guilty based on regression range.  Pretty sure this is not readability.

Comment 13

[(no longer active)]

This looks like one of the viewport changes that we did to me.  Somebody needs to bisect this range, but finding the regression range may not be very useful, as I think this code has changed a lot since then.

kats showed me a bug in rendering overflow:hidden body elements yesterday which looked a bit like this broken-ness.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

So I tracked this down and the problem, in a nutshell is this:

When the page body has overflow:hidden set on it, the mContentSize in the FrameMetrics that is received by CompositorParent.cpp contains the clipped page size rather than the full page size. This sort of makes sense, because that represents the size of the layer AIUI, and that's how big the layer is. However, the compositor also needs to know how big the page is without the overflow clipping, so that it can tell Java and let Java update the page size stored there.

The way I would like to fix this is by adding a couple of fields to the FrameMetrics that represent the actual page size without overflow-clipping, and use those in CompositorParent. However I don't know enough about the layout code to know where I would get the relevant values to populate the fields in FrameMetrics.

Note also that the issue from bug 746120 affects this page, and any other frameset/overflow:hidden pages.

Comment 17

[(no longer active)]

I'm not sure why you need the unclipped page dimensions anywhere.  Shouldn't Java just use the clipped size?  Or am I missing someting?

Comment 18

[Kartikaya Gupta (email:[e-mail])]

On my test page, when we first render the page, it has:

CSS viewport as 980x1413
page size as 980x1413
screen size as 720x1038
resolution as 0.7347

If the page does NOT have overflow:hidden, the mContentSize comes out as 720x1038, which gets sent to Java as the page size in device pixels. Java sees that the page is scaled appropriately to fit on the device screen, and all is good.

If the page DOES have overflow:hidden, the mContentSize comes out as 529x763, which gets sent to Java as the page size in device pixels. Java sees the page is too small for the device screen (which is 720x1038) and bumps the resolution to 1.0 so that it "fits". This causes the problems with part of the page being clipped out.

It seems to me that if a page is such that it fits on the screen, then overflow:hidden should basically have no effect. Which means that my test page (which has no significant content inside the body) should have the same values being propagated everywhere with or without overflow:hidden. The first point at which I observe a difference is this mContentSize variable, so I assume that is where the problem lies.

Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an overflow:hidden, with everything else being the same?

Comment 19

[(no longer active)]

I talked to kats about it.  It seems to me that mContentSize is computed correctly for the case where we don't have a scroll frame, but maybe the caller is using it to mean the wrong thing.  It's not really clear what's causing the problem at this point.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Simpler testcase

Comment 21

[Timothy Nikkel (:tnikkel)]

Frameset documents don't have a root scroll frame. That could break assumptions.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

I still need an answer to this question:

> Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an
> overflow:hidden, with everything else being the same?

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (:kats) from comment #22)
> I still need an answer to this question:
> 
> > Why does mContentSize report a size of 529x763 instead of 720x1038 when there is an
> > overflow:hidden, with everything else being the same?

The top level document is a frameset? The testcase Mats posted and the URL for this bug point to a top level document being a frameset. If so then that document won't have overflow:hidden, it has no scrollframes or any of that. So it's one of the child documents that has overflow:hidden? If so we would expect mContentSize to be smaller than the whole screen since that isn't taking up the whole screen, only a part of it.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #24)
> The top level document is a frameset? The testcase Mats posted and the URL
> for this bug point to a top level document being a frameset. If so then that
> document won't have overflow:hidden, it has no scrollframes or any of that.

Sorry, I should have mentioned: the page I was testing on was the simplified test case in bug 748526, located at http://people.mozilla.org/~kgupta/bug/748526.html

However, I believe the same issue happens on framesets because the html.css file sets a default overflow on framesets of -moz-hidden-unscrollable, see http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#686

Comment 26

[Timothy Nikkel (:tnikkel)]

Frameset documents are quite different from all other documents. They don't have scroll frames at all. In normal documents overflow:hidden would generate a scroll frame. What makes you think that your overflow:hidden testcase is having the same issue as a frameset document?

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Are you sure that overflow:hidden documents generate a scroll frame? When I stick a breakpoint at [1] I see the scrollableFrame coming out as null. Or is that chunk of code not relevant? The same page without overflow:hidden does have a scrollableFrame at that point. In fact that if statement is where the different mContentSize comes from (what I was talking about in comment 18).

As far as I can tell, frameset documents and overflow:hidden documents both do NOT generate a scroll frame, and both have a reported mContentSize that somewhere along the way doesn't take into account the resolution. This results in similar user-visible behaviour on both types of pages, namely the page appears slightly zoomed-in and cropped.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#233

Comment 28

[Timothy Nikkel (:tnikkel)]

overflow:hidden documents definitely do get a scroll frame. But there are a few steps between having a scroll frame and getting to the code you pointed out where things could be different, and that could be the cause of the bug.

The bug for framesets and overflow:hidden might be fixed by the same change, or might require two fixes.

I haven't been able to look into this bug other than reading the comments here so far.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Updating title for now since I've been using this bug to track both frameset and overflow:hidden page-truncation. It may be that they are separate issues, as tn says, in which case we can split out another bug if needed.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

:jrmuizel and I looked at this a bit further today. It looks like there are two viewport frames - one for the top-level window and one for the content window. The content window correctly has a CSS width of 980 pixels (which is what we set the CSS viewport width to). The outer viewport frame however has a CSS width of 720 pixels when the page has overflow:hidden, and that's what the compositor is picking up as the page size. Setting the CSS viewport on the top-level window didn't seem to affect anything; we believe that the outer viewport frame is sizing itself based on the NS_RESIZE event that is dispatched (i.e. using window.outerWidth) and that this may or may not be correct.

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Interestingly RecordFrameMetrics records different numbers 720 vs. 980 depending on whether we're taking a screenshot or not.

Comment 36

[Timothy Nikkel (:tnikkel)]

The screenshot path doesn't ask to reuse the widget layers so it should build a fresh set of layers, meaning that the frame metrics recorded aren't used for anything I would think.

Comment 39

[Kartikaya Gupta (email:[e-mail])]

This bug is still assigned to me, but I think the ball is in layout's court right now. I don't know what the correct fix for this bug would be.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

The patch on bug 753742 seems to fix the overflow:hidden cases but not the frameset cases. Once that patch lands, I'll undupe the overflow:hidden bugs from this bug and dupe them to bug 753742 instead. Apologies in advance for the resulting bugspam.

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: wip

This creates a scroll layer for frameset documents.

After this the width is drawn right, but there are some weird artifacts going on.

Comment 46

[Timothy Nikkel (:tnikkel)]

The artifacts are pretty severe, sometimes not drawing the right half of the page at all.

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Last update on my side: I was able to reproduce the issue tn was seeing on a droid RAZR but not the Galaxy Nexus. The reason I think stuff wasn't painting with his patch is because it is getting clipped out; at that point in the frame tree the clip rect is the size of the screen and so anything that has CSS coordinates outside that area doesn't get drawn.

IRC conversation extract:

17:13:01 kats: tn: this looks very similar to the clipping problem i was seeing in bug 757362. In nsSubdocumentFrame, the size of the dirtyRect is the size of the screen rather than the size of the CSS viewport. (I think). And so anything that has CSS coordinates that exceed your screen pixel size will be clipped out and not drawn
17:14:11 kats: tn: on the droid razr the screen width is 540x960 pixels so anything that's outside that in CSS coordinates gets clipped out
17:14:35 kats: tn: the galaxy nexus I was testing with originally has 720 width so that right frame doesn't get clipped out and draws fine
17:15:14 kats: tn: that being said i don't know what the correct fix is
17:15:30 tn: kats, that makes total sense, let me see if i can fix that
17:15:33 tn: kats, thanks

Comment 48

[Timothy Nikkel (:tnikkel)]

The fix is to change the dirty region to be the display port at the right place, just need to figure out what the right place is exactly.

Comment 49

[Timothy Nikkel (:tnikkel)]

There are still rendering errors after doing that, but they are less severe.

Comment 50

[Jet Villegas (inactive)]

tn: Can you post up the latest WIP patch and screenshots of the glitches? We may take this fix anyway, depending on the severity of the rendering issues.

Comment 51

[Timothy Nikkel (:tnikkel)]

The patch here is still the latest, the issue from comment 47 is a separate issue, so I'll post that to another bug.

I think we can probably land this patch (once it's cleaned up for lading) and worry about the rendering errors after.

Comment 52

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 53

[Mats Palmgren (inactive)]

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

> layout/base/nsDisplayList.cpp
>+nsDisplaySimpleScrollLayer::BuildLayer(nsDisplayListBuilder* aBuilder,
> ...
>+  if (content) {
>+    usingDisplayport = nsLayoutUtils::GetDisplayPort(content, &displayport);
>+  }

I don't think the null-check is necessary, 'nsLayoutUtils::FindIDFor(content)'
would have already crashed if 'content' was null.

(same in nsDisplayScrollLayer::BuildLayer)

+  nsRect viewport = mFrame->GetRect() -
+                    mFrame->GetPosition() +
+                    aBuilder->ToReferenceFrame(mFrame);

You can write the right hand side more efficiently as:
  nsRect(aBuilder->ToReferenceFrame(mFrame), mFrame->GetSize());

(same in nsDisplayScrollLayer::BuildLayer)



> layout/generic/nsSubDocumentFrame.cpp

Your code looks fine - perhaps add a comment why we create a layer
for frameset with a display port.

>  bool addedLayer = false;
>
>   if (subdocRootFrame && parentAPD != subdocAPD) {
>     NS_WARN_IF_FALSE(!addedLayer,
>                      "Two container layers have been added. "
>                       "Performance may suffer.");

Testing 'addedLayer' doesn't seem very useful... perhaps the warning was
left there as documentation though.

Actually, isn't that what we're doing?
(nsDisplaySimpleScrollLayer::BuildLayer creates a container layer)
Is performance OK if you zoom?

BTW, did you consider the order?  right now you may have:
nsDisplayZoom
  nsDisplaySimpleScrollLayer

ie, nsDisplaySimpleScrollLayer's layer may not be the root container layer.

I don't know much about layers though, so someone else should look too.

r=mats, with that reservation

Comment 54

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #53)
> I don't think the null-check is necessary,
> 'nsLayoutUtils::FindIDFor(content)'
> would have already crashed if 'content' was null.

Ok.

> You can write the right hand side more efficiently as:
>   nsRect(aBuilder->ToReferenceFrame(mFrame), mFrame->GetSize());

Ok.

> > layout/generic/nsSubDocumentFrame.cpp
> 
> Your code looks fine - perhaps add a comment why we create a layer
> for frameset with a display port.

Ok.

> Testing 'addedLayer' doesn't seem very useful... perhaps the warning was
> left there as documentation though.

The code has been moved around a few times since that was created I think. I can look to rationalize it in a different patch.

> Actually, isn't that what we're doing?
> (nsDisplaySimpleScrollLayer::BuildLayer creates a container layer)
> Is performance OK if you zoom?

Yeah, container layers don't kill performance. It's okay to create one or two or more if we need them, but we don't want to create one that we don't need.

> BTW, did you consider the order?  right now you may have:
> nsDisplayZoom
>   nsDisplaySimpleScrollLayer
> 
> ie, nsDisplaySimpleScrollLayer's layer may not be the root container layer.

I did consider that. I think that will be fine due to how the compositor looks for the scrollable layer (breadth first search to find the first scroll layer). And I think it is also the correct way, the scroll layer will be in the child document's app units, so it should be inside the zoom item/layer. Zoom isn't currently used on mobile, so it mostly doesn't matter though.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Review of attachment 631532 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +2259,5 @@
> +  ViewID scrollId = nsLayoutUtils::FindIDFor(content);
> +
> +  nsRect viewport = mFrame->GetRect() -
> +                    mFrame->GetPosition() +
> +                    aBuilder->ToReferenceFrame(mFrame);

ToReferenceFrame()

@@ +2284,5 @@
> +    // The visible region for the children may be much bigger than the hole we
> +    // are viewing the children from, so that the compositor process has enough
> +    // content to asynchronously pan while content is being refreshed.
> +
> +    nsRegion childVisibleRegion = displayport + aBuilder->ToReferenceFrame(mFrame);

ToReferenceFrame()

::: layout/base/nsDisplayList.h
@@ +2016,5 @@
>  };
>  
>  /**
> + * A display item that has no purpose but to ensure its contents get
> + * their own layer.

... and that FrameMetrics are recorded, if the frame's document's root element has a displayport?

Comment 56

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #55)
> ToReferenceFrame()

Ok.

> ToReferenceFrame()

Ok.

> ... and that FrameMetrics are recorded, if the frame's document's root
> element has a displayport?

Ah yes, forget to fix this up.

Comment 57

[Timothy Nikkel (:tnikkel)]

Attachment: patch

With review comments addressed.

Comment 58

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 59

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18b206c111cd

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18b206c111cd

Comment 61

[Timothy Nikkel (:tnikkel)]

Frameset documents still need work after this bug. Bug 757362, comment 11 applies to them as well, and bug 763570.

Comment 62

[Jet Villegas (inactive)]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: lost data when framesets are used on a page
Testing completed (on m-c, etc.): on mozilla-central for less than 1 day
Risk to taking this patch (and alternatives if risky):  moderate (patch affects frameset docs only)
String or UUID changes made by this patch: none

Comment 63

[Kevin Brosnan [Ex-Mozilla]]

This looks good on today's trunk build (2012-06-10). Checked duplicate bugs all seem to be resolved or no longer reproducing. Google reader is giving us a mobile site now (no framesets there afik).

Comment 64

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b923ee207e0

Comment 65

[Timothy Nikkel (:tnikkel)]

Back out for simple compile error due to difference between beta and central
https://hg.mozilla.org/releases/mozilla-beta/rev/2a03198e95a1

Comment 66

[Timothy Nikkel (:tnikkel)]

Landed with that fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/0d744168d314

Comment 67

[Timothy Nikkel (:tnikkel)]

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

We have this on central and beta right now. Doesn't that imply we automatically want it on aurora?

Comment 68

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4223ec8761b3

Comment 69

[Kartikaya Gupta (email:[e-mail])]

Note for posterity: this patch was backed out on m-c as part of bug 763570 as we had a fix that was cleaner and fixed other fallout as well. Leaving this bug closed because bug 763570 doesn't break this behaviour, just fixes it differently.