If you apply the attached patch to reftest.js and run the reftests you should see a whole bunch of failures that don't show up in a normal build. With the exception of bugs/438987-2[abc], they all have the same symptom: a strip at the right or bottom of the image has not been drawn on by drawWindow() and therefore remains the lurid shade of red that was painted by the patched reftest.js in order to catch this problem. The documentation for drawWindow() is not all that specific, but it seems to me that a canvas user is within its rights to expect that it will paint a rectangle of exactly the width and height specified; in this case that's the entire canvas area. (For 438987-2[abc], see bug 453566, which is what I was trying to catch when I started messing with reftest.js in the first place.)

I was never clear with how drawWindow interacts with the source window's dimensions... I know that drawWindow won't ever cause a reflow, so along the width at least it'll only draw up to the window's existing width. But, if the background color is non-transparent then we at least should fill it. roc, does this make sense?

I would argue that drawWindow should paint exactly what the regular paint path paints. In particular that means we don't paint anything outside the viewport. I don't see why you'd want it any other way.

Well, here's the thing. If the background area of a real window is not being totally filled with opaque color by the normal paint path, we can get display garbage in any area that it missed. This can be clearly seen in bug 453566. Currently the reftests can't catch that, because they are having drawWindow fill the target rectangle with white before calling the normal paint path. The patch instead has reftest.js do the fills itself, and apply one shade of red to the test canvas, a different shade to the reference canvas. This is sufficient to detect the case I care about, but it also causes these color stripes when the viewport isn't big enough to fill the entire canvas. I'm not sure whether "the bounding rectangle of the area that the regular paint path will paint" is exposed to JS -- the window.innerWidth and .innerHeight properties might be what I want but then again they might not. If it is I can account for it in reftest.js; otherwise I'm stuck.

I tried filling the canvas with white and then calling drawWindow with width and height expressions that I hoped would get a more restricted area, and the "lurid shade of red" as the background color. This produces the same undesirable color bars at the edges of the result images: ctx.drawWindow(win, win.scrollX, win.scrollY, win.innerWidth, win.innerHeight, bgcolor); This causes the run to hang up about a quarter of the way in, when one of the subtractions comes up with a negative number: ctx.drawWindow(win, win.scrollX, win.scrollY, win.innerWidth - win.scrollX, win.innerHeight - win.scrollY, bgcolor); I would be delighted to hear further suggestions.

Let's assume we have your patch for bug 453566. Then what reftest fails to fill the entire canvas? Is the problem that we're failing to paint scrollbars? If so, that's just because drawWindow passes true for aIgnoreViewportScrollbars, which means we skip drawing the scrollbars, but we won't extend the CanvasFrame which is responsible for drawing the viewport background.

> Then what reftest fails to fill the entire canvas? About 25 of them: 163504-1a 163504-1b 163504-2a 163504-2b 234964-1 243519-1 243519-7 28811-1a 28811-1b 28811-2a 28811-2b 331809-1 364862-1 389468-1 abspos-non-replaced-width-offset-margin abspos-replaced-width-offset-margin balancing-1 columnrule-basic columnrule-complex columnrule-padding long-1 malformed-uri outline-radius-percent-1 stress-6 wordwrap-05 > Is the problem that we're failing to paint scrollbars? That would explain why there's only 25 failures out of 2000+ cases, and why sometimes the unpainted rectangle is on the right and sometimes it's on the bottom, but I am not sure all of the above have scrollbars. > If so, that's just because drawWindow passes true for > aIgnoreViewportScrollbars, which means we skip drawing > the scrollbars, but we won't extend the CanvasFrame which > is responsible for drawing the viewport background. Ok, but then what to do about it? Turn scrollbars back on?

Assuming that's the problem, then one way to fix things would be: -- start with the test and reference canvases in the same state -- fill the test and reference canvases with different colors in the area given by win.contentDocument.documentElement.getBoundingClientRect() (rounded inward, I suppose) -- drawWindow in both canvases

