371864 - Background images don't load until scrolled into the viewport

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Steps to Reproduce:

1. In a trunk build, load http://www.tv.com/prison-break/show/31635/official-discussion-thread-andquotwashandquot-spoilers/topic/14245-671484/msgs.html?tag=board_topics;title;0
2. Scroll down slowly, paying attention to the user icons, which are background images, "pop-up" in the page; that is, they aren't rendered until scrolled into the viewport.

Expected Results:
Images should appear, and not "pop-up."

Actual Results:
Images only render/appear when scrolled into the viewport.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So this is basically a regression from bug 370379, as far as I can tell.  We used to get the background style struct (and thus start the background image load) at frame construction time (or more precisely when we called FrameNeedsView).  Now we probably don't get the struct until paint time, so that's when the images start to load....

Comment 2

[Eli Friedman]

Ugh, why exactly aren't dependencies like this documented?  (Or maybe was it accidental...)

Hmm, so what do we want to do here?  If we wanted a horrible hack, we could just get the style struct in FrameNeedsView even though we don't need it...  Any proposals for a non-hack solution?

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't think any of us stopped to think about it.  It wasn't a purposeful dependency by any means.

In nsCSSFrameConstructor::ConstructFrameInternal we just call GetStyleVisibility(), precisely because it has side-effects.  Maybe we should do the same for backgrounds.  And maybe we should simply not worry about it and stick with the current behavior.

Comment 4

[Jesse Ruderman]

On Mac, if I grab the scrollbar thumb and scroll down, the user icons don't appear until I let go of the scrollbar!

Comment 5

[Jesse Ruderman]

I noticed another bug on this page, so I filed bug 371903, "overflow: auto causes background to bleed through while scrolling".

Comment 6

[Eli Friedman]

(In reply to comment #3)
> I don't think any of us stopped to think about it.  It wasn't a purposeful
> dependency by any means.
> 
> In nsCSSFrameConstructor::ConstructFrameInternal we just call
> GetStyleVisibility(), precisely because it has side-effects.  Maybe we should
> do the same for backgrounds.  And maybe we should simply not worry about it and
> stick with the current behavior.

The issue is the the "current behavior" depends on calling FrameNeedsView, which  will be going away once we finish zapping views completely.  ConstructFrameInternal would be a reasonable place for it?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, by "current behavior" I meant "the behavior we have right now".  So effectively wontfixing this bug.

I do think that ConstructFrameInternal is a perfectly good place to call this.

Comment 8

[Eli Friedman]

Attachment: Patch

I think this should behave correctly in all the cases we care about... and the worst that could happen if it messes up is that we load the wrong background and have to load the right one when we paint.

Comment 9

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

Do we need the FindBackground call? Shouldn't the background load for "body background propagates to viewport" be triggered by the GetStyleBackground() for the body element itself?

Comment 10

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

Yeah, I don't see any need to do the FindBackground -- in fact, it might be harmful, since you might do FindBackground for the canvas before the body element is loaded.

Any background that will be found by FindBackground will be the background from some style context.  So why not just add a GetStyleBackground call right next to the GetStyleVisibility call?

Comment 11

[Eli Friedman]

There is the edge case of someone setting the root content or body to display: none, but I guess we don't really care... okay, I'll change it.

(In reply to comment #10)
> Yeah, I don't see any need to do the FindBackground -- in fact, it might be
> harmful, since you might do FindBackground for the canvas before the body
> element is loaded.

It would be a bug if it were harmful... we can paint a document before its body loads.

Comment 12

[Eli Friedman]

Oh wait, I didn't realize I posted a diff where I did both :( Obviously one or the other would be sufficient.

Comment 13

[Eli Friedman]

Attachment: Patch v2

Comment 14

[Eli Friedman]

Checked in.

Comment 15

[Jeff Walden [:Waldo]]

I have no idea whether this can be automatically tested, but we should at least try to think of a way to do so.

Comment 16

[Martijn Wargers (dead)]

There is a pref to fire onload when background images are set, see bug 253851.
I think that could be used for testing (it seems that pref might even be turned on for 1.9).

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2007-03-01-10 of SeaMonkey trunk on Windows XP.  Thanks for the quick turnaround!