898444 - Figure out why overflow:scroll elements aren't scrolling properly

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

The patches in bug 866232 allow me to scroll iframes just fine in the B2G browser. However, overflow:scroll divs don't scroll. I suspect that either the ScrollInfoLayer isn't being generated, or we're using the wrong thing for hit testing.

I'm marking this as blocking 866232 because it is technically a regression from that bug, but I'm splitting it out into a separate bug because (a) it is parallelizable and (b) I really want to get that stuff landed, even with this regression.

Comment 1

[Benoit Girard (:BenWa)]

I just looked at the code and the most obvious spot only disables it for overflow:none:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2278

Comment 2

[Kartikaya Gupta (email:[e-mail])]

For the record, the specific test case that I was trying to scroll was this: http://people.mozilla.com/~kgupta/tmp/div.html

It's pretty simple but on the B2G browser with my patches the layer tree doesn't contain a IsScrollable() layer.

Comment 3

[Jason Smith [:jsmith]]

Pulling from tracker bug mainly because this isn't a smoketest regression.

Comment 4

[Botond Ballo [:botond]]

This page behaves very strangely on my Unagi device (latest m-c + latest gaia), might be related: http://people.mozilla.org/~bballo/container.html

Comment 5

[Kartikaya Gupta (email:[e-mail])]

After some investigation, comment 4 appears to be a different problem. I have split it out into bug 907754.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

So after discussing this with mattwoodrow to figure out how this is supposed to work, I think at least part of the problem is around [1]. I don't think this builds a nsDisplayScrollLayer anywhere, although I'm expecting it to based on my understanding of what should be happening. It still falls through and builds the nsDisplayScrollInfoLayer even after the displayport is set. I will look into it a bit more but any thoughts on this are more than welcome.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2311

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> I don't think
> this builds a nsDisplayScrollLayer anywhere, although I'm expecting it to
> based on my understanding of what should be happening.

Correction: it does build an nsDisplayScrollLayer (it builds 3 actually, one of which is for the content, and the other two are presumably for the border backgrounds), but that nsDisplayScrollLayer's BuildLayer function is never called. Perhaps the layer is getting flattened away when it shouldn't be?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Based on my printf debugging I do believe the bug lies with the flattening behaviour when there are nsDisplayScrollLayers mixed with nsDisplayScrollInfoLayer. However I don't really understand the code so I don't know (a) what the code is supposed to be doing and (b) what it's doing wrong. I see the displayport being set and read correctly on the right frame. The code at [1] is hit, which creates 3 nsDisplayScrollLayer instances, one of which is at [2]. It then hits [3] which adds an nsDisplayScrollInfoLayer to the list. I then see it run through the algorithm at [4], the purpose of which is not clear to me. Regardless, the TryMerge and ShouldFlattenAway functions are called on all three of the nsDisplayScrollLayer instances in turn, and they all return false for TryMerge but true for ShouldFlattenAway, and so they are replaced in the list (by their contents? - not sure). The nsDisplayScrollInfoLayer returns false for both TryMerge and ShouldFlattenAway and so remains as-is.

Can somebody enlighten me as to what this code is intended to do and whether it looks like it's behaving correctly? The STR for what I'm seeing are:
1) Run master on a Geeksphone peak (I have some local changes but they do not affect this)
2) Start the browser
3) Load http://people.mozilla.com/~kgupta/tmp/div.html

[1] http://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/layout/generic/nsGfxScrollFrame.cpp#l2311
[2] http://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/layout/base/nsDisplayList.cpp#l2945
[3] http://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/layout/generic/nsGfxScrollFrame.cpp#l2317
[4] http://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/layout/base/nsDisplayList.cpp#l1004

Comment 9

[Timothy Nikkel (:tnikkel)]

For your testcase I don't think TryMerge for the nsDisplayScrollLayer should be return false and not merging.

The child items of a scroll frame can interleave with non-scrollable content on the page due to a quirk of CSS. If this happens we don't try to create an async-scrollable layer. In this case we paint the children of the scroll frame as if they are not scrollable. But we do create an empty container layer (nsDisplayScrollInfoLayer is responsible for this), this allows the frame metrics etc for the scroll frame to transferred to the compositor.