I'm trying that now (assuming you meant gBrowser.contentDocument... instead of win.contentDocument...)

> -- fill the test and reference canvases with different colors in the area given > by win.contentDocument.documentElement.getBoundingClientRect() Doesn't work, at least not if you pass that area to drawWindow as the area it is to draw. Nearly a hundred tests fail because the test and the reference have been clipped to different rectangles, and in most of those cases, the rectangle does not include part of the test content for *both* test and reference. That isn't exactly what you said to do, but if the rectangle getBoundingClientRect returns isn't the least bounding rectangle for all of the test content, then it's the wrong rectangle.

(wrong rectangle to be using in this case, I mean, not the wrong rectangle for getBoundingClientRect to be returning - having read the spec, I think the problem is that it's giving us the least bounding rectangle for the union of all border-boxes, but that excludes stuff being painted in the margin areas.)

After a great deal of back and forth, this is the proposed permanent change to reftest.js. It will cause breakage if landed before bug 453566 so I will set that bug to block this one. Thanks to roc for patiently helping me figure out what needed doing. I don't know who should review changes to this stuff.

I should say that I no longer think changes to canvas drawWindow are required, although I might change my mind again if I find that reftests still can't detect unpainted areas under nastier conditions than the existing 438987-2[abc] cases. (There may not *be* any nastier condition, but I haven't proved that to myself yet, either.) Moving to Testing for now.

Comment on attachment 337823 [details] [diff] [review] (rev 1) proposed actual change to reftest.js according to ted, reftest changes are for dbaron to review.

So does it make sense to you that drawWindow doesn't fill the space that it's being told to draw in? It doesn't to me (hence the XXX comment that you're removing, which goes back to the very beginning of reftest).

The logic goes like this: 1) drawWindow *does* fill the space if you pass an opaque color for its last argument. 2) We don't want to do that in this case, because we want to know if the regular paint path left anything unpainted. 3) But the regular paint path doesn't guarantee to fill the space, only the rectangle returned by getBoundingClientRect() [and in practice a much larger area, but not one whose bounds are accessible from chrome]. 4) roc doesn't think the regular paint path should be changed. I personally think it feels like a bug that the regular paint path doesn't guarantee to fill the space, but I didn't want to argue with roc.

So in what cases does the regular paint path not fill the rect?

There's a list in comment 6; roc thought it might have something to do with the scrollbars being suppressed.

If you use drawWindow to paint an area larger than the viewport, we won't touch the areas that are outside the viewport. I assume we agree that's logical. If you use drawWindow to paint an area equal to the viewport, we won't touch the area where viewport scrollbars are, because we're suppressing scrollbars and the canvas frame's background does not extend under the scrollbars. Making the frame area passed to nsCSSRendering::PaintBackground extend under the scrollbars would break background-image anchoring. I suppose we could pass in an extra rectangle to nsCSSRendering::PaintBackground so that the background-origin computation is not affected but we can extend the background outside the frame's border-area. But that seems like a significant complication to achieve a questionable result.

