This is quite noticeable on http://people.mozilla.org/~jmuizelaar/click-test.html.
The blue boxes don't show up until after scrolling.
My guess is that it was https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 that caused the worseness.
https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 is bad
https://hg.mozilla.org/projects/maple/rev/cf7c1f3d4c7f is good.
It's the change to nsGfxScrollFrame.cpp in that patch (forcing the creation of a scrollable layer) that's causing this.
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.
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.
this was fixed
I'm still seeing this problem.
Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this problem.
(In reply to Ali Juma [:ajuma] from comment #8)
> Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this
Actually, it fixes the problem of the clicked boxes not turning blue, but it also stops the click location div from updating.
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.
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.
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]
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).
(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?
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.
We will fix this as follow-up to landing.
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.
*** Bug 729581 has been marked as a duplicate of this bug. ***
*** Bug 728036 has been marked as a duplicate of this bug. ***
Mats: can you pick this one up?
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?
tn: another scroller for ya...
What are the specific steps to reproduce this? I haven't been successful in seeing this.
(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.
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.)
*** Bug 745471 has been marked as a duplicate of this bug. ***
Roc, are you on this?
(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]
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]
Comment on attachment 615614 [details] [diff] [review]
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.
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