933264 - In some cases with the displayport set, the clip on a scrollframe isn't correct

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

STR:

1. Load http://people.mozilla.org/~kgupta/griddiv.html in the B2G browser
2. Scroll the div

The div doesn't get clipped like it should be.

Oddly enough when I test with http://people.mozilla.org/~kgupta/tmp/div.html which is (AFAICT) an equivalent page, the div is clipped just fine. Not sure where the difference in behaviour is coming from.

Also in general we know that the clipping condition and/or value needs to be looked at. See the discussion in bug 902505 (comments 11, 13, 15, 16, 18, 22, 23, 26 etc).

Comment 1

[Timothy Nikkel (:tnikkel)]

Perhaps bug 914864 is the reason we see this in one of the testcases but not the other.

Comment 2

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

I'm seeing this on metro too now.

Comment 3

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

I debugged this a bit today with tn's help, and it looks like a TEXT display item (corresponding to the red text above the scrollable div) is ending up in the display list between the two SCROLL_LAYER display items. This causes those items to not get merged and so we end up with a SCROLL_INFO_LAYER rather than SCROLL_LAYER items. The clip is therefore empty.

Comment 4

[Jim Mathies [:jimm]]

This this be marked blocking our 28 release kats?

Comment 5

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

Attachment: Make nsLayoutDebugger more mobile-friendly

Might as well fix this once and for all.

Comment 6

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

Attachment: Add more logging features to nsLayoutDebugger

Comment 7

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

Attachment: Logging of scrolledContent and aLists

This is what i get by logging "scrolledContent" and "aLists" just before the line at [1].

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=13fb1897c93c#2400

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 833477 [details]
Logging of scrolledContent and aLists

The problematic text item isn't in this list.

I think what happens is that when these display list sets get flatten into one display list. The text above the scroll frame goes into the content list below the text from inside the scrollframe, and the block border backgrounds go under all of the content in the stacking context. So I think the display list is correct. So I think we will need to look at the bounds of items to determine if they don't intersect so that we can change their order to make the scroll layer items adjacent so that they can merge.

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 833474 [details] [diff] [review]
Make nsLayoutDebugger more mobile-friendly

>+#define PRINT(dest, ...) \
>+    { if (!dest || dest == stderr) { \
>+        printf_stderr(__VA_ARGS__); \
>+      } else { \
>+        fprintf(dest, __VA_ARGS__); \
>+      } \
>+    }

Presumably this is going to need an ifdef for android and b2g so it can build?

Comment 10

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