(In reply to comment #18) > If you use drawWindow to paint an area larger than the viewport, we won't touch > the areas that are outside the viewport. I assume we agree that's logical. Oops, this is not always true, we will paint areas of the document that extend outside the viewport vertically, because we're suppressing scrollbars and scroll clipping.

I suppose a simpler option for root content documents would be to paint the (opaque) prescontext default background over the entire viewport to cover up the area where the viewport scrollbars would be.

dbaron, I think we need your feedback here to make progress.

Comment on attachment 337823 [details] [diff] [review] (rev 1) proposed actual change to reftest.js r+sr=dbaron

tagging for checkin. NPOTbrowser, but could conceivably break reftests and therefore cause tinderbox failures. (It didn't at the time when I wrote the patch, but who knows what the state is now.)

Comment on attachment 337823 [details] [diff] [review] (rev 1) proposed actual change to reftest.js { Hunk #1 FAILED at 429 1 out of 1 hunk FAILED } I wanted to manually update the code, but the very last part has a new zoom/scale feature, so it's best to leave the unbitrotting to you.

I think this is the right way to fix up the patch for the new zoom functionality. Unfortunately, I'm now seeing the patch expose what look like genuine bugs: bugs/28811-[12][ab], bugs/163504-[12][ab], and bugs/446100-1[cefgh] all regress with the patch applied. For 28811 and 163504 the difference is a small number of pixels unpainted at the edge of an IMG. For 446100 there is a region of varying size and shape left unpainted in the upper right-hand corner of the canvas. I don't have any real idea how to debug either of them.

Comment on attachment 349864 [details] [diff] [review] update for zoom functionality >+ // drawWindow always draws one canvas pixel for each CSS pixel in >+ // the source window, so we scale the drawing to show the zoom >+ // (making each canvas pixel be one device pixel instead) Seems like you should leave this comment where it was, separate from yours. It's about a separate subject. >+ var scale = gBrowser.markupDocumentViewer.fullZoom; >+ var bbox = gBrowser.contentDocument.documentElement.getBoundingClientRect(); >+ > var ctx = canvas.getContext("2d"); >- var scale = gBrowser.markupDocumentViewer.fullZoom; No need to reorder scale vs. ctx. Did you test that this is drawing the correct area for the zoomed case? r+sr=dbaron

See also bug 451332, "Need a way to test invalidation issues".

(In reply to comment #25) > Unfortunately, I'm now seeing the patch expose what look like genuine bugs: > bugs/28811-[12][ab], bugs/163504-[12][ab], and bugs/446100-1[cefgh] all regress > with the patch applied. For 28811 and 163504 the difference is a small number > of pixels unpainted at the edge of an IMG. For 446100 there is a region of > varying size and shape left unpainted in the upper right-hand corner of the > canvas. It's probably better to get this in sooner rather than later, anyway. I just hit the problems on 446100-1[efgh] because of bad interaction of my tests in bug 421203 with the new reftest canvas caching. (Is the reftest canvas caching done in a way that's even valid without this patch?) Maybe we should just land this patch and mark those tests as known to fail until we fix them.

So, actually, there's also one other problem with this patch -- you need to modify the reftest image cache to cache whether things are a test or a reference. (You can probably just add "test-" or "ref-" to the beginning of the cache key for both gURIUseCounts and gURICanvases or something like that.) Otherwise you risk failing to do this new comparison properly for things that are used sometimes as a test and sometimes as a reference.

Actually, since I want to test that, I'm doing that merge myself right now (and addressing comment 29).

I decided to switch url[12], gCanvas[12], and the values of gState to use "test" and "ref" instead, and then use "test-" and "ref-" as prefixes in the cache. I'm seeing a total canvas count of 13 with this, which I think is the same as before. (In reply to comment #25) > Unfortunately, I'm now seeing the patch expose what look like genuine bugs: > bugs/28811-[12][ab], bugs/163504-[12][ab], and bugs/446100-1[cefgh] all regress > with the patch applied. For 28811 and 163504 the difference is a small number > of pixels unpainted at the edge of an IMG. For 446100 there is a region of > varying size and shape left unpainted in the upper right-hand corner of the > canvas. Right now I'm seeing only bugs/446100-1[cefgh] as failing. It seems likely that the others were fixed with roc's recent image drawing pixel snapping work (and various followup patches). Though it's possible there's some condition with those tests that only shows up on your machine.

(In reply to comment #31) > Right now I'm seeing only bugs/446100-1[cefgh] as failing. It seems likely > that the others were fixed with roc's recent image drawing pixel snapping work > (and various followup patches). So I think these are actually all bugs in this patch, and in particular its interaction with the reftest-zoom code. In particular: * I think painting bbox after scaling isn't actually doing the right thing (this is what breaks [efgh]) * I think there are issues when the width of what we're drawing isn't an exact number of image pixels, because then we get the painted background showing through anyway (this is what breaks c); I think we need to round the size of what we draw down to the nearest number of image pixels. I'd also note that the reftest-zoom patch introduced a regression where each test run doesn't always paint over the entire reftest canvas, which is why I'm seeing problems in bug 421203. This patch fixes that regression by filling the entire canvas with white before the drawWindow.

(In reply to comment #32) > * I think painting bbox after scaling isn't actually doing the right thing > (this is what breaks [efgh]) It's actually the reftest-zoom patch that was wrong here: it needs to pass canvas.width / scale and canvas.height / scale to drawWindow instead of just canvas.width and canvas.height.

I thought this would fix it, but e and h are still failing (with a multiple-pixel-wide band at the left). This fixes what I described in the previous comment, does a bunch of floor/ceil calls, and also fixes a swapped left and top in the fillRect call.

I applied your revision on top of a totally unmodified tree and ran the reftests in Xvfb; I still see all of these fail: 28811-1a } 28811-1b } unpainted fringe at right of an IMG 28811-2a } 28811-2b } 163504-1a } 163504-1b } unpainted fringe at bottom of an IMG 163504-2a } 163504-2b } 446100-1f } unpainted multi-pixel stripe in upper right hand corner 446100-1h } I am going to see if the patch in bug 468498 helps with the IMG fringes.