So what the code in question is doing is trying to merge all nsDisplayScrollLayer's into one, if that succeeds we use that one item and destroy the nsDisplayScrollInfoLayer. If that fails (can't merge all the nsDisplayScrollLayer's) then we keep the nsDisplayScrollInfoLayer, and the scroll frame's child items aren't wrapped in a scroll layer.

From your description the code seems to be working as expected except that I would think TryMerge should be returning true for your testcase. Looking at why it returns false for TryMerge and what the list of display items looks like in that ComputeVisibility call would be helpful.

(The other issue is that we should still be able to scroll reasonably if this case actually comes up, but I think the above is more important to figure out.)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #9)
> From your description the code seems to be working as expected except that I
> would think TryMerge should be returning true for your testcase. Looking at
> why it returns false for TryMerge and what the list of display items looks
> like in that ComputeVisibility call would be helpful.

I know from my previous logging that it was returning false from the first condition in TryMerge, at [1]. I can update my logging to print out the entire list of display items and see what that looks like.

[1] http://hg.mozilla.org/mozilla-central/file/d54e0cce6c17/layout/base/nsDisplayList.cpp#l3326

Comment 11

[Kartikaya Gupta (email:[e-mail])]

So at the time that ComputeVisibility is run, the displaylist now looks like this:

bug898444: dumping out ComputeVisibilityForSublist types
bug898444: at index [5] item is [0x44aaa878] of type [44:SCROLL_INFO_LAYER]
bug898444: at index [4] item is [0x44aaa8e0] of type [57:TEXT]
bug898444: at index [3] item is [0x44aaa810] of type [43:SCROLL_LAYER]
bug898444: at index [2] item is [0x40445c48] of type [57:TEXT]
bug898444: at index [1] item is [0x44aaa390] of type [43:SCROLL_LAYER]
bug898444: at index [0] item is [0x40445c10] of type [13:CANVAS_BACKGROUND_COLOR]

I'm pretty sure this is different than what I was seeing before though, because this arrangement is triggering the following assertion:

###!!! ABORT: nsDisplayScrollLayer should always be defined: 'hasCount', file /Users/kats/zspace/mozilla-git/layout/base/nsDisplayList.cpp, line 3393

which wasn't getting triggered before. Were there any recent changes that might have caused this?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Hm. Perhaps my logging caused the above crash somehow. I rolled back to an older changeset without my logging and it doesn't crash any more.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Yeah, ignore comment 11. I had a local change left in amidst my debugging code. This is the actual list in ComputeVisibilityForSublist:

bug898444: dumping out ComputeVisibilityForSublist types
bug898444: at index [5] item is [0x44bab4e0] of type [57:TEXT]
bug898444: at index [4] item is [0x44bab410] of type [43:SCROLL_LAYER]
bug898444: at index [3] item is [0x40442c48] of type [57:TEXT]
bug898444: at index [2] item is [0x44baa790] of type [43:SCROLL_LAYER]
bug898444: at index [1] item is [0x44bab478] of type [44:SCROLL_INFO_LAYER]
bug898444: at index [0] item is [0x40442c10] of type [13:CANVAS_BACKGROUND_COLOR]

My logging shows that TryMerge returns false for 0x44bab410 because the item below it is not a scroll layer, and then it gets flattened away. Same for 0x44baa790, except I believe that during the flattening process another scroll layer is added to the list, again just above the SCROLL_INFO_LAYER. That one also returns false for TryMerge and gets flattened away.

From what you said in comment 9 it sounds like what is happening is perfectly normal and expected. However I guess it's not really what I was expecting to happen because I really do want a scroll layer forced for this div now that we have a displayport set on it. Is there some way to make that happen?

Comment 14

[Timothy Nikkel (:tnikkel)]

Debugged this some with kats on irc. At least one of the problems here is that the background/borders for the element are wrapped in scroll layer items when that is (usually) incorrect: the background/border usually doesn't scroll with the element. This is just due to the way nsGfxScrollFrameInner::BuildDisplayList is structured.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

I get different behavior with current B2G master. I can scroll the div on Kats' test site, but the clipping is all busted (text shows outside the bounds of the div). Unfortunately I can't seem to figure out how to take a screen capture (wtf?).

Comment 16

[Kartikaya Gupta (email:[e-mail])]

I've seen that behaviour too on occasion. Usually after scrolling it a little it does a reflow or whatever and then it's back to the case where it doesn't scroll properly.