(In reply to Timothy Nikkel (:tn) from comment #9)
> Presumably this is going to need an ifdef for android and b2g so it can
> build?

I didn't think so - Layers.cpp does the same thing [1], and AFAIK printf_stderr works on all platforms. But I pushed the patches to try anyway to make sure. [2]

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp?rev=92f737230338#1231
[2] https://tbpl.mozilla.org/?tree=Try&rev=795aa729ab2f

Comment 11

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

(In reply to Timothy Nikkel (:tn) from comment #8)
> The problematic text item isn't in this list.
> 
> I think what happens is that when these display list sets get flatten into
> one display list. The text above the scroll frame goes into the content list
> below the text from inside the scrollframe, and the block border backgrounds
> go under all of the content in the stacking context. So I think the display
> list is correct. So I think we will need to look at the bounds of items to
> determine if they don't intersect so that we can change their order to make
> the scroll layer items adjacent so that they can merge.

Do you have some time to take over this bug? It's getting deeper in layout code and there other APZC bugs I need to look at before I go off on PTO.

Comment 12

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

Thanks!

Comment 13

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> But I pushed the patches to try anyway
> to make sure. [2]
> 
> [2] https://tbpl.mozilla.org/?tree=Try&rev=795aa729ab2f

That try run failed on OS X because I was using a non-string-literal for the format string in a couple of places. New try run: https://tbpl.mozilla.org/?tree=Try&rev=ca7a71ebcb9d

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> I didn't think so - Layers.cpp does the same thing [1], and AFAIK
> printf_stderr works on all platforms.

Okay, for some reason I thought printf_stderr wouldn't compile on desktop platforms.

Comment 15

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

Attachment: Make nsLayoutDebugger more mobile-friendly (v2)

Updated to fix the OS X compile error, carrying r+

Comment 16

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

Attachment: Add more logging features to nsLayoutDebugger

Ditto

Comment 17

[Jim Mathies [:jimm]]

What are the side effects of this bug, and is it wide spread?

Marking blocking metrofx beta uplift, but if the fallout from this bug isn't wide spread, we can punt.

Comment 18

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

(In reply to Jim Mathies [:jimm] from comment #17)
> What are the side effects of this bug, and is it wide spread?

The effect is that sometimes divs with overflow:scroll render their content outside the div. The entire displayport area gets rendered rather than getting clipped to the scrollframe. I don't know how widespread it is, as it happens only in some circumstances that are not yet clear.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: try to merge scroll layer items by moving them if possible

Comment 20

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

We could do something like this. However, why are we failing to render correctly when the layers fail to merge? We surely have to fix that first. Failing to merge scrollayers into a single layer should never result in incorrect rendering, because there are legitimate cases where we can't merge.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> We could do something like this. However, why are we failing to render
> correctly when the layers fail to merge? We surely have to fix that first.
> Failing to merge scrollayers into a single layer should never result in
> incorrect rendering, because there are legitimate cases where we can't merge.

Ok, sure, but we will need something like this in order to async scroll in cases like these even after we fix the other rendering issue.

Comment 22

[Timothy Nikkel (:tnikkel)]

So I'll look into fixing that issue too, but we'll need this patch as well.

Comment 23

[Timothy Nikkel (:tnikkel)]

The reason for the visual bug is that the scroll layer item is the only place we store the clip that the scroll frame induces on it's content. We can't clip the scrolled content because we need it to be rendered so we can async scroll it into view. So I suppose that if we flatten a scroll layer item we need to apply it's clip it's child list.

Comment 24

[Timothy Nikkel (:tnikkel)]

Attachment: properly clip children of scroll layer items that we have to flatten away

This implements comment 23 and fixes the clipping issue when we can't merge our scroll layer items.

Comment 25

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

Comment on attachment 8344827 [details] [diff] [review]
properly clip children of scroll layer items that we have to flatten away

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

Can you make a reftest for this?

Comment 26

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

(In reply to Timothy Nikkel (:tn) from comment #21)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> > We could do something like this. However, why are we failing to render
> > correctly when the layers fail to merge? We surely have to fix that first.
> > Failing to merge scrollayers into a single layer should never result in
> > incorrect rendering, because there are legitimate cases where we can't merge.
> 
> Ok, sure, but we will need something like this in order to async scroll in
> cases like these even after we fix the other rendering issue.

Even with your patch, we can't always async scroll. So the question is, is your patch worth taking for the extra cases it allows us to async scroll? I'm not sure how to answer that. Can you argue that it is?

Comment 27

[Timothy Nikkel (:tnikkel)]

All it takes to interleave scroll layer items with non scroll layer items is content that goes on different lists (ie Content, BorderBackground) inside the scrollable frame and outside it before and after it (in the stacking context). So if there are borders/backgrounds and text inside and outside before and after the scrollable frame we will hit this problem. Text, borders/backgrounds are some of the most common elements that make up webpages.

Comment 28

[Timothy Nikkel (:tnikkel)]

This approach does have it limits though, eventually if we find important cases that can't be handled reasonable we might want to move scroll layer handling into FrameLayerBuilder.

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> Comment on attachment 8344827 [details] [diff] [review]
> properly clip children of scroll layer items that we have to flatten away
> 
> Review of attachment 8344827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you make a reftest for this?

I wasn't able to create a reftest that I saw reproduce the problem without user interaction. I added a reftest that reproduces the problem if I pinch the main html document while js scrolls the scrollable element, but the problem didn't show up without the pinching. I checked that reftest in.

Comment 30

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa2b73736aa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec30593d13ba

Comment 31

[Ryan VanderMeulen [:RyanVM]]

OSX 10.6 needed a touch of fuzz due to some minor differences on the scrollbar.
https://hg.mozilla.org/integration/mozilla-inbound/rev/74089d56dcdf

Comment 32

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

(In reply to Timothy Nikkel (:tn) from comment #28)
> This approach does have it limits though, eventually if we find important
> cases that can't be handled reasonable we might want to move scroll layer
> handling into FrameLayerBuilder.

We could do that. We would need new layers API to tie together multiple layers so that APZC knows to scroll them together.

Comment 33

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

Comment on attachment 8343459 [details] [diff] [review]
try to merge scroll layer items by moving them if possible

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

::: layout/base/nsDisplayList.cpp
@@ +1028,5 @@
> +                 (elements[j]->GetType() != nsDisplayItem::TYPE_SCROLL_LAYER ||
> +                  elements[j]->Frame() != item->Frame()) &&
> +                 !item->GetClippedBounds(aBuilder).Intersects(elements[j]->GetClippedBounds(aBuilder))) {
> +            --j;
> +          }

We need to limit the length of this search otherwise we get into O(N^2) situations.

OTOH if we limit the length of this search, we could make this optimization ineffective :-(.

I think we should move this patch to a new bug. But I also think that we should locate sites where this actually matters. We've had this limitation on Android for a while and I don't recall seeing many bugs about async scrolling failing due to this.

Comment 34

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> We need to limit the length of this search otherwise we get into O(N^2)
> situations.

Yeah, I thought about that, didn't figure out anything better unfortunately. We go over the list at most one extra time for every scroll frame in the stacking context that has a display port set. So we could limit it to some constant scroll frame per stacking context that we will try to do this maybe.
 
> OTOH if we limit the length of this search, we could make this optimization
> ineffective :-(.
> 
> I think we should move this patch to a new bug. But I also think that we
> should locate sites where this actually matters. We've had this limitation
> on Android for a while and I don't recall seeing many bugs about async
> scrolling failing due to this.

This limitation really only comes up when we try to do async scrolling on non-root scroll frames because there is no content before/after root scroll frames to interleave with scrolled content and get in the way of the merging process. AFAIK we only started to try to async scroll non-root scroll frames recently, which is why it is coming up now.

But yes, we can move this to a new bug.

Comment 35

[Timothy Nikkel (:tnikkel)]

Bug 939239 seems to already be filed for this very problem, I moved the patch over there.

Comment 36

[Timothy Nikkel (:tnikkel)]

kats, you still have some debugging patches unlanded from this bug. I know BenWa landed something similar, not sure how much overlap there is exactly.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aa2b73736aa2
https://hg.mozilla.org/mozilla-central/rev/ec30593d13ba
https://hg.mozilla.org/mozilla-central/rev/74089d56dcdf

Comment 38

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

It looks like his patches obsoleted the first of my debugging patches but the second one is still valid (although needs rebasing). I might clean it up and land it tomorrow... or the next time I need it.

Comment 39

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

This needs to go with the rest of the APZC work for the Gaia apps.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d3986855f3f
https://hg.mozilla.org/releases/mozilla-aurora/rev/b88814af4d30

Comment 42

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

Comment on attachment 8334484 [details] [diff] [review]
Add more logging features to nsLayoutDebugger

Rebased this debugging patch and landed it:

https://hg.mozilla.org/integration/b2g-inbound/rev/f0d0b84f2f48

It doesn't need uplifting anywhere.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0d0b84f2f48

Comment 44

[u279076]

Timothy, does this bug only affect B2G? If not can you advise QA on testing for desktop Firefox?

Comment 45

[Timothy Nikkel (:tnikkel)]

It should affect b2g, fennec, and metro. Not desktop though. Does that answer your question?

Comment 46

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #45)
> It should affect b2g, fennec, and metro. Not desktop though. Does that
> answer your question?

CCing Juan, Jason, and Aaron (QA for Metro, B2G, and Mobile, respectively). I'll let them ask any follow up questions as required.

Comment 47

[Matt Brubeck (:mbrubeck)]

Duplicate bug 937198 has an additional test case and steps to reproduce for Metro.

Comment 48

[Jason Smith [:jsmith]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #46)
> (In reply to Timothy Nikkel (:tn) from comment #45)
> > It should affect b2g, fennec, and metro. Not desktop though. Does that
> > answer your question?
> 
> CCing Juan, Jason, and Aaron (QA for Metro, B2G, and Mobile, respectively).
> I'll let them ask any follow up questions as required.

Naoki can probably help on this from the FxOS side - he's driving testing of the APZC across all Gaia apps.