Correction: bug 468496.

The patch in bug 468496 eliminates all of the failures! Unfortunately, we can't use it, as it leaves us either relying on known-buggy X Render support, or on slow client-side Cairo fallbacks. How should we proceed, then?

Disable the tests on Linux, with a comment that they rely on bug 468496.

(In reply to comment #38) > Disable the tests on Linux, with a comment that they rely on bug 468496. OK, except it sounds like dbaron is seeing fewer failures than I am...

I messed up with patch queue manipulation; what I said in comment 37 is incorrect. The patch in bug 468496 only eliminates the "fringe at bottom/right of an IMG" regressions.

So this revision marks the reftests whose regression would be cured by bug 468496 as random on gtk2, and also makes some minor editorial changes to the comments in reftest.js. The regression of 446100-1[fh] is still to be dealt with.

The 446100-1[fh] regressions turn out to be due to an arithmetic error in the hand-scaling for the box drawn under the boundingClientRect; we need to be multiplying by 'scale', not dividing.

the copy-editor in my head insists on adjusting the comment formatting a little.

Comment on attachment 353302 [details] [diff] [review] tidy up Looks good, r=dbaron. I'll land it for you shortly; I was just getting ready to land bug 421203. (I'll keep the zoom fix there separate since it is logically distinct.)

That was fast! I'm a little worried about the image-seam problem showing up on Mac too, but I guess we'll see what the tinderbox makes of it.

Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5bf3661db065

Had to back out due to massive numbers of reftest failures on Windows: http://hg.mozilla.org/mozilla-central/rev/5e236f0d5669 http://hg.mozilla.org/mozilla-central/rev/dbd177a9e5f4

This tarball contains decoded images for all the windows reftest failures, and an index.html to make it easy to browse through them. (I just picked a bad build at random.) All the failures are very similar. There is either an eight-pixel vertical strip, the height of the canvas and flush against the right edge of the canvas, that is allegedly inside getBoundingClientRect but isn't painted; or there is a single-pixel vertical line, again the height of the canvas, eight pixels in from the right edge. None of the failures appeared on Linux. All of them involve SVG, either standalone or embedded in HTML.

Here's a patch which applies to current trunk. No other changes.

Have you got a Windows environment in which you can debug the failures?

Not easily - I have Windows but I have no development tools for it. I'd have to go through the try server and I'm not sure that'd even work for reftests.

You can download MozillaBuild and Visual Studio Express and get a Windows build going fairly easily.

I didn't know there was a no-cost downloadable version of Visual Studio. I'm setting that up now.

rediffed.

I no longer work for Mozilla and have no intention of working on this bug as a volunteer.