509329 - background image rendered incorrectly when window resized

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[Michael Ryan]

Attachment: testcase

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090809 Minefield/3.6a2pre

A background image may be rendered incorrectly when the window is resized.

Steps to reproduce:
1) Open testcase
2) Resize the window width to make the window as narrow as possible
3) Return window width to normal size

Expected: There should be a vertical white line in the middle of the page.
Actual: There are a random number of vertical white lines at random locations.

Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441

Comment 1

[Michael Ryan]

Attachment: screenshot

Comment 2

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

Hmm.  Does the bit at the end of nsIFrame::CheckInvalidateSizeChange need to use nsCSSRendering::FindBackground?  (And, even so, what would have caused the regression?)

Comment 3

[Michael Ryan]

good: http://hg.mozilla.org/mozilla-central/rev/026403392281
bad: http://hg.mozilla.org/mozilla-central/rev/ff084019260e

Comment 4

[Jeff Walden [:Waldo]]

I think it does need to change, at the very least to address bug 507750, which is probably a dup of this.  That doesn't explain why it doesn't appear on OS X, unfortunately.

Comment 5

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

For what it's worth, on Linux, I'm seeing the background simply not get updated until something else happens to repaint the window (which I think includes ending the drag).

Comment 6

[Jeff Walden [:Waldo]]

Er, make that bug 507550.  I'm pretty sure no change we make to our own code will affect Opera!

Comment 7

[RNicoletto]

Cannot reproduce.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090731 Shiretoko/3.5.2

Comment 8

[Jeff Walden [:Waldo]]

Aha, maybe I see the (rest of the) problem!

First, as previously noted, there's not using FindBackground.

Second, as I note in bug 507550, the frame's width is the full window width and its height is zero (of course -- it doesn't have anything in it!).  FindBackground needs to be modified to get both the nsStyleBackground* and the actual frame for it so that *that* frame's rect, not aOldRect, can be invalidated.  Sound right?  I can whip up the patch if this reasoning sounds plausible.

Comment 9

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

Hmmm.  I had been thinking that we'd pick up the right background when resizing the canvas because we'd call CheckInvalidateSizeChange on the canvas frame.  But the problem with background fixup is that the background positioning area is actually derived from the dimensions of the root frame.  And I think it's possible for the dimensions of the root frame to change without the size of the canvas changing.

(Am I right that the root element dimensions are used to compute the background positioning area for the canvas, and the canvas dimensions *are not*?  Or are both used?)

So, really (if I'm right above), I think we should probably rely on knowledge of the internals of FindBackground, and, in CheckInvalidateSizeChange, if we're about to invalidate, call FindBackground, and if it returns null (i.e., our background is propagated), invalidate the canvas instead.

(Or does FindBackground do any other fixup than root->canvas and body->canvas?)


For what it's worth, we could probably also skip this step for fixed backgrounds.  Except, if there are any fixed backgrounds, we need to invalidate when the size of the canvas changes.  And we don't do that either!

Comment 10

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

(In reply to comment #9)
> But the problem with background fixup is that the background positioning area
> is actually derived from the dimensions of the root frame.  And I think it's
> possible for the dimensions of the root frame to change without the size of
> the canvas changing.

Yes.

> (Am I right that the root element dimensions are used to compute the
> background positioning area for the canvas, and the canvas dimensions *are
> not*?  Or are both used?)

The canvas is not used (except for background-attachment:fixed, which uses *only* the canvas).

> So, really (if I'm right above), I think we should probably rely on knowledge
> of the internals of FindBackground, and, in CheckInvalidateSizeChange, if
> we're about to invalidate, call FindBackground, and if it returns null (i.e.,
> our background is propagated), invalidate the canvas instead.

Sounds good.

> For what it's worth, we could probably also skip this step for fixed
> backgrounds.  Except, if there are any fixed backgrounds, we need to
> invalidate when the size of the canvas changes.  And we don't do that either!

Argh!

Comment 11

[Jeff Walden [:Waldo]]

I hope to get to this next week.

Comment 12

[Jeff Walden [:Waldo]]

So much for "next week"; building a maybe-patch right now, tho.

Comment 13

[Jeff Walden [:Waldo]]

That patch, as might have been guessed, didn't work -- still on this, tho.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Half-fix, addresses the testcase in this bug

This fixes the original testcase.  It also fixes the original testcase if I make the background fixed.  However, if I add a width to the background (in addition to making it fixed), it's still broken.  I'm kind of debating making this halfway step here and then finish it up in bug 507550, rather than doing it all at once here, but for now I'm going to continue work on one patch for bug.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Another test (likely fixt by the layer.RenderingMightDependOnFrameSize() change)

Comment 16

[Jeff Walden [:Waldo]]

So, the reason the patch breaks if you add an explicit width to body, which has a fixed background, as in the reduced testcase in bug 507550, is the early-exit check in nsIFrame::CheckInvalidateSizeChange:

void
nsIFrame::CheckInvalidateSizeChange(const nsRect& aOldRect,
                                    const nsRect& aOldOverflowRect,
                                    const nsSize& aNewDesiredSize)
{
  if (aNewDesiredSize == aOldRect.Size())
    return;

aNewDesiredSize and aOldRect.Size() have the same width when width is explicitly set, of course.  For all the testcases so far the height of the body is much less than the height of the canvas, so resizing doesn't change the height, either.  Therefore the equality is true and we return early, without invalidating the canvas.

This actually points out a further bug/deficiency in my past testing.  If there's no width, I thought I'd fixed matters -- but the only reason was that I was resizing the window horizontally and vertically both!  If I resize a fixed/no-width testcase solely in the vertical direction, width doesn't change, height is smaller than the window so it doesn't change, and therefore the same invalidation issues occur.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Testcase (appears fixed if resized diagonally, broken if resized vertically)

Resize this testcase in the vertical direction only, and you see bustage, as noted in the last paragraph of comment 16.

Fundamentally, it seems the == early-exit check is unsound when:

  layer.mAttachment == NS_STYLE_BG_ATTACHMENT_FIXED &&
  layer.RenderingMightDependOnFrameSize()

...when the reflow is a presshell resize-reflow.  It seems the == early-exit should be changed to this:

  if (!doingPresshellResizeReflow(this) && aNewDesiredSize == aOldRect.Size())
    return;

...maybe, but 1) I'm not sure what doingPresshellResizeReflow should be, and 2) just axing the == check entirely doesn't work either, so probably I need to get the nsStyleBackground* differently.  Still working on that at the moment, distracted by an entirely unrelated assertion I somehow seem to have picked up via weird sessionstore.js entries...

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Fixed, percent-sized background not propagated to canvas

Here's a final testcase; resize it to be narrow and the widths of the two yellow boxes should change (at least to a certain point, see the source for precise details).  The widths should change as the window is resized, but on Linux they don't change until the mouse is released and the drag ends.  OS X probably doesn't have an issue because something causes the entire window to redraw as the drag happens.  Windows doesn't even do the end-of-drag redraw, so it stays bad until a full-window repaint happens.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Patch for everything but the immediate previous testcase

This fixes everything that's mentioned in this bug, in bug 507550, and in bug 508353 except for attachment 409210 [details].  I'm not sure how to handle that attachment.  The nsCanvasArea change addresses the case in the attachment for propagated backgrounds, but in the non-propagated place it seems -- given the information we have now -- the only way to invalidate enough would be to walk the entire frame tree.  Clearly we don't want that, but I don't have enough experience with this code to say what we should be doing instead to make sure non-propagated frames with size-dependent backgrounds are invalidated when necessary.

Anyway: this fixes most of the cases people will care about, and we're getting close to the wire, so I'd like to get the 95% cases fixed even if the 5% cases end up being deferred.  I'll file a followup to address the non-propagated fixed size-dependent case when this attachment or some form of it makes it into the tree.

(Also regarding attachment 409210 [details]: while that attachment uses background-size to get misrenderings, I'm pretty sure you can get errors for the same reason with a non-zero percentage background-position, so the mistakes demonstrated are not new with background-size.)

Comment 20

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

+void InvalidateRect(nsIFrame* aFrame, const nsRect& aRect)

static

InvalidateRect propagates invalidation to the root no matter what caused it --- outline, border, background, etc. But only background propagates to the root. You're relying on the fact that the rootframe covers everything, which isn't quite true actually; Fennec requires that invalidation happens even on content which seems to be outside the viewport. So I suggest that you change InvalidateRect to invalidate both the frame and the rootframe. Also, let's call it something a bit more specific, like InvalidateRectForFrameSizeChange.

You should probably leave the review with dbaron though, because of the nsStyleStruct.h change.

Comment 21

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

Comment on attachment 409217 [details] [diff] [review]
Patch for everything but the immediate previous testcase

In nsStyleStruct.h, could you put a comment in the definition of
DimensionType pointing to DependsOnFrameSize?

In nsCanvasFrame.cpp:

+    if (aReflowState.mFlags.mHResize || aReflowState.mFlags.mVResize) {

I think it makes more sense to check (after setting aDesiredSize.width
and .height):
  if (nsSize(aDesiredSize.width, aDesiredSize.height) != GetSize()) {
since what you're really interested in is whether the frame's size
changed, not what optimizations we can check during reflow.  And moving
the code down to there shouldn't be a problem.

However, you should file a followup bug on doing this right (which
probably requires either storing or recomputing the areas painted with
fixed size-dependent backgrounds so we can invalidate them when the
canvas size changes).

Your comment should also make it clearer that this code isn't the right
fix, and doesn't handle fixed backgrounds that aren't propagated to the
canvas.

In nsFrame.cpp:

You should probably comment that InvalidateRect's going up to the root
frame is only really needed for the background
RenderingMightDependOnFrameSize() case, but you're doing it for all
cases so that you can leave the early return after invalidation.

Also, InvalidateRect should be |static|.

r=dbaron with that

Comment 22

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/19a209a691be

I filed bug 525556 on making non-propagated, fixed, percent-sized or -positioned backgrounds invalidate correctly on window resize.

I tried writing invalidation reftests for this, but I couldn't get it to work either using the MozReftestInvalidate event or by manually making it work with MozAfterPaint.  I tried using and resizing a nested iframe, since the reftest window's size can't be changed, but the invalidation information exposed by MozAfterPaint didn't seem to make much sense.  If anyone else wants to try with the tests here, in bug 507550, or in bug 508353, go ahead, but after spending more than a few hours trying to get it work and failing I'm inclined to move to spending time on other issues and blockers.

I'll let this bake for a few days or a week or so before asking for 192 approval.

Comment 25

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

(In reply to comment #22)
> I'll let this bake for a few days or a week or so before asking for 192
> approval.

It's a blocker; you don't need approval.  And I'd suggest something more like "a few days" than "a week or so".

Comment 26

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8945f271cfac

The patch port wasn't quite painless due to nsCanvasFrame separation into a new file and lack of nsStyleBackground::Image::IsEmpty, but it was close enough to not present any mental complexity.