Closed Bug 786502 Opened 12 years ago Closed 12 years ago

Static background on Bungie.net appears to scroll away with the content

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Version:
18 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox18 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 18
Tracking Flags:
Tracking Status
firefox18 --- verified

People

(Reporter: mcomella, Assigned: cwiiis)

References

(
URL
)

Details

Attachments

(7 files, 14 obsolete files)

Textured background
477.29 KB, image/png
Details
Textured background after zooming
390.20 KB, image/png
Details
Page appearance after background is scrolled away
671.14 KB, image/png
Details
Part 1 - Split backgrounds into multiple items per layer
31.04 KB, patch
cwiiis
: review+
Details | Diff | Splinter Review
Part 3 - Add a couple of tests for background layers
7.04 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 2 - Fix async scrolling with background-attachment:fixed
16.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 4 - Add a test to make sure that fixed backgrounds aren't constantly redrawn
2.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached image 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.
The page content is still loading which is why it does not appear.
Thinking this should have been in the panning/zooming component.
Component: General → Graphics, Panning and Zooming
Adding Cwiiis as this seems to be related to fixed-position layers.
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.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
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/)
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.
(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.
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).
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.
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...
(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)
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.
This fixes async scrolling only when all layers of a background are fixed.
Attachment #657290 - Flags: review?(roc)
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.
Attachment #657291 - Flags: review?(roc)
Amended comment and fixed small error, both from earlier versions of the patch.
Attachment #657291 - Attachment is obsolete: true
Attachment #657291 - Flags: review?(roc)
Attachment #657307 - Flags: review?(roc)
Sorry, made sure it built this time.
Attachment #657307 - Attachment is obsolete: true
Attachment #657307 - Flags: review?(roc)
Attachment #657315 - Flags: review?(roc)
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
I think it would be better to do part 2 first and then do part 1 on top of that.
(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)
(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.
(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.
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.
Attachment #657290 - Attachment is obsolete: true
Attachment #657315 - Attachment is obsolete: true
Attachment #657290 - Flags: review?(roc)
Attachment #657315 - Flags: review?(roc)
Attachment #658489 - Flags: review?(roc)
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".
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.
Attachment #658489 - Attachment is obsolete: true
Attachment #658489 - Flags: review?(roc)
Attachment #658905 - Flags: review?(roc)
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...
Attachment #658906 - Flags: review?(roc)
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.
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.
Remove the -1 from nsDisplayBackground and make a few miscellaneous changes.
Attachment #658905 - Attachment is obsolete: true
Attachment #658905 - Flags: review?(roc)
Attachment #659089 - Flags: review?(roc)
Sorry, some left-over -1 badness in nsDisplayBackground::IsSingleFixedPositionImage and nsDisplayCanvasBackground::Paint.
Attachment #659089 - Attachment is obsolete: true
Attachment #659089 - Flags: review?(roc)
Attachment #659095 - Flags: review?(roc)
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).
(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.
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.
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.
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.
Attachment #659095 - Attachment is obsolete: true
Attachment #659095 - Flags: review?(roc)
Attachment #659194 - Flags: review?(roc)
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.
(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.
Made the suggested changes.
Attachment #659194 - Attachment is obsolete: true
Attachment #659194 - Flags: review?(roc)
Attachment #659282 - Flags: review?(roc)
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.
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.
Attachment #659282 - Attachment is obsolete: true
Attachment #659282 - Flags: review?(roc)
Attachment #659467 - Flags: review?(roc)
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".
Attachment #659467 - Flags: review?(roc) → review+
It would be nice if Part 1 could be split into "one nsDisplayBackground per layer" and "fixed-pos changes".
For reference, try looks good: https://tbpl.mozilla.org/?tree=Try&rev=5ef21aabaafd

Working on splitting and writing more test-cases.
Attachment #659467 - Attachment is obsolete: true
Carrying r=roc.
Attachment #659813 - Flags: review+
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.
Attachment #659816 - Flags: review?(roc)
Remove the empty onload handler, as pointed out on IRC.

(will devise further tests in subsequent patches)
Attachment #658906 - Attachment is obsolete: true
Attachment #658906 - Flags: review?(roc)
Attachment #659818 - Flags: review?(roc)
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.
Attachment #659816 - Flags: review?(roc) → review-
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).
Attachment #659816 - Attachment is obsolete: true
Attachment #660022 - Flags: review?(roc)
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.
(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.
Do as suggested.
Attachment #660022 - Attachment is obsolete: true
Attachment #660022 - Flags: review?(roc)
Attachment #660040 - Flags: review?(roc)
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.
(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.
(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 :/
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.
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.
Attachment #660501 - Flags: review?(roc)
(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 on attachment 660040 [details] [diff] [review]
Part 2 - Fix async scrolling with background-attachment:fixed

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

OK
Attachment #660040 - Flags: review?(roc) → review+
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!
Attachment #660501 - Flags: review?(roc) → review+
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If this is broken, it's regressed - Perhaps by bug 788877. I'll verify and see about preparing a fix.
(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.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You can see here: http://www.youtube.com/watch?v=-xbHscPmptg, but maybe this is something else. Should I file a new bug?
(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.
(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
Status: RESOLVED → VERIFIED
status-firefox18: --- → verified
Depends on: 797021
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: