728983 - Maple invalidation problems with scrollable layer and multiple <div>s

Status: VERIFIED FIXED

(Core :: Graphics, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

This is quite noticeable on http://people.mozilla.org/~jmuizelaar/click-test.html.

The blue boxes don't show up until after scrolling.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

My guess is that it was https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 that caused the worseness.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Confirmed.

https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 is bad
https://hg.mozilla.org/projects/maple/rev/cf7c1f3d4c7f is good.

Comment 3

[Ali Juma [:ajuma]]

It's the change to nsGfxScrollFrame.cpp in that patch (forcing the creation of a scrollable layer) that's causing this.

Comment 4

[Ali Juma [:ajuma]]

The problem is indeed with invalidation when we have scrollable layers. I made a larger click-test page at http://people.mozilla.org/~ajuma/click-test.html, which is large enough to cause a scrollable layer to be created even without the patch applied, and the blue boxes don't get updated until after scrolling even without the patch applied. I'm now looking into what's wrong with invalidation for scrollable layers.

Comment 5

[Ali Juma [:ajuma]]

Looking into this further, it seems that Gecko is getting confused about visibility. If we click on the box where the click location div happens to be, the part of the box that overlaps the div turns purple, and when we click away that part turns white again. If we remove the div that displays the click location, everything works fine. If we move this div above the table, then the boxes in the table correctly turn blue when clicked, but the click location div doesn't get updated until a pan or zoom.

Comment 6

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

this was fixed

Comment 7

[Ali Juma [:ajuma]]

I'm still seeing this problem.

Comment 8

[Ali Juma [:ajuma]]

Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this problem.

Comment 9

[Ali Juma [:ajuma]]

(In reply to Ali Juma [:ajuma] from comment #8)
> Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this
> problem.

Actually, it fixes the problem of the clicked boxes not turning blue, but it also stops the click location div from updating.

Comment 10

[Ali Juma [:ajuma]]

This appears to be caused by the merging of nsDisplayScrollLayers. When we merge two nsDisplayScrollLayers, the frame associated with one of the nsDisplayScrollLayers is used to retain the layer; the problem seems to be that since the other frame is not being used to retain a layer, invalidation requests it receives are essentially having no effect (they just get passed on to the frame's parent).

This explains why the fix to Bug 729354 changes the behaviour we're seeing here -- since that fix changes which frame is used to retain a layer, it changes which frame correctly handles invalidation.

I believe Bug 729581 is caused by the same problem, but I've mostly been debugging this bug since it's easier to reproduce.

There may also be a relationship to the way we're creating new nsDisplayScrollLayers in nsGfxScrollFrameInner::BuildDisplayList on Maple, where we always set mShouldBuildLayer = true (which might explain why we're so frequently having to merge away nsDisplayScrollLayers).

Comment 11

[Joe Drew (not getting mail)]

Mats, if you have time today, can you look at this?

Failing that, I'd like Timothy to look at it once he gets up.

Comment 12

[Timothy Nikkel (:tnikkel)]

In nsGfxScrollFrame.cpp in ScrollLayerWrapper::WrapItem can we do

-return new (aBuilder) nsDisplayScrollLayer(aBuilder, aItem, aItem->GetUnderlyingFrame(), mScrolledFrame, mScrollFrame);
+return new (aBuilder) nsDisplayScrollLayer(aBuilder, aItem, mScrolledFrame, mScrolledFrame, mScrollFrame);

?

Comment 13

[Mats Palmgren (inactive)]

Unfortunately, that breaks z-index ordering.  I tried a few variations of your idea,
but something always breaks.

-> stechz

Comment 14

[Timothy Nikkel (:tnikkel)]

Hmm, how does it break z-index ordering? We don't change the order of the items in the display list, do we?

Comment 15

[Mats Palmgren (inactive)]

We do, http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1805

Comment 16

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Here's the test I was using; the pink block is on top with your suggested fix.

Comment 17

[Timothy Nikkel (:tnikkel)]

Can we hack display scroll layer items to have a z-order frame and use that for comparison when we sort?

I'd try this out myself but I don't know where to begin to get a working build for this (do I use the maple branch? what platforms does it work on? etc).

Comment 18

[Ali Juma [:ajuma]]

(In reply to Timothy Nikkel (:tn) from comment #17)
> I'd try this out myself but I don't know where to begin to get a working
> build for this (do I use the maple branch? what platforms does it work on?
> etc).

The maple branch on Android is where we've been running into these problems, so it's best to use that. (The build instructions are the same as for Native Fennec -- see https://wiki.mozilla.org/Android)

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Ali Juma [:ajuma] from comment #18)
> The maple branch on Android is where we've been running into these problems,
> so it's best to use that. (The build instructions are the same as for Native
> Fennec -- see https://wiki.mozilla.org/Android)

Those make it sound like I need an Android device, which I do not have.

Comment 20

[Joe Drew (not getting mail)]

Ben doesn't work on much Mozilla stuff any more, tragically.

Comment 21

[Joe Drew (not getting mail)]

We will fix this as follow-up to landing.

Comment 22

[Joe Drew (not getting mail)]

Timothy, Mats, Jet:

Can we get a confirmed owner for this bug? I'm not too concerned who that is, but right now it feels under-owned.

Comment 25

[Jet Villegas (inactive)]

Mats: can you pick this one up?

Comment 26

[Johnathan Nightingale [:johnath]]

Poking this - it's a fennec beta blocker and hasn't had activity in 2 weeks. Is there someone who should take this from Mats if he's over-committed?

Comment 27

[Jet Villegas (inactive)]

tn: another scroller for ya...

Comment 28

[Timothy Nikkel (:tnikkel)]

What are the specific steps to reproduce this? I haven't been successful in seeing this.

Comment 29

[Ali Juma [:ajuma]]

(In reply to Timothy Nikkel (:tn) from comment #28)
> What are the specific steps to reproduce this? I haven't been successful in
> seeing this.

1) Visit http://people.mozilla.org/~jmuizelaar/click-test.html
2) Tap on squares.

Result:
The square tapped on will turn blue, but the red rectangle showing the position of the last tap won't update until you pan enough for there to be a repaint. (And, fwiw, before Bug 729534 landed, the problem was reversed: the red rectangle would update, but the square tapped on would not turn blue.)

Comment 31

[Damon Sicore (:damons)]

Roc, are you on this?

Comment 32

[JP Rosevear [:jpr]]

(In reply to Damon Sicore (:damons) from comment #31)
> Roc, are you on this?

Yes he is, we discussed it in a meeting today.

Comment 33

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

Attachment: simple testcase

This testcase reproduces a related problem on desktop. The red box should turn green almost immediately after loading, but you don't see that happen. The reason is exactly what Ali pointed out in comment #10, applied to nsDisplayOpacity; we've merged nsDisplayOpacity items together, so we have a single layer containing the content under #o, but #o's first-in-flow frame isn't marked as a container layer, only the last-in-flow. So when we invalidate #d, the correct ThebesLayer is not invalidated.

Comment 34

[Matt Woodrow (:mattwoodrow)]

Interestingly, this testcase works with DLBI. Not sure why though, if the invalidation happens on the merged frame it won't be checked.

For DLBI I think we want merged display items to remember the list of frames that they correspond to. Then we can check each of the frames for the invalidated bit.

Comment 35

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> For DLBI I think we want merged display items to remember the list of frames
> that they correspond to. Then we can check each of the frames for the
> invalidated bit.

I'm doing the first part to fix this on trunk.

Comment 36

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

Attachment: fix

Comment 37

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 615614 [details] [diff] [review]
fix

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

Can I be a pain and ask that you land this as two separate pieces? I'd like to be able to backout the FrameLayerBuilder changes so they don't conflict with DLBI.

Comment 38

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

Sure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9157e2b13913
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbdb799fa6d

Comment 39

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/9157e2b13913
https://hg.mozilla.org/mozilla-central/rev/6dbdb799fa6d

Comment 40

[Adrian Tamas (:AdrianT)]

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

The squares are displayed blue when the user taps them at http://people.mozilla.org/~jmuizelaar/click-test.html