Closed Bug 625672 Opened 9 years ago Closed 9 years ago

missing content when scrolling

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: tnikkel, Assigned: roc)

References

()

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(4 files)

Try scrolling http://www.scribd.com/doc/46819931/19-Main . I see missing chunks of text.

Vlad reported the problem.

Happens for me on Windows and Linux (both without hw acceleration).

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a05e91710adb&tochange=c20f34eefa5d
The first bad revision is:
changeset:   59780:475fe8dd48a3
user:        Robert O'Callahan <robert@ocallahan.org>
date:        Mon Jan 03 14:48:09 2011 +1300
summary:     Bug 602757. Part 4: Let nsDisplayBackground::GetOpaqueRegion return opaque regions for arbitrary repeat modes, background-clip, border-radius, etc. r=tnikkel,sr=dbaron,a=blocking
blocking2.0: --- → ?
Assignee: nobody → roc
blocking2.0: ? → final+
Whiteboard: [softblocker]
The pages are inside a transform which probably has something to do with it.
Attached file reduced testcase
Scroll down using the arrow key, and you'll see that some text is not being repainted. Causing invalidation, e.g. by selecting it, makes it visible.

I don't think bug 602757 caused this, it just revealed the bug somehow. Definitely a regression though.
The problem is the following sequence of events:
-- Compute visibility for the entire display list. Text display items under the fixed-pos footer are marked invisible.
-- Build layer tree. This includes building a temporary layer manager and its layer tree to render the contents of the inactive transform's child display list. That layer tree is built using the current visibility settings, so text display items under the footer aren't included.
-- In DrawThebesLayer for the retained ThebesLayer, we recompute visibility for all display items including the transform's child display list, based on the area that the retained ThebesLayer wants us to draw, which includes the area under the footer.
-- We paint the display items to the ThebesLayer. For the transform, this means asking its temporary layer manager to render. Unfortunately, since we haven't updated its layer tree for the new visibility information, items under the footer aren't drawn.
So basically we need to delay constructing the layer tree for the temporary layer managers until we know what area the ThebesLayer wants us to paint, so we can compute the correct visibility for the display items rendering through that temp layer manager.

The reason we'd moved it earlier is that constructing the temp layer manager can require changes to the retained layer manager ... see the comment in BuildTempManagerForInactiveLayer. Although right now I can't remember what those changes actually are.
While working on a test for this bug, I realized that many of the scrolling reftests aren't actually testing anything, because we're trying to scroll the body element, but we should be scrolling via the root element.

This patch adds code to scrolling.js so if we try to scroll but fail, the test will fail with a helpful message.
Attachment #505188 - Flags: review?(dbaron)
This fixes the bug. The question is whether it breaks anything. Running reftests now.
I should mention that the test in part 2 doesn't actually work :-(. I couldn't get it to fail in the reftest harness, although it does work if you execute it manually.
Comment on attachment 505188 [details] [diff] [review]
Part 1: Fix scrolling tests

r=dbaron
Attachment #505188 - Flags: review?(dbaron) → review+
Turns out with part 2, we need this patch. Creating a temp layer manager for a video which doesn't return a layer needs to not crash.
Attachment #505217 - Flags: review?
Attachment #505217 - Flags: review? → review?(bas.schouten)
Comment on attachment 505217 [details] [diff] [review]
Part 1.5: don't crash on EndTransaction with no root layer.

Looks good, for the D3D versions we can probably delete the mRoot checks in Render() then.
Attachment #505217 - Flags: review?(bas.schouten) → review+
Comment on attachment 505189 [details] [diff] [review]
Part 2: move temp layer manager construction

Green on try.
Attachment #505189 - Flags: review?(tnikkel)
Whiteboard: [softblocker] → [softblocker][needs landing]
Whiteboard: [softblocker][needs landing] → [softblocker][needs review]
It should be very easy to convert the testcase here to an automated test...
Flags: in-testsuite?
(In reply to comment #14)
> It should be very easy to convert the testcase here to an automated test...

Which is what attachment 505189 [details] [diff] [review] does, it seems.  Sorry for the noise.  :/
(In reply to comment #8)
> I should mention that the test in part 2 doesn't actually work :-(. I couldn't
> get it to fail in the reftest harness, although it does work if you execute it
> manually.

Hmm.  Couldn't you just take the minimized testcase and convert it into a mochitest which takes snapshots of an iframe?
I don't think so. That's what the reftest harness does :-).
(In reply to comment #17)
> I don't think so. That's what the reftest harness does :-).

yes, but we can simulate the down arrow key in a mochitest, right?
Yes, but I don't think the down arrow key is important to reproducing the problem.
Attachment #505189 - Flags: review?(tnikkel) → review+
Whiteboard: [softblocker][needs review] → [softblocker][needs landing]
Verified on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.