Closed
Bug 728983
Opened 12 years ago
Closed 12 years ago
Maple invalidation problems with scrollable layer and multiple <div>s
Categories
(Core :: Graphics, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla14
Tracking | Status | |
---|---|---|
blocking-fennec1.0 | --- | beta+ |
People
(Reporter: jrmuizel, Assigned: roc)
References
Details
(Whiteboard: MAPLE [layout])
Attachments
(3 files)
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•12 years ago
|
Whiteboard: mwc-demo maple
Reporter | ||
Comment 1•12 years ago
|
||
My guess is that it was https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 that caused the worseness.
Reporter | ||
Comment 2•12 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•12 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•12 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•12 years ago
|
Whiteboard: mwc-demo maple → maple
Comment 5•12 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•12 years ago
|
Whiteboard: maple → MAPLE mwc-demo
Updated•12 years ago
|
Keywords: fennecnative-betablocker
Updated•12 years ago
|
Priority: -- → P3
Whiteboard: MAPLE mwc-demo → MAPLE
Comment 6•12 years ago
|
||
this was fixed
Comment 7•12 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
Updated•12 years ago
|
blocking-fennec1.0: --- → beta+
Comment 8•12 years ago
|
||
Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this problem.
Comment 9•12 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•12 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).
Comment 11•12 years ago
|
||
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
Updated•12 years ago
|
Blocks: land-maple
Comment 12•12 years ago
|
||
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•12 years ago
|
||
Unfortunately, that breaks z-index ordering. I tried a few variations of your idea, but something always breaks. -> stechz
Assignee: matspal → ben
Comment 14•12 years ago
|
||
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•12 years ago
|
||
We do, http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1805
Comment 16•12 years ago
|
||
Here's the test I was using; the pink block is on top with your suggested fix.
Comment 17•12 years ago
|
||
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•12 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)
Comment 19•12 years ago
|
||
(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•12 years ago
|
||
Ben doesn't work on much Mozilla stuff any more, tragically.
Assignee: ben → tnikkel
Updated•12 years ago
|
Whiteboard: MAPLE → MAPLE [gfx]
Updated•12 years ago
|
Whiteboard: MAPLE [gfx] → MAPLE [layout]
Comment 22•12 years ago
|
||
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 26•12 years ago
|
||
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?
Updated•12 years ago
|
Priority: P3 → P1
Comment 28•12 years ago
|
||
What are the specific steps to reproduce this? I haven't been successful in seeing this.
Comment 29•12 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.)
Updated•12 years ago
|
Assignee: tnikkel → roc
Comment 31•12 years ago
|
||
Roc, are you on this?
Comment 32•12 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.
Assignee | ||
Comment 33•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 35•12 years ago
|
||
(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.
Assignee | ||
Comment 36•12 years ago
|
||
Attachment #615614 -
Flags: review?(matt.woodrow)
Comment 37•12 years ago
|
||
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+
Assignee | ||
Comment 38•12 years ago
|
||
Sure. https://hg.mozilla.org/integration/mozilla-inbound/rev/9157e2b13913 https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbdb799fa6d
Comment 39•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9157e2b13913 https://hg.mozilla.org/mozilla-central/rev/6dbdb799fa6d
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 40•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•