Screen captures on B2G are usually "tap power while holding down home"

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, no matter what I do, can't seem to reproduce this on a Peak. The clipping thing is weird too.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Screenshot if clipping issue

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Alright the clipping issue is apparently caused by the displayport on the frame coupled with improper layerization. I can scroll it fine, just not async. Digging some more...

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

If I make nsDisplayScrollLayer::ShouldFlattenAway always return false, I get "correct" layerization and async (fast) scrolling. The clipping problem persists, however.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Daniel, can you lend a hand here as well?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

For additional context on our investigation on Monday, see the IRC log at around http://logbot.glob.com.au/?c=mozilla%23gfx&s=9%20Sep%202013&e=9%20Sep%202013#c86218

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Just FYI, this is the paint list I get:

Painting --- before optimization (dirty 0,-11,55980,85804):
  CanvasBackgroundColor 0x43ef08c0() bounds(0,0,58800,85804) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  uniform layer=0x434e9100
  ScrollInfoLayer 0x439235d8() bounds(0,0,0,0) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434fa800
  ScrollLayer 0x43924418() bounds(480,3920,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434fc800
    ScrollLayer 0x43924418() bounds(480,3920,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434fc800
      Border 0x439235d8() bounds(480,3920,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434ebb00
  Text 0x43923560() bounds(440,440,39932,3520) visible(0,0,0,0) componentAlpha(440,440,39932,3520) clip()  layer=0x434e92c0
  ScrollLayer 0x43924418() bounds(480,-3635,18011,60000) visible(0,0,0,0) componentAlpha(480,-3645,18011,60240) clip()  layer=0x434fc800
    Text 0x43924500() bounds(480,-3645,17438,1360) visible(0,0,0,0) componentAlpha(480,-3645,17438,1360) clip(480,-3635,18060,60000)  layer=0x434e9d40
    <bunch of Text>

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #23)
>   ScrollLayer 0x43924418() bounds(480,3920,18080,24080) visible(0,0,0,0)
> componentAlpha(0,0,0,0) clip()  layer=0x434fc800
>     ScrollLayer 0x43924418() bounds(480,3920,18080,24080) visible(0,0,0,0)
> componentAlpha(0,0,0,0) clip()  layer=0x434fc800
>       Border 0x439235d8() bounds(480,3920,18080,24080) visible(0,0,0,0)
> componentAlpha(0,0,0,0) clip()  layer=0x434ebb00

Both of these scroll layer items shouldn't be there. One of them is because we add the border/background to the display list and then wrap that list with scroll layer items in nsGfxScrollFrameInner::BuildDisplayList. (The background item should only be wrapped if it is background-attachment: local i think). Where the other scroll layer item comes from I'm not sure. We need to figure out where it comes from.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Milan Sreckovic [:milan] from comment #21)
> Daniel, can you lend a hand here as well?

(I'm not particularly familiar our scroll-frame or layers code, so I don't think I can help without doing a lot of digging to (re-)familiarize myself with those areas.  I'll defer to tn, since he seems to have an idea of what's going on. :) )

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

:snorp will see where tn's instructions get us - thinks there may be multiple bugs.  Matt, we don't mind if you have a comment as well.

Comment 27

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Before APZN was in place, I filed this bug 894991; is that related?

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

So if I comment out the ScrollLayerWrapper stuff here: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2305

I get the following paint list:

Painting --- before optimization (dirty -29,-7378,47430,85804):
  CanvasBackgroundColor 0x43f988c0() bounds(-40,-7360,58800,85804) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  uniform layer=0x434e9100
  ScrollInfoLayer 0x439225d8() bounds(0,0,0,0) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x43ee5c00
  Border 0x439225d8() bounds(440,-3440,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434e9100
  Text 0x43922560() bounds(400,-6920,39932,3520) visible(0,0,0,0) componentAlpha(400,-6920,39932,3520) clip()  layer=0x434e9100
  <lots more Text>

The list looks the same after optimization as well. So that means that all ScrollLayer items are coming from this wrapper. So we just need to stop doing that for some circumstances? Like when the immediate child is also a ScrollLayer? What else?

Comment 29

[Timothy Nikkel (:tnikkel)]

Ok, so that wrapper is making both scroll layer items somehow (which I'd still like to understand). So we just need a patch that applies the scroll layer wrapping to the correct items only.

Comment 30

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ok so I have a patch now that just detects if we are trying to wrap a scroll layer and doesn't do that if so. I now have a paint list like this:

Painting --- before optimization (dirty 0,0,54720,85804):
  CanvasBackgroundColor 0x43d898c0() bounds(0,0,58800,85804) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  uniform layer=0x434ea600
  ScrollInfoLayer 0x440665d8() bounds(0,0,0,0) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x43de0400
  ScrollLayer 0x44067418() bounds(480,3920,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip() 
    Border 0x440665d8() bounds(480,3920,18080,24080) visible(0,0,0,0) componentAlpha(0,0,0,0) clip()  layer=0x434ea600
  Text 0x44066560() bounds(440,440,39932,3520) visible(0,0,0,0) componentAlpha(440,440,39932,3520) clip()  layer=0x434ea600
  ScrollLayer 0x44067418() bounds(480,-14080,18000,60000) visible(0,0,0,0) componentAlpha(480,-14975,18011,61520) clip() 
    Text 0x441ca390() bounds(480,-14975,17360,1360) visible(0,0,0,0) componentAlpha(480,-14975,17360,1360) clip(480,-14080,18000,60000)  layer=0x434ea7c0

Is that mostly correct now? Things still don't work. Do I need to eliminate the ScrollLayer for the Border still? Does the ScrollInfoLayer need to be beside the ScrollLayer for the Text?

Comment 31

[Timothy Nikkel (:tnikkel)]

The problem is that the background/border items that get added to the display list here http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#2162 are wrapped in scroll layer items. The border/background of the scroll frame don't scroll with the scroll frame (except in a few cases), so they shouldn't be inside a scroll layer item. I don't know how we get those two nested scroll layer items (instead of just one), but your experiment indicated that we wouldn't get either scroll layer item if we didn't (incorrectly) wrap the border item in the first place.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't wrap borders or other scroll layers in a scroll layer

Not sure if this is Right or not, but it does seem to make things work. Clipping issue remains, however.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

I split the clipping problem out into bug 916125 because it will probably need its own investigation and patch.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

I wonder if https://bug919144.bugzilla.mozilla.org/attachment.cgi?id=810380 will make any difference here?

Comment 35

[Timothy Nikkel (:tnikkel)]

Comment on attachment 803933 [details] [diff] [review]
Don't wrap borders or other scroll layers in a scroll layer

I don't think this approach is workable. The type of the display item isn't important, it's whether or not the item is scrolled or not. It just happens in this case it's a border item that is not scrolled but put installed a scroll layer item.

Possible ideas to fix this.

Add a flag to display items that says "put me in a scroll layer (if you are creating one)" and then call a function on the display list builder when we start adding scrollable items that will make the builder set this flag to true on all newly created display items (we overload new, so that shouldn't be too hard to do). Then when we wrap the items we only wrap items with this flag set to true (we clear the flag when we wrap).

Do a full pass over the display list after we've created it in nsGfxScrollFrame and wrap the items whose active scrolled root is this? This needs more thought.

Maybe there is more structure to how we create the display list in nsGfxScrollFrame that we can exploit to only capture the scrollable items in a better/easier way that I haven't figured out yet.

Comment 36

[Timothy Nikkel (:tnikkel)]

roc, you have any ideas here?

The basic problem is that we wrap the background/border items for the scroll frame that we add in nsGfxScrollFrameInner::BuildDisplay list in the scroll layer items when they are not scrollable. So we need a good way to only wrap the scrollable items and not any other items that are be in the display list.

Comment 37

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

Sorry, wrapping aLists is totally wrong, since in general that contains lots of stuff that doesn't even belong to the scrollable frame. Let me give you a patch to try.

Comment 38

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

Attachment: totally untested patch

kats, does this fix things for you?

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 812231 [details] [diff] [review]
totally untested patch

Ah yes, this was the obvious patch that I thought wouldn't be correct because we could place positioned items above or below items in the list. But I guess that process happens at the stacking context level of building display lists, not here.

One thing though, does this handle background-attachment: local backgrounds correctly? I guess if this doesn't then we didn't before either.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 812231 [details] [diff] [review]
totally untested patch

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

So with a recent master build I don't see the problem on my original test page. I'm not sure if some other change fixed it or I'm just "intermittently" not seeing it. Regardless, the behaviour with the patch is what I would like, so f+.

Comment 41

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/42f245833598

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/42f245833598

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14a9e13e8c65