331458 - Background image only displays top half

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Neil Lee [:neilio]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060322 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060322 Camino/1.2+

In the latest Camino nightly, if I go to http://www.hushboom.com/ I get only 1/2 of the background image displaying - the bottom half is black. If I switch away to another tab and come back, however, the background image displays correct.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.hushboom.com/
2. Watch the background image load half-way and then stop.
3. Create a new tab and make it active.
4. Switch back to the first tab to see the background image fully displayed *without a page refresh*.

Comment 1

[Neil Lee [:neilio]]

Attachment: Screenshot of issue w/ only top half of background displaying

Comment 2

[Chris Lawson (gone)]

FWIW, this works great in 1.0.

cl

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Someone have a recent trunk Firefox to test this with, and/or any idea of when it broke (my trunk Gecko bits are about a week old, so approx that long at least)?

Comment 4

[:Mook]

Also happens on Windows; Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060325 Firefox/1.6a1

Cairo or not does not affect this bug.

Regression range seems to be around 2006-03-15-06 to 2006-03-15-12 (the second date provided by Mossop over IRC, I tested 2006-03-16-05).  This points to dbaron, so CCing him :)

Comment 5

[:Mook]

Attachment: Testcase - open in browser, then open context menu somewhere near the bottom away from the text. The exposed area is not repainted.

Comment 6

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

So the problem is that PlaceScrollArea doesn't set NS_FRAME_OUTSIDE_CHILDREN (preferably by using FinishAndStoreOverflow) on the scrolledFrame (in this case, the canvas), so its parent skips it during the paint because the intersection of the dirty rect and the scrolled area is empty.

But when I fix (I tried both just setting the bit and using FinishAndStoreOverflow) that the background doesn't paint at all on the initial paint.  It fixes the repaint bug, though.

Comment 7

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

er, intersection of the dirty rect and the **child's overflow area**

Comment 8

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

(In reply to comment #6)
> So the problem is that PlaceScrollArea doesn't set NS_FRAME_OUTSIDE_CHILDREN
> (preferably by using FinishAndStoreOverflow) on the scrolledFrame (in this
> case, the canvas), so its parent skips it during the paint because the
> intersection of the dirty rect and the scrolled area is empty.

I see...

> But when I fix (I tried both just setting the bit and using
> FinishAndStoreOverflow) that the background doesn't paint at all on the
> initial paint.  It fixes the repaint bug, though.

Nothing springs to mind...

Comment 9

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

Making nsImageLoader invalidate the canvas correctly did spring to mind, but that didn't help either.

Comment 10

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

Then again, maybe I just wasn't testing right.  I can't reproduce either problem now, although the background image was loading more slowly yesterday, which made it easier to see the second problem...

Comment 11

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

Attachment: patch

Comment 12

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

Checked in to trunk.

Comment 13

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060326 Firefox/1.6a1 ID:2006032614 (non-cairo)

I'm still seeing this.

1. Load testcase. If all background is visible, ctrl-f5.
2. Right click somewhere on the page
3. Right click in another area. The previous area where the context menu was now shows the background that should be present.

Repo steps in comment 0 also still work for me.

Comment 14

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060326 Firefox/1.6a1 ID:2006032616 (NON-CAIRO)

not fixed yet.
http://img95.imageshack.us/img95/5237/as8ji.jpg

Comment 15

[Hideo Saito]

I think that it is necessary to extend a scrolled area to scroll area for scrolledFrame, as the previous code did.

--- ./nsGfxScrollFrame.cpp2	Sun Mar 26 22:46:06 2006
+++ ./nsGfxScrollFrame.cpp	Tue Mar 28 23:46:03 2006
@@ -675,8 +675,9 @@ nsHTMLScrollFrame::PlaceScrollArea(const
 
   nsRect scrolledArea;
   scrolledArea.UnionRect(mInner.GetScrolledRect(aState.mScrollPortRect.Size()),
                          nsRect(nsPoint(0,0), aState.mScrollPortRect.Size()));
+  scrolledFrame->SetRect(scrolledArea);
 
   // Note that making the view *exactly* the size of the scrolled area
   // is critical, since the view scrolling code uses the size of the
   // scrolled view to clamp scroll requests.

Comment 16

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

The reason we can't do that anymore is that it's much harder to do that when the overflow is to the left or top.  So, instead, I want to make everything work without that.

Comment 17

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

Also, after my patch, I can't see the bug on Linux anymore.  Do other people see the bug on Linux, or is it now Windows-only?  Or do I have some other change in my tree?

Comment 18

[Hideo Saito]

I saw this problem with testcase on my PowerPC-Linux although applying posted patch. It seems that it is easy to see in a super-reload(reload with Shift-key).

Comment 19

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

I see the bug on initial load, but not on context-menu popup. So I think David's patch fixed a bug, but not the originally reported bug :-). It looks to me like the imageloader invalidation isn't working yet...

Comment 20

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

To be specific, on shift-ctrl-R, the document's body gets invalidated when the image has loaded but the canvas is not invalidated.

Comment 21

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

Yeah, I do still see the initial load problem when the image loads slowly enough.  So the steps to repro for the reduced testcase really weren't the steps to reproduce for the original bug, it seems.

Comment 22

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

The problem is that the canvas frame has a view, so the nsIFrame::Invalidate calls UpdateView on a view that's smaller than the scrolled view, which clips the invalidate.

Comment 23

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

Attachment: patch #2

I should probably add a comment at some point as well, perhaps something like:

// If it's the canvas frame, we need the frame's view to be sized correctly 
// as well so invalidates from background image loads will work correctly.

Comment 24

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

Attachment: fix

This is the straightforward fix: just invalidate the entire document. This isn't really wasteful because in almost all cases that's nearly what we're going to do anyway.

Comment 25

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

(In reply to comment #23)
> Created an attachment (id=216693) [edit]
> patch #2

scrolledView is scrolledFrame->GetView(). Why don't you just have the call to nsContainerFrame::SyncFrameViewAfterReflow set scrolledView's bounds to scrolledArea? The only reason that isn't happening now is because scrolledFrame doesn't get FRAME_OUTSIDE_CHILDREN until after that call.

Comment 26

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

Comment on attachment 216694 [details] [diff] [review]
fix

It would be better to fix the view bounds if we can do that without breaking scrolling.

Comment 27

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

We should fix the #if 0 code in nsImageLoader after we've fixed this...

Comment 28

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

(In reply to comment #25)
> scrolledView is scrolledFrame->GetView().

Oh, so it is.  I was thinking it was the anonymous view.

Comment 29

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

Attachment: patch #2

Yeah, this works.

Comment 30

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

Fix checked in to trunk.

Comment 31

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

Attachment: better testcase

Comment 32

[Boris Zbarsky [:bzbarsky]]

It looks like this fixed perf regression bug 331385 (which was a regression from bug 192767).