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

VERIFIED FIXED in mozilla14

Status

()

Core
Graphics
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: roc)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-fennec1.0 beta+)

Details

(Whiteboard: MAPLE [layout])

Attachments

(3 attachments)

(Reporter)

Description

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

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

Updated

6 years ago
Whiteboard: mwc-demo maple
(Reporter)

Comment 1

6 years ago
My guess is that it was https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 that caused the worseness.
(Reporter)

Comment 2

6 years ago
Confirmed.

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

Comment 3

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

Comment 4

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

Updated

6 years ago
Whiteboard: mwc-demo maple → maple

Comment 5

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

Updated

6 years ago
Whiteboard: maple → MAPLE mwc-demo
Keywords: fennecnative-betablocker
Priority: -- → P3
Whiteboard: MAPLE mwc-demo → MAPLE
Keywords: qawanted
this was fixed
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Keywords: qawanted
Resolution: --- → FIXED

Comment 7

6 years ago
I'm still seeing this problem.
Assignee: nobody → ajuma
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Maple invalidation got much worse after Feb 19 → Maple invalidation problems with scrollable layer and multiple <div>s
blocking-fennec1.0: --- → beta+

Comment 8

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

Comment 9

6 years ago
(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

6 years ago
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).
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.
Assignee: ajuma → matspal
Blocks: 725095
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);

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

-> stechz
Assignee: matspal → ben
Hmm, how does it break z-index ordering? We don't change the order of the items in the display list, do we?
We do, http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1805
Created attachment 602768 [details]
Testcase #1

Here's the test I was using; the pink block is on top with your suggested fix.
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

6 years ago
(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)
(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.
Ben doesn't work on much Mozilla stuff any more, tragically.
Assignee: ben → tnikkel
We will fix this as follow-up to landing.
No longer blocks: 725095
Whiteboard: MAPLE → MAPLE [gfx]
Whiteboard: MAPLE [gfx] → MAPLE [layout]
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.
Duplicate of this bug: 729581
Duplicate of this bug: 728036

Comment 25

6 years ago
Mats: can you pick this one up?
Assignee: tnikkel → matspal
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

6 years ago
tn: another scroller for ya...
Assignee: matspal → tnikkel
Blocks: 729951
Priority: P3 → P1
What are the specific steps to reproduce this? I haven't been successful in seeing this.

Comment 29

5 years ago
(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.)
Blocks: 735864
Duplicate of this bug: 745471

Updated

5 years ago
Assignee: tnikkel → roc
Roc, are you on this?

Comment 32

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

Yes he is, we discussed it in a meeting today.
Created attachment 615596 [details]
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.
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.
(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.
Created attachment 615614 [details] [diff] [review]
fix
Attachment #615614 - Flags: review?(matt.woodrow)
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.
Attachment #615614 - Flags: review?(matt.woodrow) → review+
Sure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9157e2b13913
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbdb799fa6d
https://hg.mozilla.org/mozilla-central/rev/9157e2b13913
https://hg.mozilla.org/mozilla-central/rev/6dbdb799fa6d
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 753329
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
Status: RESOLVED → VERIFIED
Depends on: 765677

Updated

5 years ago
Depends on: 775146

Updated

5 years ago
No longer depends on: 775146
You need to log in before you can comment on or make changes to this bug.