618722 - Incorrect rendering of background-attachment:fixed when opacity<1

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[gadjo]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101212 Firefox/4.0b8pre
Build Identifier: 

When element has background-attachment:fixed, background-repeat: repeat-x and opacity<1 it is randomly (?) repeated also in y axis. Testcase and screenshots will be attached

Reproducible: Always

Steps to Reproduce:
1. Open the testcase
2. Scroll page up/down
Actual Results:  
Background is repeated in y-axis

Expected Results:  
Background should be repeated only in x-axis

Comment 2

[gadjo]

Attachment: test case

Scroll the page up and down to see the bug

Comment 3

[gadjo]

Attachment: screenshot

Upper part shows the expected behavious, the lower one shows the behaviour after scrolling down and up
After switching to other tab and then switching back to the test case tab it looks ok

Comment 4

[gadjo]

More info - the bug doesn't show after removing "div { opacity: 0.5 }" or "body { margin: 0 }" rule

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also see this on Linux.  Probably a regression from layers work.

Comment 6

[Alice0775 White]

Regression window(cached hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/92339b84d089
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre ID:20100715145415
Fails:
http://hg.mozilla.org/mozilla-central/rev/e1d7fd5255fd
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre ID:20100715152722
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=92339b84d089&tochange=e1d7fd5255fd

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This reminds me very much of bug 612459.  That bug has a regression range but this one has a better testcase.

Comment 8

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

Attachment: fix

This fixes it. I'm still trying to write a reliable test.

Comment 9

[Timothy Nikkel (:tnikkel)]

Looks like you uploaded the wrong patch here.

Comment 10

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

Attachment: real patch

Comment 11

[My.Other.Mailbox]

I've got 2 reliable tests for you:

1.  Using the Stylish extension, create a user style with this simple CSS:

    @namespace url(http://www.w3.org/1999/xhtml);
    @-moz-document domain("facebook.com") 
    {
    .UIStory {background:transparent !important; opacity:0.25 !important}

    body{
    background-image: url("http://i43.tinypic.com/2jb0c3d.jpg") !important;
    background-repeat:no-repeat !important;
    background-position:center !important;
    background-attachment:fixed !important;
    }
    }


Then visit any Facebook wall with this user style activated... other portions of the page will be copied under the posts.

2.  Same thing but using GreaseMonkey (to prove it's not Stylish causing it)...  Create a GM script with this content:

// ==UserScript==
// @name           TESTER
// @namespace      http://userstyles.org
// @description    overlay test
// @include        http://facebook.com/*
// @include        https://facebook.com/*
// @include        http://*.facebook.com/*
// @include        https://*.facebook.com/*
// ==/UserScript==
(function() {
var css = ".UIStory {background:transparent !important; opacity:0.25 !important}\nbody{background-image: url(http://i43.tinypic.com/2jb0c3d.jpg) !important;background-repeat:no-repeat !important;background-position:center !important;background-attachment:fixed !important;}";
if (typeof GM_addStyle != "undefined") {
	GM_addStyle(css);
} else if (typeof PRO_addStyle != "undefined") {
	PRO_addStyle(css);
} else if (typeof addStyle != "undefined") {
	addStyle(css);
} else {
	var heads = document.getElementsByTagName("head");
	if (heads.length > 0) {
		var node = document.createElement("style");
		node.type = "text/css";
		node.appendChild(document.createTextNode(css));
		heads[0].appendChild(node); 
	}
}
})();


and then visit a Facebook wall with the script active... same thing, copies of portions of the page underlying posts.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 503078 [details] [diff] [review]
real patch

>background-attachment:fixed display items rendered via temporary layer managers can't have their scrolling taken care of via the layer system

Here is a dumb question: why not?

Comment 13

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

Because they don't have retained layers whose offsets we can manipulate to scroll.

Comment 14

[Timothy Nikkel (:tnikkel)]

Do they not get assigned to thebes layers the same way that display items without temporary layer managers do?

Comment 15

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

They do, but those layers aren't retained.

The problem here is in InvalidateFixedBackgroundFramesFromList:
      if (item->IsFixedAndCoveringViewport(aBuilder)) {
        // FrameLayerBuilder takes care of scrolling these
The idea is that such items will have been placed in their own layer, and FrameLayerBuilder will set the offset on the layer to account for scrolling (typically by making it invariant when we scroll!), and that will take care of scrolling without us needing to invalidate any part of a ThebesLayer.

But when an item is drawn through a temporary LayerManager, that assumption is not true. The item has been drawn into some ThebesLayer and what it rendered will be scrolled along with the item that induced the temporary layer manager (e.g., an element with a transform not marked active).

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to comment #15)
> They do, but those layers aren't retained.

The layers inside the temp layer manager aren't retained, but the result of drawing the entire temp layer manager is retained in whatever thebes layer the item was assigned to.

> The problem here is in InvalidateFixedBackgroundFramesFromList:
>       if (item->IsFixedAndCoveringViewport(aBuilder)) {
>         // FrameLayerBuilder takes care of scrolling these
> The idea is that such items will have been placed in their own layer, and
> FrameLayerBuilder will set the offset on the layer to account for scrolling
> (typically by making it invariant when we scroll!), and that will take care of
> scrolling without us needing to invalidate any part of a ThebesLayer.
> 
> But when an item is drawn through a temporary LayerManager, that assumption is
> not true. The item has been drawn into some ThebesLayer and what it rendered
> will be scrolled along with the item that induced the temporary layer manager
> (e.g., an element with a transform not marked active).

I don't see why the non-temp layer manager case doesn't apply to the temp layer manager case. In both cases the result of drawing the item ends up in a thebes layer with the same active scrolled root. The transform on this layer should be adjusted the same if it contains items that have or do not have temp layer managers. What am I missing?

Comment 17

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

ContainerState::ProcessDisplayItems has this:

      nsIFrame* activeScrolledRoot =
        nsLayoutUtils::GetActiveScrolledRootFor(f, mBuilder->ReferenceFrame());
      if (item->IsFixedAndCoveringViewport(mBuilder)) {
        // Make its active scrolled root be the active scrolled root of
        // the enclosing viewport, since it shouldn't be scrolled by scrolled
        // frames in its document. InvalidateFixedBackgroundFramesFromList in
        // nsGfxScrollFrame will not repaint this item when scrolling occurs.
        nsIFrame* viewportFrame =
          nsLayoutUtils::GetClosestFrameOfType(f, nsGkAtoms::viewportFrame);
        NS_ASSERTION(viewportFrame, "no viewport???");
        activeScrolledRoot =
          nsLayoutUtils::GetActiveScrolledRootFor(viewportFrame, mBuilder->ReferenceFrame());

So in the simple retained case, the fixed item ends being drawn into a ThebesLayer whose active scrolled root is the viewport. But in the temp-layer-manager case, the fixed item probably ends up being drawn into a ThebesLayer whose active scrolled root is NOT the viewport (it's probably the root scrolled frame).

Comment 18

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

The temp layer manager will actually build a layer tree where the fixed item is assigned to a ThebesLayer whose active scrolled root is the viewport, but then that entire layer subtree is mashed together and drawn into the single ThebesLayer containing the nsDisplayTransform (or whatever), whose active scrolled root is not the viewport.

Comment 19

[Timothy Nikkel (:tnikkel)]

Ah ok, so the item with the temp layer manager has the scrolled frame as active scrolled root, but an item under that item has the viewport as its active scrolled root.

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 503078 [details] [diff] [review]
real patch

Doesn't seem like it would be too hard to write a reftest for this.

Comment 21

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

http://hg.mozilla.org/mozilla-central/rev/84cebd5b0f93
http://hg.mozilla.org/mozilla-central/rev/1431568b3e1b

Comment 22

[Alex Lakatos[:AlexLakatos]]

Verified on Build identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b12) Gecko/20100101 Firefox/4.0b12