Last Comment Bug 786502 - Static background on Bungie.net appears to scroll away with the content
: Static background on Bungie.net appears to scroll away with the content
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: 18 Branch
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
http://bungie.net
Depends on: 797021
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 16:55 PDT by Michael Comella (:mcomella)
Modified: 2012-10-28 23:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
Textured background (477.29 KB, image/png)
2012-08-28 16:55 PDT, Michael Comella (:mcomella)
no flags Details
Textured background after zooming (390.20 KB, image/png)
2012-08-28 16:56 PDT, Michael Comella (:mcomella)
no flags Details
Page appearance after background is scrolled away (671.14 KB, image/png)
2012-08-28 16:57 PDT, Michael Comella (:mcomella)
no flags Details
Part 1 - Fix async scrolling with background-attachment:fixed (13.76 KB, patch)
2012-08-31 08:16 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not (27.64 KB, patch)
2012-08-31 08:20 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not (27.62 KB, patch)
2012-08-31 08:40 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not (27.57 KB, patch)
2012-08-31 09:04 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Fix async scrolling with background-attachment:fixed (31.32 KB, patch)
2012-09-05 07:49 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 - Fix async scrolling with background-attachment:fixed (31.25 KB, patch)
2012-09-06 09:31 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 2 - Add a couple of tests for background layers (7.22 KB, patch)
2012-09-06 09:33 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 - Fix async scrolling with background-attachment:fixed (33.38 KB, patch)
2012-09-06 19:33 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 - Fix async scrolling with background-attachment:fixed (32.33 KB, patch)
2012-09-06 19:43 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 - Fix async scrolling with background-attachment:fixed (40.57 KB, patch)
2012-09-07 03:25 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 (bonus) - Synchronise Layer Insert/Remove (40.44 KB, patch)
2012-09-07 09:34 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 1 - Fix async scrolling with background-attachment:fixed (41.01 KB, patch)
2012-09-08 03:11 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Review
Part 1 - Split backgrounds into multiple items per layer (31.04 KB, patch)
2012-09-10 12:03 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Review
Part 2 - Fix async scrolling with background-attachment:fixed (13.85 KB, patch)
2012-09-10 12:07 PDT, Chris Lord [:cwiiis]
roc: review-
Details | Diff | Review
Part 3 - Add a couple of tests for background layers (7.04 KB, patch)
2012-09-10 12:09 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Review
Part 2 - Fix async scrolling with background-attachment:fixed (13.89 KB, patch)
2012-09-11 03:02 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Part 2 - Fix async scrolling with background-attachment:fixed (16.27 KB, patch)
2012-09-11 05:14 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Review
Part 4 - Add a test to make sure that fixed backgrounds aren't constantly redrawn (2.89 KB, patch)
2012-09-12 10:15 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Review

Description Michael Comella (:mcomella) 2012-08-28 16:55:56 PDT
Created attachment 656263 [details]
Textured background

1) Open Firefox
2) Go to http://bungie.net
3) Scroll the page vertically

Expected: The content scrolls over the background
Actual: The background moves with the content, revealing white space under the content. Eventually the background seems to "catch up" and puts itself behind the content again.

Clicking the central link on the page (https://www.bungie.net/News/content.aspx?type=topnews&cid=32142) loads many news items which gives a larger space to scroll and play around in.

This behavior seems to be worse while the page is loading, which is especially true for the link in the paragraph above since it has a lot of content.

When I load the page in portrait mode on the phone, you can see a textured background that I cannot see on desktop (but this may just be a difference in displays – I can't see the texture in my screenshots which I will attach). Zooming into the page shows the background moving. In mobile Chrome, however, the background does not move while zooming (Chrome does not have these scrolling artifact issues).

Zooming out such that the content window is smaller than the screen also seems to cause white artifacts, those these artifacts are slightly different in appearance.
Comment 1 Michael Comella (:mcomella) 2012-08-28 16:56:40 PDT
Created attachment 656264 [details]
Textured background after zooming
Comment 2 Michael Comella (:mcomella) 2012-08-28 16:57:25 PDT
Created attachment 656267 [details]
Page appearance after background is scrolled away

The page content is still loading which is why it does not appear.
Comment 3 Michael Comella (:mcomella) 2012-08-28 16:57:49 PDT
Thinking this should have been in the panning/zooming component.
Comment 4 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-08-29 14:10:35 PDT
Adding Cwiiis as this seems to be related to fixed-position layers.
Comment 5 Chris Lord [:cwiiis] 2012-08-30 01:00:31 PDT
You can see this problem more pronounced on http://moonintern.com/ - The background stays fixed when zoomed out fully, but zoom in slightly and it starts moving with the page.

The problem seems to be here that we don't make any effort to separate out background-attachment: fixed into their own layers, so once you zoom in, for whatever reason it doesn't get its own layer and thus doesn't get marked as a fixed item (as it gets drawn on the same layer as non-fixed items).

The way to fix this would be to use nsDisplayFixedPosition for background-attachment: fixed, as well as for position: fixed items. I think this ought to be relatively simple, having a look.
Comment 6 Chris Lord [:cwiiis] 2012-08-30 06:15:13 PDT
What I stated in comment #5, but regardless I have a patch that fixes background-attachment:fixed scrolling - unfortunately, it doesn't yet work on this site as its layout is a bit more complicated than that - will see if it's easily fixed, otherwise I'll open a separate bug for attachment:fixed (as this fixes other sites, like http://moonintern.com/)
Comment 7 Chris Lord [:cwiiis] 2012-08-30 07:27:31 PDT
Ugh, ok, the difficulty with bungie.net is that the main background actually contains two background elements (wish I'd noticed this earlier..), one which is fixed and one which isn't.

To make that work, we need to separate out fixed background layers into different items - they can't share items, as they need to be on separate layers. The easiest thing to do here is probably add a flag to nsDisplayBackground that determines whether it draws fixed, non-fixed or both, and create two items as necessary in nsCanvasFrame, wrapping one in an nsDisplayFixedPosition so that it doesn't end up sharing its layer.

Will give this a shot.
Comment 8 Chris Lord [:cwiiis] 2012-08-30 08:21:32 PDT
(In reply to Chris Lord [:cwiiis] from comment #7)
> Ugh, ok, the difficulty with bungie.net is that the main background actually
> contains two background elements (wish I'd noticed this earlier..), one
> which is fixed and one which isn't.
> 
> To make that work, we need to separate out fixed background layers into
> different items - they can't share items, as they need to be on separate
> layers. The easiest thing to do here is probably add a flag to
> nsDisplayBackground that determines whether it draws fixed, non-fixed or
> both, and create two items as necessary in nsCanvasFrame, wrapping one in an
> nsDisplayFixedPosition so that it doesn't end up sharing its layer.
> 
> Will give this a shot.

Ok, not quite as easy as this either - the background layers need to be drawn in the right order of course - so my new plan is to allow a background display item to represent just a single background layer, and in the case of there being fixed layers, creating an item for each layer.

It might actually be nicer for an item to be able to represent a span of layers, for efficiency... I'll see how this goes. Unfortunately, this involves modifying the background drawing functions, but I don't think it's a large modification.
Comment 9 Chris Lord [:cwiiis] 2012-08-30 08:24:08 PDT
Another quick idea about the spans - this may be more easily done by implementing a TryMerge that merges if underlying frame and layer fixed/non-fixed status matches (and then storing the span and passing that to the drawing function).
Comment 10 Chris Lord [:cwiiis] 2012-08-30 10:48:40 PDT
So I've done as I said in comment #7 and that fixes this page... But for some reason, it also constantly redraws now... Getting there.
Comment 11 Chris Lord [:cwiiis] 2012-08-30 10:50:47 PDT
Ah of course, this is likely because I now have the same item type and frame item multiple places in the display-list... I guess I'll need to alter GetType...
Comment 12 Chris Lord [:cwiiis] 2012-08-30 10:57:58 PDT
(In reply to Chris Lord [:cwiiis] from comment #11)
> Ah of course, this is likely because I now have the same item type and frame
> item multiple places in the display-list... I guess I'll need to alter
> GetType...

(and by GetType, I meant GetPerFrameKey)
Comment 13 Chris Lord [:cwiiis] 2012-08-30 13:21:38 PDT
Just to note, I have this fixed, just refining the patches so that they're in a reasonable state to push to try and review.
Comment 14 Chris Lord [:cwiiis] 2012-08-31 08:16:57 PDT
Created attachment 657290 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

This fixes async scrolling only when all layers of a background are fixed.
Comment 15 Chris Lord [:cwiiis] 2012-08-31 08:20:41 PDT
Created attachment 657291 [details] [diff] [review]
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not

This fixes async scrolling for pages with a mixture of fixed and non-fixed background layers (with some constraints).

It also causes/exposes an invalidation bug on bungie.net, so even if it was r+ worthy, I won't be pushing it until I've tracked down the cause of that. I'm sure there are some very questionable design decisions in these patches too.
Comment 16 Chris Lord [:cwiiis] 2012-08-31 08:40:59 PDT
Created attachment 657307 [details] [diff] [review]
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not

Amended comment and fixed small error, both from earlier versions of the patch.
Comment 17 Chris Lord [:cwiiis] 2012-08-31 09:04:39 PDT
Created attachment 657315 [details] [diff] [review]
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not

Sorry, made sure it built this time.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-31 15:39:36 PDT
Comment on attachment 657315 [details] [diff] [review]
Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not

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

This looks like the right approach, but wouldn't it be simpler to *always* have nsDisplayBackground represent a single layer? I'm not worried about the overhead of multiple items per background ... multiple-backgrounds aren't common enough to worry about that.

::: layout/base/nsDisplayList.cpp
@@ +1361,5 @@
>        borderBox.ToNearestPixels(aFrame->PresContext()->AppUnitsPerDevPixel()));
>  }
>  
> +static bool
> +LayerCanHaveFixedBackground(const nsStyleBackground::Layer& layer,

aLayer
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-31 15:42:21 PDT
I think it would be better to do part 2 first and then do part 1 on top of that.
Comment 20 Chris Lord [:cwiiis] 2012-09-04 03:55:34 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Comment on attachment 657315 [details] [diff] [review]
> Part 2 - Fix async scrolling with multiple bg layers, some fixed, some not
> 
> Review of attachment 657315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like the right approach, but wouldn't it be simpler to *always*
> have nsDisplayBackground represent a single layer? I'm not worried about the
> overhead of multiple items per background ... multiple-backgrounds aren't
> common enough to worry about that.
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +1361,5 @@
> >        borderBox.ToNearestPixels(aFrame->PresContext()->AppUnitsPerDevPixel()));
> >  }
> >  
> > +static bool
> > +LayerCanHaveFixedBackground(const nsStyleBackground::Layer& layer,
> 
> aLayer

The act of merging I think is simple enough for it to be justified to do it. I realise I have some of the code in PaintBackgroundWithSC wrong (which I'll update in a revised patch). Once it's correct, there's pretty much no difference in terms of work done between merging and not merging, but I'd pick a simpler display list over marginally simpler merging. (but if you think it's better the other way round, I don't mind having it that way either)
Comment 21 Chris Lord [:cwiiis] 2012-09-04 03:56:41 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> I think it would be better to do part 2 first and then do part 1 on top of
> that.

So separate background layer items first, then the attachment-fixed stuff? Fair enough, I can do that.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-05 03:25:36 PDT
(In reply to Chris Lord [:cwiiis] from comment #20)
> The act of merging I think is simple enough for it to be justified to do it.
> I realise I have some of the code in PaintBackgroundWithSC wrong (which I'll
> update in a revised patch). Once it's correct, there's pretty much no
> difference in terms of work done between merging and not merging, but I'd
> pick a simpler display list over marginally simpler merging. (but if you
> think it's better the other way round, I don't mind having it that way
> either)

It's not just merging, but the fact that nsDisplayBackground can sometimes be multiple layers and sometimes just one, and all of its methods would need to be able to handle both cases. I think always splitting them is the way to go. It also means that when the bounds of the images are quite different (which is common), the individual display items have more accurate bounds, which is a good thing. Also, some of them can be opaque even if not all are.
Comment 23 Chris Lord [:cwiiis] 2012-09-05 07:49:48 PDT
Created attachment 658489 [details] [diff] [review]
Fix async scrolling with background-attachment:fixed

We discussed on IRC how to go about this, and this is my interpretation of the discussion.

In short;
- Forget nsDisplayFixedPosition, there's no need for it - FrameLayerBuilder will already separate out fixed items as necessary in FindThebesLayerFor. This fixes the invalidation bugs the first two patches introduced.
- Always separate out background layers, for the reasons mentioned.
- Fix nsDisplayBackground::ShouldFixToViewport to respect the clamping scroll-port (instead of just discarding the size check).

The patch is reduced enough that I don't think it really helps or makes sense to have it in two parts, so I've not bothered separating it.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-05 19:26:51 PDT
Comment on attachment 658489 [details] [diff] [review]
Fix async scrolling with background-attachment:fixed

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

::: layout/generic/nsFrame.cpp
@@ +1477,5 @@
> +
> +    // Background is themed or has no style background, just create a single
> +    // item.
> +    *aBackground = new (aBuilder) nsDisplayBackground(aBuilder, this);
> +    return aLists.BorderBackground()->AppendNewToTop(*aBackground);

I think we can just say that layer zero always paints the background color, and the theme background (if there is one), and also can handle the case where there's no image.

Then we don't need to support a -1 parameter that means "all layers".
Comment 25 Chris Lord [:cwiiis] 2012-09-06 09:31:21 PDT
Created attachment 658905 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

I haven't removed the layer = -1 part of this patch, as table cell frames still have a separate background element that doesn't inherit from nsDisplayBackground and I'll need to do something there (either adding multi-layer separately there, or making it a sub-class of nsDisplayBackground... Suggestions?)

This does replace mIsFirstLayerInSequence (which was calculated wrong anyway) with mIsFirstLayer. Ends up the clearing thing wasn't an issue, so I've removed all that code.

I've changed the background layer iteration macros to accept NULL (and in that case, run the loop once with i==0) - this lets me simplify the item creation in nsCanvasFrame and nsFrame. I haven't added a utility function to do this iteration as in all three cases where we'd want it to happen, different classes are created. With this macro change, at least the code is pretty slim.
Comment 26 Chris Lord [:cwiiis] 2012-09-06 09:33:32 PDT
Created attachment 658906 [details] [diff] [review]
Part 2 - Add a couple of tests for background layers

This adds two tests that test that background compositing is working correctly (for nsDisplayBackground and nsTableCellBackground). I should probably devise a third test for nsCanvasDisplayBackground...
Comment 27 Chris Lord [:cwiiis] 2012-09-06 09:36:17 PDT
My plan for a part 3 is to modify nsDisplayTableCellBackground to either be a sub-class of nsDisplayBackground or just reimplement the represent-a-layer bits (it's pretty tiny, so this may be a less dangerous path) and then remove the '-1 means all layers' code.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-06 14:31:55 PDT
Why don't we just leave table cell backgrounds alone? You can still remove "-1 means all layers" from nsDisplayBackground, just leave it in PaintBackgroundWithSC.
Comment 29 Chris Lord [:cwiiis] 2012-09-06 19:33:58 PDT
Created attachment 659089 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

Remove the -1 from nsDisplayBackground and make a few miscellaneous changes.
Comment 30 Chris Lord [:cwiiis] 2012-09-06 19:43:42 PDT
Created attachment 659095 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

Sorry, some left-over -1 badness in nsDisplayBackground::IsSingleFixedPositionImage and nsDisplayCanvasBackground::Paint.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-06 20:16:39 PDT
Comment on attachment 659095 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

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

::: layout/base/nsDisplayList.cpp
@@ +1956,4 @@
>    nsIScrollableFrame* scrollable = do_QueryFrame(rootScrollFrame);
> +  nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> +                    aBuilder->ToReferenceFrame(rootScrollFrame),
> +                    scrollable->GetScrollPositionClampingScrollPortSize());

Why the ClampingScrollPortSize? Why did you change this?

::: layout/base/nsDisplayList.h
@@ +1610,5 @@
>    virtual ~nsDisplayBackground();
>  
> +  // This will create and append new items for all the layers of the
> +  // background. If given, aBackground will be set with the address of the
> +  // last successfully added background item.

I think this should return the bottommost item, right? We want to return the one that has any theme/color.

@@ +1656,5 @@
>  
>    /* Used to cache mFrame->IsThemed() since it isn't a cheap call */
>    bool mIsThemed;
> +  /* true if the item paints only attachment-fixed layers of the background */
> +  bool mIsFixed;

The comment can change to say that the item represents a background-attachment:fixed layer.

@@ +1658,5 @@
>    bool mIsThemed;
> +  /* true if the item paints only attachment-fixed layers of the background */
> +  bool mIsFixed;
> +  /* true if the item will paint the lowest background layer */
> +  bool mIsFirstLayer;

More precise to say mIsBottommostLayer.

::: layout/generic/nsFrame.cpp
@@ +1451,5 @@
>  nsresult
>  nsFrame::DisplayBackgroundUnconditional(nsDisplayListBuilder*   aBuilder,
>                                          const nsDisplayListSet& aLists,
>                                          bool                    aForceBackground,
>                                          nsDisplayBackground**   aBackground)

Document that DisplayBackgroundUnconditional returns the bottommost background item if there is one (i.e. the one with the theme/background color, if any).
Comment 32 Chris Lord [:cwiiis] 2012-09-07 01:16:48 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Comment on attachment 659095 [details] [diff] [review]
> Part 1 - Fix async scrolling with background-attachment:fixed
> 
> Review of attachment 659095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +1956,4 @@
> >    nsIScrollableFrame* scrollable = do_QueryFrame(rootScrollFrame);
> > +  nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> > +                    aBuilder->ToReferenceFrame(rootScrollFrame),
> > +                    scrollable->GetScrollPositionClampingScrollPortSize());
> 
> Why the ClampingScrollPortSize? Why did you change this?

I assume this check is to see if scrolling would expose new parts of the fixed background (which due to clipping, looks really terrible and performs terribly too) - As it was, this meant that zooming in broke this check and would end up disabling async scrolling. Checking against the clamping scroll-port size fixes this on mobile and is functionally the same otherwise.

> ::: layout/base/nsDisplayList.h
> @@ +1610,5 @@
> >    virtual ~nsDisplayBackground();
> >  
> > +  // This will create and append new items for all the layers of the
> > +  // background. If given, aBackground will be set with the address of the
> > +  // last successfully added background item.
> 
> I think this should return the bottommost item, right? We want to return the
> one that has any theme/color.

It should, I was being lazy. Will update.

> @@ +1656,5 @@
> >  
> >    /* Used to cache mFrame->IsThemed() since it isn't a cheap call */
> >    bool mIsThemed;
> > +  /* true if the item paints only attachment-fixed layers of the background */
> > +  bool mIsFixed;
> 
> The comment can change to say that the item represents a
> background-attachment:fixed layer.

Ok.

> @@ +1658,5 @@
> >    bool mIsThemed;
> > +  /* true if the item paints only attachment-fixed layers of the background */
> > +  bool mIsFixed;
> > +  /* true if the item will paint the lowest background layer */
> > +  bool mIsFirstLayer;
> 
> More precise to say mIsBottommostLayer.

Sure.

> ::: layout/generic/nsFrame.cpp
> @@ +1451,5 @@
> >  nsresult
> >  nsFrame::DisplayBackgroundUnconditional(nsDisplayListBuilder*   aBuilder,
> >                                          const nsDisplayListSet& aLists,
> >                                          bool                    aForceBackground,
> >                                          nsDisplayBackground**   aBackground)
> 
> Document that DisplayBackgroundUnconditional returns the bottommost
> background item if there is one (i.e. the one with the theme/background
> color, if any).

Ok.
Comment 33 Chris Lord [:cwiiis] 2012-09-07 01:26:47 PDT
hmm, after reading what SetHasFixedItems actually does for the display-list, I realise it should only be set when the background will actually be fixed. Changing up the testing order a bit.
Comment 34 Chris Lord [:cwiiis] 2012-09-07 01:28:50 PDT
I also think that the IsFixedItem call should be public and FrameLayerBuilder should use it (and FrameLayerBuilder's special-case code should be moved into nsDisplayList) - this will fix some of the odd visibility we get with fixed items.
Comment 35 Chris Lord [:cwiiis] 2012-09-07 03:25:17 PDT
Created attachment 659194 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

Made the suggested changes.

Also changed IsFixedItem() in nsDisplayList.cpp to be a public method on nsDisplayListBuilder - This let me consolidate the code that decides this in one place, rather than duplicating it between FrameLayerBuilder and nsDisplayList.cpp. This may fix some invalidation bugs on fixed items, more likely on mobile than desktop.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-07 03:57:16 PDT
Comment on attachment 659194 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1810,2 @@
>      }
> +    bool isFixed = mBuilder->IsFixedItem(item, &activeScrolledRoot);

I don't think we should overwrite activeScrolledRoot as returned by the NO_COMPONENT_ALPHA path.

::: layout/base/nsDisplayList.cpp
@@ +1433,5 @@
> +          //     the viewport/is the viewport.
> +          nsIScrollableFrame* scrollable = do_QueryFrame(rootScrollFrame);
> +          nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> +                            aBuilder->ToReferenceFrame(rootScrollFrame),
> +                            scrollable->GetScrollPositionClampingScrollPortSize());

I think we probably should clip fixed backgrounds to the displayport and check the displayport here, but I guess that should be a separate change.

@@ +1437,5 @@
> +                            scrollable->GetScrollPositionClampingScrollPortSize());
> +
> +          if (bounds.Contains(scrollport)) {
> +            mIsFixed = true;
> +            aBuilder->SetHasFixedItems();

We need to call SetHasFixedItems unconditionally whenever there's a background-attachment:fixed item visible.

::: layout/base/nsDisplayList.h
@@ +302,5 @@
>    /**
> +   * Determines if this item is scrolled by content-document display-port
> +   * scrolling. aActiveScrolledRoot is an in-out parameter - it can be used
> +   * to specify what frame to treat as the active scrolled root, and will be
> +   * set to whichever frame is used for the active scrolled root.

I think aActiveScrolledRoot should be an out-parameter only.
Comment 37 Chris Lord [:cwiiis] 2012-09-07 09:17:44 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #36)
> Comment on attachment 659194 [details] [diff] [review]
> Part 1 - Fix async scrolling with background-attachment:fixed
> 
> Review of attachment 659194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1810,2 @@
> >      }
> > +    bool isFixed = mBuilder->IsFixedItem(item, &activeScrolledRoot);
> 
> I don't think we should overwrite activeScrolledRoot as returned by the
> NO_COMPONENT_ALPHA path.

Was just copying what it did before - seemed a bit weird to me too, if you don't think it's necessary then great :)

> ::: layout/base/nsDisplayList.cpp
> @@ +1433,5 @@
> > +          //     the viewport/is the viewport.
> > +          nsIScrollableFrame* scrollable = do_QueryFrame(rootScrollFrame);
> > +          nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> > +                            aBuilder->ToReferenceFrame(rootScrollFrame),
> > +                            scrollable->GetScrollPositionClampingScrollPortSize());
> 
> I think we probably should clip fixed backgrounds to the displayport and
> check the displayport here, but I guess that should be a separate change.

Not entirely sure what you mean by checking the displayport here, but separate change so no matter.

> @@ +1437,5 @@
> > +                            scrollable->GetScrollPositionClampingScrollPortSize());
> > +
> > +          if (bounds.Contains(scrollport)) {
> > +            mIsFixed = true;
> > +            aBuilder->SetHasFixedItems();
> 
> We need to call SetHasFixedItems unconditionally whenever there's a
> background-attachment:fixed item visible.

Ok, having looked at it further, this sounds reasonable.

> ::: layout/base/nsDisplayList.h
> @@ +302,5 @@
> >    /**
> > +   * Determines if this item is scrolled by content-document display-port
> > +   * scrolling. aActiveScrolledRoot is an in-out parameter - it can be used
> > +   * to specify what frame to treat as the active scrolled root, and will be
> > +   * set to whichever frame is used for the active scrolled root.
> 
> I think aActiveScrolledRoot should be an out-parameter only.

With the first change, that would negate the need for it to be in anyway, will do.
Comment 38 Chris Lord [:cwiiis] 2012-09-07 09:34:51 PDT
Created attachment 659282 [details] [diff] [review]
Part 1 (bonus) - Synchronise Layer Insert/Remove

Made the suggested changes.
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-07 15:21:49 PDT
Comment on attachment 659282 [details] [diff] [review]
Part 1 (bonus) - Synchronise Layer Insert/Remove

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1804,2 @@
>      nsIFrame* activeScrolledRoot;
> +    bool isFixed = mBuilder->IsFixedItem(item, &activeScrolledRoot);

No, we need to keep the old code that sets activeScrolledRoot when NO_COMPONENT_ALPHA is set. We just need to not overwrite that value with the result from IsFixedItem.
Comment 40 Chris Lord [:cwiiis] 2012-09-08 03:11:47 PDT
Created attachment 659467 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

Revert the overridden active scrolled root behaviour and add a comment as to why it's done. Removed the in-out parameter of IsFixedItem and just replaced it with two parameters.
Comment 41 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-08 04:10:42 PDT
Comment on attachment 659467 [details] [diff] [review]
Part 1 - Fix async scrolling with background-attachment:fixed

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

::: layout/base/nsDisplayList.h
@@ +304,5 @@
> +   * scrolling. aActiveScrolledRoot will be set to the active scrolled root
> +   * of the item. This may not necessarily correspond to the active scrolled
> +   * root of the item's underlying frame.
> +   * If specified, aOverrideActiveScrolledRoot will be treated as the active
> +   * scrolled root.

"and aActiveScrolledRoot will be set to aOverrideActiveScrolledRoot".
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-08 04:11:10 PDT
It would be nice if Part 1 could be split into "one nsDisplayBackground per layer" and "fixed-pos changes".
Comment 43 Chris Lord [:cwiiis] 2012-09-10 09:18:31 PDT
For reference, try looks good: https://tbpl.mozilla.org/?tree=Try&rev=5ef21aabaafd

Working on splitting and writing more test-cases.
Comment 44 Chris Lord [:cwiiis] 2012-09-10 12:03:59 PDT
Created attachment 659813 [details] [diff] [review]
Part 1 - Split backgrounds into multiple items per layer

Carrying r=roc.
Comment 45 Chris Lord [:cwiiis] 2012-09-10 12:07:42 PDT
Created attachment 659816 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

Requesting for review with one small change that I'm not 100% sure on;

Changed:

nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
                  aBuilder->ToReferenceFrame(rootScrollFrame),
                  scrollable->GetScrollPositionClampingScrollPortSize());

to:

nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
                  ToReferenceFrame(),
                  scrollable->GetScrollPositionClampingScrollPortSize());

I found that the origin of the scrollport didn't match the bounds origin for some pages, and this fixed it and seems ok to me, but I'd like to make sure.
Comment 46 Chris Lord [:cwiiis] 2012-09-10 12:09:07 PDT
Created attachment 659818 [details] [diff] [review]
Part 3 - Add a couple of tests for background layers

Remove the empty onload handler, as pointed out on IRC.

(will devise further tests in subsequent patches)
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-10 15:07:13 PDT
Comment on attachment 659816 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

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

::: layout/base/nsDisplayList.cpp
@@ +1434,5 @@
> +            //     the viewport/is the viewport.
> +            nsIScrollableFrame* scrollable = do_QueryFrame(rootScrollFrame);
> +            nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> +                              ToReferenceFrame(),
> +                              scrollable->GetScrollPositionClampingScrollPortSize());

I don't think this is right. The previous code was correct; GetScrollPortRect returns a rect relative to the top-left of scrollable's border-box, and ToReferenceFrame(rootScrollFrame) maps that to the displaylist's reference frame.
Comment 48 Chris Lord [:cwiiis] 2012-09-11 03:02:14 PDT
Created attachment 660022 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

ok, this time:

- ToReferenceFrame(),
+ aBuilder->ToReferenceFrame(scrollable->GetScrolledFrame()),

I think this is still correct(?), but fixes the issue for me and also seems to fix a lot of cases where a fixed background was constantly redrawn when scrolled (which makes sense).
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-11 03:52:32 PDT
I meant what I said in comment #47. The old code is correct.

Maybe you should just enable mIsFixed for nsDisplayCanvasBackground? That would make this bounds-check unnecessary.
Comment 50 Chris Lord [:cwiiis] 2012-09-11 04:54:10 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> I meant what I said in comment #47. The old code is correct.
> 
> Maybe you should just enable mIsFixed for nsDisplayCanvasBackground? That
> would make this bounds-check unnecessary.

Better idea.
Comment 51 Chris Lord [:cwiiis] 2012-09-11 05:14:04 PDT
Created attachment 660040 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

Do as suggested.
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-11 05:47:46 PDT
Comment on attachment 660040 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

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

::: layout/base/nsDisplayList.cpp
@@ +1440,5 @@
> +              nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> +                                aBuilder->ToReferenceFrame(rootScrollFrame),
> +                                scrollable->GetScrollPositionClampingScrollPortSize());
> +
> +              mIsFixed = bounds.Contains(scrollport);

Should we have this code path at all? I suspect not. Most uses of background-attachment:fixed have it on the canvas background.

If we take this out. then we can set mIsFixed to false in the nsDisplayBackground constructor and set mIsFixed appropriately in the nsDisplayCanvasBackground constructor, so we don't need a new parameter either.

I'm not sure what kinds of bugs you had with this code path, but it sounds to me like this code path is fragile in some way.
Comment 53 Chris Lord [:cwiiis] 2012-09-11 05:55:46 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> Comment on attachment 660040 [details] [diff] [review]
> Part 2 - Fix async scrolling with background-attachment:fixed
> 
> Review of attachment 660040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsDisplayList.cpp
> @@ +1440,5 @@
> > +              nsRect scrollport(scrollable->GetScrollPortRect().TopLeft() +
> > +                                aBuilder->ToReferenceFrame(rootScrollFrame),
> > +                                scrollable->GetScrollPositionClampingScrollPortSize());
> > +
> > +              mIsFixed = bounds.Contains(scrollport);
> 
> Should we have this code path at all? I suspect not. Most uses of
> background-attachment:fixed have it on the canvas background.
> 
> If we take this out. then we can set mIsFixed to false in the
> nsDisplayBackground constructor and set mIsFixed appropriately in the
> nsDisplayCanvasBackground constructor, so we don't need a new parameter
> either.
> 
> I'm not sure what kinds of bugs you had with this code path, but it sounds
> to me like this code path is fragile in some way.

This would be a reasonable thing to do, though I would like to fix non-canvas fixed backgrounds at some point. I guess when I get round to that, I can move the code back then, and then this bounds check would be unnecessary at that point anyway.

Let me try this out and see how it does.
Comment 54 Chris Lord [:cwiiis] 2012-09-11 06:59:38 PDT
(In reply to Chris Lord [:cwiiis] from comment #53)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #52)
> Let me try this out and see how it does.

So, it makes the code read much nicer, but it breaks Bungie.net - given this bug is about fixing that site, I guess we need to do it in nsDisplayBackground and have this check :/
Comment 55 Chris Lord [:cwiiis] 2012-09-11 07:04:48 PDT
So as it is, my current patch stands - let me know if there's anything you'd like fixed/tidied. I'm going to try writing some tests to ensure this stays fixed.
Comment 56 Chris Lord [:cwiiis] 2012-09-12 10:15:37 PDT
Created attachment 660501 [details] [diff] [review]
Part 4 - Add a test to make sure that fixed backgrounds aren't constantly redrawn

This tests that fixed backgrounds aren't constantly redrawn when scrolling. Fails on current central and if you change nsDisplayCanvasBackground to not pass the aSkipFixedItemBoundsCheck flag.
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 15:10:05 PDT
(In reply to Chris Lord [:cwiiis] from comment #53)
> This would be a reasonable thing to do, though I would like to fix
> non-canvas fixed backgrounds at some point.

The question is whether there are non-canvas fixed background that cover the whole viewport. I would guess that's pretty rare, and if they don't cover the whole viewport this optimization doesn't work very well.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 15:21:43 PDT
Comment on attachment 660040 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

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

OK
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 15:22:27 PDT
Comment on attachment 660501 [details] [diff] [review]
Part 4 - Add a test to make sure that fixed backgrounds aren't constantly redrawn

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

great!
Comment 62 Andreea Pod 2012-09-19 01:58:43 PDT
The background still scrolls with the content revealing white space when zooming in and zooming out on the main page: bungie.net.

Build: Firefox 18.0a1 (2012-09-18)
Device: Samsung Galaxy Nexus (Android 4.1.1), Samsung Galaxy Note (Android 4.0.4)
Comment 63 Chris Lord [:cwiiis] 2012-09-19 03:26:58 PDT
If this is broken, it's regressed - Perhaps by bug 788877. I'll verify and see about preparing a fix.
Comment 64 Chris Lord [:cwiiis] 2012-09-19 04:30:27 PDT
(In reply to Andreea Pod from comment #62)
> The background still scrolls with the content revealing white space when
> zooming in and zooming out on the main page: bungie.net.
> 
> Build: Firefox 18.0a1 (2012-09-18)
> Device: Samsung Galaxy Nexus (Android 4.1.1), Samsung Galaxy Note (Android
> 4.0.4)

This works correctly in Nightly based on 0d3b17a88d5f (2012-09-17), and on a local build from m-c, 80499f04e875.

I suggest that perhaps you're not identifying what the background actually is? The background isn't the thing behind the picture of the planet, but behind that - if you zoom in, you can see it scrolling asynchronously behind the translucent box at the bottom of the page.

If you still think this is broken, please attach a video and reproduction steps so that I can confirm.
Comment 65 Andreea Pod 2012-09-19 05:37:07 PDT
You can see here: http://www.youtube.com/watch?v=-xbHscPmptg, but maybe this is something else. Should I file a new bug?
Comment 66 Chris Lord [:cwiiis] 2012-09-19 05:51:54 PDT
(In reply to Andreea Pod from comment #65)
> You can see here: http://www.youtube.com/watch?v=-xbHscPmptg, but maybe this
> is something else. Should I file a new bug?

This is something else. This is the problem that covered areas of fixed position elements are not rendered (and so can be exposed when async zooming/scrolling). Please file a new bug and Cc me, this may be worth fixing, but may gate on DLBI (I'd need to look at it to know).

There's also a separate bug that sometimes the page will render outside of its bounds when in overscroll, but I can't quite tell if that's being exhibited here.
Comment 67 Andreea Pod 2012-09-19 07:35:18 PDT
(In reply to Chris Lord [:cwiiis] from comment #66)
> (In reply to Andreea Pod from comment #65)
> > You can see here: http://www.youtube.com/watch?v=-xbHscPmptg, but maybe this
> > is something else. Should I file a new bug?
> 
> This is something else. This is the problem that covered areas of fixed
> position elements are not rendered (and so can be exposed when async
> zooming/scrolling). Please file a new bug and Cc me, this may be worth
> fixing, but may gate on DLBI (I'd need to look at it to know).
> 
> There's also a separate bug that sometimes the page will render outside of
> its bounds when in overscroll, but I can't quite tell if that's being
> exhibited here.

Ok, filed Bug 792415 for that problem. Marking this as verified fixed.

Build: Firefox 18.0a1 (2012-09-19)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Note You need to log in before you can comment on or make changes to this bug.