Craiglist Forums Unusable / pages with framesets not fully visible

VERIFIED FIXED in Firefox 16

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
11 months ago

People

(Reporter: damons, Assigned: tnikkel)

Tracking

unspecified
Firefox 16
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-fennec1.0 +)

Details

(Whiteboard: [layout], URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
blocking-fennec1.0: --- → ?
(Reporter)

Updated

5 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
(Reporter)

Comment 1

5 years ago
Works perfectly on Chrome on Android.
blocking-fennec1.0: ? → +
Whiteboard: [readability]
(Reporter)

Comment 2

5 years ago
Created attachment 617070 [details]
video of the damage
I guess bug 746681 might be a dup of this bug then?
this doesn't look like font inflation, can we try turning it off and seeing if it still reproduces?
Keywords: qawanted

Updated

5 years ago
Created attachment 617506 [details]
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.

Updated

5 years ago
Keywords: regressionwindow-wanted
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
Keywords: qawanted, regressionwindow-wanted
(Reporter)

Comment 7

5 years ago
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?
This might have just not worked on the Maple branch.
Benoit, Ali, is there anything QA can do more here?

Comment 9

5 years ago
(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.

Updated

5 years ago
Keywords: regressionwindow-wanted

Comment 10

5 years ago
To Mats for a look.
Assignee: nobody → matspal
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
Keywords: regressionwindow-wanted

Comment 12

5 years ago
Adding in people who might be guilty based on regression range.  Pretty sure this is not readability.
Whiteboard: [readability]
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.
Duplicate of this bug: 735652
Duplicate of this bug: 748526
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.
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?
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?
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.

Updated

5 years ago
Assignee: matspal → bugmail.mozilla
Created attachment 619202 [details]
Simpler testcase
(Assignee)

Comment 21

5 years ago
Frameset documents don't have a root scroll frame. That could break assumptions.
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?
Duplicate of this bug: 743681
(Assignee)

Comment 24

5 years ago
(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.
(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
(Assignee)

Comment 26

5 years ago
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?
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
(Assignee)

Comment 28

5 years ago
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.

Updated

5 years ago
Blocks: 750896
Duplicate of this bug: 752575
Summary: Craiglist Forums Unusable → Craiglist Forums Unusable / pages with framesets not fully visible
Duplicate of this bug: 753014
Duplicate of this bug: 753026
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.
Summary: Craiglist Forums Unusable / pages with framesets not fully visible → Craiglist Forums Unusable / pages with framesets or overflow:hidden not fully visible
Duplicate of this bug: 753351
: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.
Interestingly RecordFrameMetrics records different numbers 720 vs. 980 depending on whether we're taking a screenshot or not.
(Assignee)

Comment 36

5 years ago
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.
Duplicate of this bug: 753069
Duplicate of this bug: 754144
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.
(Assignee)

Updated

5 years ago
Assignee: bugmail.mozilla → tnikkel
Duplicate of this bug: 754927
Duplicate of this bug: 754833
Duplicate of this bug: 755092
Duplicate of this bug: 755580
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.
Summary: Craiglist Forums Unusable / pages with framesets or overflow:hidden not fully visible → Craiglist Forums Unusable / pages with framesets not fully visible

Updated

5 years ago
Whiteboard: [layout]
(Assignee)

Comment 45

5 years ago
Created attachment 628129 [details] [diff] [review]
wip

This creates a scroll layer for frameset documents.

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

Comment 46

5 years ago
The artifacts are pretty severe, sometimes not drawing the right half of the page at all.
Blocks: 762427
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
(Assignee)

Comment 48

5 years ago
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.
(Assignee)

Comment 49

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

Comment 50

5 years ago
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.
(Assignee)

Comment 51

5 years ago
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.
(Assignee)

Comment 52

5 years ago
Created attachment 631532 [details] [diff] [review]
patch
Attachment #628129 - Attachment is obsolete: true
Attachment #631532 - Flags: review?(matspal)
(Assignee)

Updated

5 years ago
Attachment #631532 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #631532 - Flags: review?(matt.woodrow)
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
Attachment #631532 - Flags: review?(matspal) → review+
(Assignee)

Comment 54

5 years ago
(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 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?
Attachment #631532 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Attachment #631532 - Flags: review?(matt.woodrow)
(Assignee)

Comment 56

5 years ago
(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.
(Assignee)

Comment 57

5 years ago
Created attachment 631662 [details] [diff] [review]
patch

With review comments addressed.
Attachment #631532 - Attachment is obsolete: true
(Assignee)

Comment 58

5 years ago
Created attachment 631664 [details] [diff] [review]
patch
Attachment #631662 - Attachment is obsolete: true
(Assignee)

Comment 59

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b206c111cd
https://hg.mozilla.org/mozilla-central/rev/18b206c111cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
(Assignee)

Comment 61

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

Comment 62

5 years ago
[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
blocking-fennec1.0: + → ?

Updated

5 years ago
Attachment #631664 - Flags: approval-mozilla-beta?
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).
Status: RESOLVED → VERIFIED
Attachment #631664 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 64

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/5b923ee207e0
(Assignee)

Comment 65

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

Comment 66

5 years ago
Landed with that fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/0d744168d314
(Assignee)

Comment 67

5 years ago
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?
Attachment #631664 - Flags: approval-mozilla-aurora?
blocking-fennec1.0: ? → +
Attachment #631664 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox14: --- → fixed
status-firefox15: --- → affected
status-firefox16: --- → fixed
(Assignee)

Comment 68

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/4223ec8761b3
(Assignee)

Updated

5 years ago
status-firefox15: affected → fixed
Depends on: 764639
Blocks: 763570
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.
No longer depends on: 764639
status-firefox14: fixed → ---
status-firefox15: fixed → ---
status-firefox16: fixed → ---
You need to log in before you can comment on or make changes to this bug.