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

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: kats, Assigned: tnikkel)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

()

Attachments

(5 attachments, 2 obsolete attachments)

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).
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
No longer blocks: 902505
Assignee

Comment 1

6 years ago
Perhaps bug 914864 is the reason we see this in one of the testcases but not the other.
Blocks: 937198
I'm seeing this on metro too now.
Assignee: nobody → bugmail.mozilla
Blocks: 915723
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
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.
This this be marked blocking our 28 release kats?
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
Whiteboard: [block28]
Might as well fix this once and for all.
Attachment #833474 - Flags: review?(tnikkel)
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
Attachment #833477 - Flags: feedback?
Attachment #833477 - Flags: feedback?
Assignee

Comment 8

6 years ago
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.
Assignee

Updated

6 years ago
Attachment #833475 - Flags: review?(tnikkel) → review+
Assignee

Comment 9

6 years ago
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?
Attachment #833474 - Flags: review?(tnikkel) → review+
(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
(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.
Assignee

Updated

6 years ago
Assignee: bugmail.mozilla → tnikkel
Thanks!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Assignee

Comment 14

6 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Updated to fix the OS X compile error, carrying r+
Attachment #833474 - Attachment is obsolete: true
Attachment #8334482 - Flags: review+
Ditto
Attachment #833475 - Attachment is obsolete: true
Attachment #8334484 - Flags: review+

Updated

6 years ago
No longer blocks: 915723

Updated

6 years ago
Blocks: metro-apzc
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.
Whiteboard: [block28] → [beta28]

Updated

6 years ago
Whiteboard: [beta28] → [beta28][layout]

Updated

6 years ago
Summary: In some cases with the displayport set, the clip on a scrollframe isn't correct → In some cases with the displayport set, the clip on a scrollframe isn't correct (was: multiple flings result in blank screen)
Blocks: metrov1backlog, metrov1omtc&apzc
No longer blocks: metro-apzc

Updated

6 years ago
Summary: In some cases with the displayport set, the clip on a scrollframe isn't correct (was: multiple flings result in blank screen) → In some cases with the displayport set, the clip on a scrollframe isn't correct
Whiteboard: [beta28][layout]
(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.

Updated

6 years ago
No longer blocks: metrov1backlog, metrov1omtc&apzc
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.
Assignee

Comment 21

6 years ago
(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.
Assignee

Comment 22

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

Comment 23

6 years ago
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.
Assignee

Comment 24

6 years ago
This implements comment 23 and fixes the clipping issue when we can't merge our scroll layer items.
Attachment #8344827 - Flags: review?(roc)
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?
Attachment #8344827 - Flags: review?(roc) → review+
(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?
Assignee

Comment 27

6 years ago
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.
Assignee

Comment 28

6 years ago
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.
Assignee

Comment 29

6 years ago
(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.
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
(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 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.
Attachment #8343459 - Flags: review?(roc)
Assignee

Comment 34

6 years ago
(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.
Assignee

Updated

6 years ago
Whiteboard: [leave open]
Assignee

Comment 35

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

Comment 36

6 years ago
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.
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
Attachment #8334482 - Flags: checkin-
Attachment #8344827 - Flags: checkin+
This needs to go with the rest of the APZC work for the Gaia apps.
blocking-b2g: --- → 1.3+
No longer blocks: 937198
Duplicate of this bug: 937198
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.
Attachment #8334484 - Flags: checkin+
Timothy, does this bug only affect B2G? If not can you advise QA on testing for desktop Firefox?
Flags: needinfo?(tnikkel)
Assignee

Comment 45

6 years ago
It should affect b2g, fennec, and metro. Not desktop though. Does that answer your question?
Flags: needinfo?(tnikkel)
(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.
Duplicate bug 937198 has an additional test case and steps to reproduce for Metro.
(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.
You need to log in before you can comment on or make changes to this bug.