Closed Bug 898444 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Project Flags:
blocking-b2g koi+
Tracking Flags:
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: kats, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(3 files)

Screenshot if clipping issue
(deleted), image/png
Details
Don't wrap borders or other scroll layers in a scroll layer
(deleted), patch
tnikkel
: review-
Details | Diff | Splinter Review
totally untested patch
(deleted), patch
tnikkel
: review+
kats
: feedback+
Details | Diff | Splinter Review 
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.
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
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.
Assignee: nobody → bgirard
blocking-b2g: --- → koi?
Pulling from tracker bug mainly because this isn't a smoketest regression.
No longer blocks: b2g-central-dogfood
Keywords: smoketest
This page behaves very strangely on my Unagi device (latest m-c + latest gaia), might be related: http://people.mozilla.org/~bballo/container.html
After some investigation, comment 4 appears to be a different problem. I have split it out into bug 907754.
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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
Flags: needinfo?(tnikkel)
Flags: needinfo?(matt.woodrow)
Blocks: 889883
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.)
Flags: needinfo?(tnikkel)
(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
Summary: Figure out why overflow:scroll subdocuments aren't scrolling properly → Figure out why overflow:scroll elements aren't scrolling properly
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?
Assignee: bgirard → bugmail.mozilla
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.
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?
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.
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?).
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"
Yeah, no matter what I do, can't seem to reproduce this on a Peak. The clipping thing is weird too.
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...
If I make nsDisplayScrollLayer::ShouldFlattenAway always return false, I get "correct" layerization and async (fast) scrolling. The clipping problem persists, however.
Daniel, can you lend a hand here as well?
Assignee: bugmail.mozilla → nobody
Flags: needinfo?(dholbert)
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
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>
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(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. :) )
Flags: needinfo?(dholbert)
: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.
Assignee: nobody → snorp
Before APZN was in place, I filed this bug 894991; is that related?
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?
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.
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?
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.
Not sure if this is Right or not, but it does seem to make things work. Clipping issue remains, however.
Attachment #803933 - Flags: review?(tnikkel)
I split the clipping problem out into bug 916125 because it will probably need its own investigation and patch.
blocking-b2g: koi? → koi+
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.
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.
Flags: needinfo?(roc)
Attachment #803933 - Flags: review?(tnikkel) → review-
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.
Flags: needinfo?(roc)
Attached patch totally untested patch (deleted) — — Splinter Review 
kats, does this fix things for you?
Assignee: snorp → roc
Attachment #812231 - Flags: review?(tnikkel)
Attachment #812231 - Flags: feedback?(bugmail.mozilla)
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.
Attachment #812231 - Flags: review?(tnikkel) → review+
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+.
Attachment #812231 - Flags: feedback?(bugmail.mozilla) → feedback+
https://hg.mozilla.org/mozilla-central/rev/42f245833598
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: