449647 - After scrolling down and up again, the border-image parts haven't been repainted

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, I already see some red at the bottom part of the green box, which shouldn't happen, I think.
When scrolling down and then up, the rest of the green box also becomes red.

Comment 1

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

I see this on Linux as well.

I'd thought the check for IsBorderImageLoaded here:
http://hg.mozilla.org/mozilla-central/index.cgi/file/ce485b9145bf/layout/base/nsDisplayList.cpp#l604
ought to prevent this, but it seems not to.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> I already see some red at the bottom part of the green box, which
> shouldn't happen, I think.

Related issue: The positioning of the already-visible red depends on the scrolled position of the page.

Steps to reproduce this:
 0. Load testcase
 1. Scroll down just a little bit (so that some or most of the colored square is still visible)
 2. Reload, or alt-tab to another window.

RESULTS: The red band disappears from the bottom and reappears at the top of the visible area.

Note:
 The sizing of this load-time red band seems to depend upon the scroll position in a very deterministic way.  Specifically: 
 - If there's visible space above the green square, the red band is the height of that space.  It's also located at the bottom of the square.
 - If the green square is partially scrolled out of view, the red band is the height of the obscured part. It's also located at the top of the visible part of the square.
 - If the green square is positioned *exactly* at the top of the visible area, then no red is present.

Comment 4

[Daniel Holbert [:dholbert]]

I tried tweaking the "overflow" property in the testcase, with these results.

 OVERFLOW VALUE                                     BUGGY (See any red?)
 ------------------------------------------------   --------------------
 overflow: scroll                                     Yes
 overflow: auto                                       Yes
 overflow: hidden                                      No
 overflow: visible                                     No

 overflow-x: [non-visible]                            Yes
 overflow-y: [non-visible]                            Yes

 overflow-x: hidden;      overflow-y: hidden           No
 overflow-x: hidden;      overflow-y: [non-hidden]    Yes
 overflow-x: [non-hidden]; overflow-y: hidden         Yes

Note:
 [non-hidden]  = auto, scroll, visible
 [non-visible] = auto, scroll, hidden

Questions that this raises:
 1.  Why does "overflow: hidden" behave differently from "overflow: auto", when they render the same on this testcase (and both use scrollframes & views, which the "overflow:visible" testcase doesn't)?

 2.  Why are we buggy when we have *one of* overflow-x/y set to hidden, but we're not buggy when we have *both* set to hidden?

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (lots of combinations)

This testcase illustrates most of the combinations mentioned in Comment 4.

For me, the only green blocks are: "hidden", "visible", and "xhidden yhidden" (which is equivalent to "hidden", AFAIK)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (lots of combinations)

Here's an updated version of testcase 2, with a tall empty div at the end to give us a browser scrollbar to play with.

This testcase shows that the only blocks affected by browser-scrolling are the left column -- specifically, it's exactly the blocks with any of their overflow propoerties set to "scroll".

(Maybe the other red blocks are correct to be red? I haven't looked at them in depth, but at least they don't have repaint issues, apparently.)

Comment 7

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

I think the underlying problem has something to do with the code in DrawBorderImageSide, which:
 * calls aThebesContext->UserToDevicePixelSnapped(aDestRect)
 * later calls aThebesContext->Translate(aDestRect.pos)
This has the effect (at 1:1 scale) of doubling whatever translation happened to be in the context's matrix (since the original aDestRect is frame-relative, and the new aDestRect is now frame-relative + the context's translation).

Comment 8

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

Attachment: possible patch

This seems to work, and the border-image reftests still pass, although I'd like to poke around a bit more.

Comment 9

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

Attachment: possible patch

So here's the above patch, plus reftests.

I took one of the existing reftests and stuck it inside a -moz-transform:  it turns out that makes us fail it *if* the transform doesn't have a scale (because we turn off pixel-snapping if the transform does have a scale).  I stuck in reftests for both the transform-with-translate-only and the transform-with-scale-and-translate.

Without the patch, I confirmed that we fail multicolor-image-5; with the patch, we pass all border-image reftests.

I don't claim to understand all of this code, but requesting review anyway to see what other people think.

Comment 10

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

(In reply to comment #4)
>  1.  Why does "overflow: hidden" behave differently from "overflow: auto", when
> they render the same on this testcase (and both use scrollframes & views, which
> the "overflow:visible" testcase doesn't)?
> 
>  2.  Why are we buggy when we have *one of* overflow-x/y set to hidden, but
> we're not buggy when we have *both* set to hidden?

The reason is that we don't give the scrollframe a widget in that case.  (Having a widget means (I presume, otherwise why bother) that we can scroll it faster, but it has some overhead.  overflow:hidden elements can be scrolled only programmatically, so we optimize for the case that they're not scrolled.)  See
http://hg.mozilla.org/mozilla-central/annotate/184ad4f909cd/layout/generic/nsGfxScrollFrame.cpp#l1401 although note that that code doesn't really need to check NS_STYLE_OVERFLOW_VISIBLE since we'll never have that for scrollbars.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 341508 [details] [diff] [review]
possible patch

fwiw, that looks right -- the intent is that it's used with an identity matrix on the surface, as in http://mxr.mozilla.org/mozilla-central/source/gfx/src/thebes/nsThebesRenderingContext.cpp#230 .  So if that's not being done here, then the patch looks correct -- after the rectangle is transformed (again), we should end up with pixel coordinates, barring float precision issues.

Comment 12

[Rob Arnold [:robarnold]]

Comment on attachment 341508 [details] [diff] [review]
possible patch

>+// FIXME:  This function takes two rects and a size and modifies them.
>+// Some callers pass the size as the size within the first rect.  Does
>+// that always do what we want?

Probably not. I think we could construct a case where the scaling doesn't work properly since the rect will be pixel snapped which will cause the size to lose precision. Changing the parameter to a gfxSize instead of a gfxSize & would be an easy fix to the problem.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 341508 [details] [diff] [review]
possible patch

(Doh, yes, I meant to sr+ this.)  I agree with rob's suggestion as well -- just make the passed in dest/source (and maybe size?) non-refs to avoid modifying the originals..

Comment 14

[Rob Arnold [:robarnold]]

To clarify, the gfxRects can be modified. They are constructed for each call. The issue is that the gfxSize parameter is sometimes aliased to one of the rects which DrawBorderImageSize does not expect. Only gfxSize needs to be made non-ref.

Comment 15

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

Fixed: http://hg.mozilla.org/mozilla-central/rev/a9f44a03b7d0

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090414 Minefield/3.6a1pre ID:20090414030735

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre ID:20090414035212