See testcase, when hovering over the options in the select drop down, I get black lines that seem to linger at the bottom of every option, when moving upwards, they disappear again. This regressed between 2009-01-07 and 2009-01-08: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-01-07+04%3A00%3A00&enddate=2009-01-08+06%3A00%3A00

The problem only seems to happen in strict mode.

This is a dupe of bug 472851 (or vice-versa your call).

Martijn, I can't reproduce this on trunk with your testcase, although I can using Kurt's with his str. Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5

Kurt's testcase is https://bugzilla.mozilla.org/attachment.cgi?id=356207

So with the regression range, the most likely cause for this bug seems to me the patch for bug 456219.

Like I said in the dupe bug, I can't even reproduce with the test case. But paste in the code from the test case into the htmledit link I provided and then I can reproduce. Not sure what is going on there. I also can not reproduce when the test case is locally. I changed the encoding type which did not help and the html edit and the test case are in quirks mode. You can also try through the add an attachment link for bugzilla. The drop down for the content type shows the same bug and that page is in standards mode.

I experience this bug with the attachment link for bugzilla and Martijn's testcase, but not with Kurt's testcase. Depending on my View > Zoom amount I see a gray line lingering at the bottom of every option either when I move the mouse up or when I move down. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090110 Minefield/3.2a1pre ID:20090110033715

I can't reproduce with the testcase here, but I can reproduce on a mxr page, like the "view using tree" select here: http://mxr.mozilla.org/mozilla-central/source/Makefile.in

I can't reproduce using any of the available testcases on a Linux trunk build. I am running a Windows trunk build right now, but it takes an insane amount of time. More news probably late this evening.

(In reply to comment #10) > I am running a Windows trunk build right now, but it takes an insane amount of > time. It takes an insane amount of time to run a Windows trunk build? That's strange, it should start up pretty quickly. Perhaps you should file a new bug for that?

(In reply to comment #11) > > It takes an insane amount of time to run a Windows trunk build? That's strange, > it should start up pretty quickly. No, it takes an insane amount of time to *compile* a Windows trunk build in the slow, single-processor virtual machine that's the only place I've got Windows installed. A null build takes twenty minutes; a full build, which is what I had to do this morning, takes more like three hours. I have been able to reproduce the bug on Windows, though, using the MXR url suggested by Ted.

(In reply to comment #10) > I can't reproduce using any of the available testcases on a Linux trunk build. Zack: See duplicate-bug 474817's attachment 358223 [details] and attachment 358226 [details], which are very simple and which reproduce the problem on Linux (on my Ubuntu machines, at least).

Here's the first testcase from bug 474817. It's simpler than this bug's testcase, and it reproduces the bug for me on both Windows & Linux.

(In reply to comment #1) > The problem only seems to happen in strict mode. Nope -- this testcase reproduces it, using quirks mode. The original testcase was affected by the mode because it uses an inline-block, which behaves differently in quirks vs strict mode, and that affects the vertical placement of the combobox.

FWIW: It looks like the corruption at the top of row n is left behind when we move *downwards* off of row n, but the corruption is fixed/cleared when we move *upwards* off of n, as illustrated in this OGG screencast.

Not 100% sure it's correct, but this patch fixes the bug on my machine. It just replaces "dirtyRectGfx.Round();" with "dirtyRectGfx.RoundOut();". See method descriptions here: http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxRect.h#138 This change makes sense intuitively to me -- if a dirty rect has, say, y=4.5, and we Round() that up to y=5.0, then we won't correctly recognize that the area from y=4.5 to 5.0 is dirty. RoundOut, on the other hand, rounds outwards, such that the new rect always encloses the old rect (rounding to y=4.0 in this example). Using RoundOut means we might mark a small number of extra pixels as dirty, but that's much better than marking too *few* pixels as dirty. Zack / roc, what do you think -- is this the right fix?

I'm not sure if, or why, we need the Round/Condition pair there at all, but your patch is the first thing I was going to try.

Comment on attachment 358268 [details] [diff] [review] fix? This patch actually fails some reftests -- it apparently causes us to sometimes draw 1px of extra background on top & to the left in some situations. In particular, it gives me an unexpected 1px-high blue bar at the top of http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/369882.xul and it adds 1px of yellow padding to the top and left of the rectangle generated by http://mxr.mozilla.org/mozilla-central/source/layout/reftests/reftest-sanity/zoom-invalidation.html Perhaps the Round/Condition aren't needed at all, as Zack suggested -- I'll give that a shot and see if it passes reftests.

Nope, looks like we need those Round/Condition lines -- if I get rid of them, we fail a whole bunch of tests, including almost all "pixel-rounding/background-color-*" tests -- e.g. background-color-height-4.html, background-color-height-5.html, background-color-height-6.html

So it sounds like we need what Round() does for the upper left hand corner, and what RoundOut() does for the width and height? Gfx folks, is there anything off-the-shelf that does that?

Round() rounds the edges to the nearest pixel boundaries, not the width and height directly.

Normally dirty rects are pixel-aligned. You might want to find out why this one isn't pixel-aligned.

(In reply to comment #24) > Normally dirty rects are pixel-aligned. You might want to find out why this one > isn't pixel-aligned. The non-pixel-alignment stems from the view's "ViewToWidgetOffset", as demonstrated by these lines in nsViewManager::Refresh: 442 nsPoint vtowoffset = aView->ViewToWidgetOffset(); 445 nsRegion damageRegion; 447 ConvertNativeRegionToAppRegion(aRegion, &damageRegion, mContext); 449 damageRegion.MoveBy(viewRect.TopLeft() - vtowoffset); http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp Here are some variable values when we hit this: * vtowoffset {x = 0, y = -30} * aRegion->mRegion->mBoundRect {x = 2, y = 17, width = 88, height = 35} * damageRegion STARTS as {x = 120, y = 1020, width = 5280, height = 2100} * damageRegion ENDS UP as {x = 120, y = 1050, width = 5280, height = 2100} (after the MoveBy call) So note that damageRegion _starts out_ pixel-aligned, but it ends up off-alignment after we shift it by the view to the widget. From that point, damageRegion gets passed down to RenderViews, which hands it to mObserver->Paint, and its values eventually make it down to the call to nsCSSRendering::PaintBackgroundWithSC, where it's giving us trouble in this bug. FWIW, the above was all true bug 456219 landed, also. At that time, we got around this by calling PaintBackgroundColor, and **not** passing it the dirty rect, but instead "bgClipArea", which in this particular case is copied from aBorderArea = {x = 0, y = 0, width = 5460, height = 3180}.

(In reply to comment #25) > FWIW, the above was all true bug 456219 landed, also. At that time, we got Sorry -- I meant "the above was all true _before_ bug 456219 landed, also."

I see. So instead of rounding rects there, we really need to use gfxContext's snap-to-pixels API.

(In reply to comment #27) > So instead of rounding rects there, we really need to use gfxContext's > snap-to-pixels API. I'm not sure if this is what you were suggesting, but I tried replacing dirtyRectGfx.Round(); with ctx->UserToDevicePixelSnapped(dirtyRectGfx, PR_FALSE); but that still breaks -- it leaves residue at the *bottom* of the highlight-rectangle, rather than at the top. This seems to be because it rounds down the y-value without modifying the height, leaving 1/2px of ignored dirtiness at the bottom of the rect. I looked into this a bit more, though, and I think I know what we need to do. The initial strategy -- using RoundOut() -- is correct, except that we need to do it *before* the intersection with bgClipArea. My first patch failed in situations with non-pixel-aligned **clip areas**. In those situations, and if we intersect and THEN round out, then we can end up expanding to paint 1/2px outside of the clip area, which is bad. (This explains why I was getting an incorrect 1px of extra background-color in comment 23.) However, if we round-out the dirty area first, and then intersect, we're ok. I've got a patch that uses this strategy, and it passes reftests. I'll attach it in a minute.

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp >+ // Grow dirty-rect to the nearest containing pixel-box, to make sure >+ // that we don't miss some dirty area due to rounding later on. >+ nsIntRect dirtyRectRoundedOutPixels = >+ nsRect::ToOutsidePixels(aDirtyRect, appUnitsPerPixel); >+ nsRect dirtyRectRoundedOut = >+ nsIntRect::ToAppUnits(dirtyRectRoundedOutPixels, appUnitsPerPixel); The above lines accomplish the "RoundOut()" behavior, giving us the tightest pixel-aligned nsRect that contains "aDirtyRect". Perhaps it would be useful to include this as a helper method in nsRect.h? Something like this: nsRect nsRect::ToOutsidePixelsInAppUnits(const nsRect &aRect, nscoord aAppUnitsPerPixel)

(In reply to comment #28) > (In reply to comment #27) > > So instead of rounding rects there, we really need to use gfxContext's > > snap-to-pixels API. > > I'm not sure if this is what you were suggesting, but I tried replacing > dirtyRectGfx.Round(); > with > ctx->UserToDevicePixelSnapped(dirtyRectGfx, PR_FALSE); > but that still breaks -- it leaves residue at the *bottom* of the > highlight-rectangle, rather than at the top. This seems to be because it > rounds down the y-value without modifying the height, leaving 1/2px of ignored > dirtiness at the bottom of the rect. I really think that is the right direction to pursue. That is the way that other code, like DrawImage, handles snapping. In fact we probably shouldn't be doing anything to dirtyRectGfx, but just pass PR_TRUE to the snap parameter when we call ctx->Rectangle. Once we get that working, we will need to do something for the RoundedRectangle call --- maybe add a snap parameter to it.

(In reply to comment #31) > In fact we probably shouldn't be doing anything to dirtyRectGfx, but just pass > PR_TRUE to the snap parameter when we call ctx->Rectangle. Ah, great! That indeed fixes this bug, without breaking any reftests. This attached patch removes all dirtyRectGfx tweaking, and adds PR_TRUE to three ctx->Rectangle() calls. Only the first ctx->Rectangle call is actually relevant to this bug's STR, but it like the other calls need fixing as well for cases where we have bg-color + loading bg-image.

This looks good. Is there a reason it's not ready for review? Maybe you're working on a reftest? :-)

Comment on attachment 360124 [details] [diff] [review] fix v3 (In reply to comment #33) > This looks good. Is there a reason it's not ready for review? Maybe you're > working on a reftest? :-) Yeah -- I'm not sure how to reftest this at this point -- I was thinking about trying to build off of related bug 474208, since that one doesn't require clicking open a drop-down-list to reproduce. Ideally I'd like to make a reftest for each tweaked Rectangle() call. (including the two that involve background-images) In any case, I'll tentatively request review on the patch now, and I'll post an automated testcase once I figure out how to make one. :)

If you need a widget-to-view offset, use an overflow:auto element positioned at a fractional coordinate. The widget will be snapped to pixels but the view won't move.

We'll need to do something similar for the rounded-rect path, like I said in comment #31. I'd appreciate it if you can file a followup bug for that (and fix it :-) ).

Once baked, please nominate the patch for approval. Good polish, not a blocker, though.

Just to note: I've tested the latest tryserver build from Daniel and everything works as expected.

(In reply to comment #35) > If you need a widget-to-view offset, use an overflow:auto element positioned at > a fractional coordinate. The widget will be snapped to pixels but the view > won't move. After working on some testcases & talking with roc over IRC, it seems this can't be directly reftested. Basically, we need a situation where the view has *its own* display-root, with both being positioned at the fractional coordinate. That's true of a dropdown menu's list-pane, but it's not true of other types of widgets, AFAICT -- other widgets (or at least, overflow:auto elements) use the main viewport as their display root. That ends up working around this bug for those widgets, because we subtract "offsetToRoot" from the dirtyRect at some point, and if the viewport is our display root, that subtraction will remove off any fractional component from the dirtyRect long before we hit PaintBackgroundWithSC. So, roc suggested using a CSS transform instead, to translate a div by 0.5px, which would test a related bug that "patch v3" should also fix. That seems to work (i.e. patch v3 changes our behavior there). I'll attach that testcase in a minute.

This reftest asserts that a CSS-transform-translation by 0.5px should be equivalent to adding a 0.5px margin, and that both should *actually* move the div by 1px. In current mozilla-central trunk, a 0.5px translation ends up blurring 1px on either side of the div (averaging the white body-background and the black div-background). This makes test "472769-1a.html" fail in current mozilla-central. But after applying fix v3, it passes.

472769-1b.html (which asserts that a 0.5px margin should round up to a 1px margin) passes both before and after "fix v3" -- I've just included it for clarity & completeness, as a way of explaining why 472769-1a should pass.

Here's a test that shows a possible regression caused by fix v3, however, at various zoom-levels (or at least a change in behavior). Details to follow.

Here's a summary of current mozilla-central behavior vs. patched mozilla central behavior, on attachment 360786 [details]. CONDITIONS CURRENT M-C RESULTS PATCHED M-C RESULTS ---------- -------------------- ------------------- a) No zooming * Top border is blurred orange/black * Everything crisp * Bottom 1px of background is blurred orange/white b) Zoom in 1x * same as (a) * Top border is solid orange c) Zoom in 2x * same as (a) * same as (b) d) Zoom in 3x * same as (a) * Bottom 1px of background is solid white e) Zoom in 4x * same as (a) * Everything crisp f) Zoom in max * same as (a) * Everything crisp g) Zoom out 1x * Top 1px of background is * Top 1px of background blurred orange/white is solid white * Bottom border is blurred * Bottom border is orange/black solid orange h) Zoom out 2x * same as (g) * same as (g) i) Zoom out 3x * same as (g) * Everything crisp j) Zoom out max * same as (g) * same as (g)

Here's a summary of the table above - Current mozilla-central has consistent (though slightly ugly) pixel-blurring behavior across zoom levels. - Patched mozilla-central has no blurring, but has *inconsistent* behavior across zoom levels (and it looks clearly broken at most zoom-levels, with white-background showing through where it shouldn't be, and/or with orange background completely painting over the border). Maybe we just need to add pixel-snapping in the code where we paint borders, too?

(In reply to comment #44) > Maybe we just need to add pixel-snapping in the code where we paint borders Ah -- nsCSSBorderRenderer::FillSolidBorder() calls gfxContext::RoundedRectangle as part of its painting, so I'm guessing this issue would be fixed by adding snap-to-pixels capability to RoundedRectangle, as roc suggested in comment 31 and comment 36. I've filed bug 477122 on adding that.

I'm adding two more -moz-transform reftests that are fixed by "fix v3." The new tests assert that a background-rect and an equivalent border-rect should still look equivalent after they're translated by a fractional pixel amount. These new tests fail on current mozilla-central -- the background-rects move in a gradual, blurry-edged way, whereas the border-rects don't move at all for translations < 0.5px, and then they snap up at 0.5px. The new tests pass after applying "fix v3", though, because it gives background-rects the pixel-snapping behavior. (Note that if I change the zoomlevel, the new tests fail, even after this bug's patch -- probably due to the zooming issue already described above. I'm hoping that'll be fixed in bug 477122.)

I've actually filed bug 477157 specifically on the background- & border-zooming issues described above (e.g. in comment 43 and at the end of comment 46), just in case they don't end up being fixed by the RoundedRectangle snapToPixel stuff in bug 477122. (because I'm not yet convinced that they'll be fixed by that)

Patch landed as http://hg.mozilla.org/mozilla-central/rev/50e934e4979b with reftests. Note that the version of "reftest v2" attached here was missing the line for 472769-1b.html from reftest.list. I fixed that before pushing, and I'm attaching the full patch + reftests (including that reftest.list fix) as landed.

We should be snapping the rectangles used to draw borders, if we aren't already. But borders get special treatment where the style system tries to round their widths to device pixels to ensure that borders with a given specified width are always rendered with consistent device pixel width, even when drawn at different fractional offsets.

Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090207 Minefield/3.2a1pre and the appropriate build on OS X.

Comment on attachment 360841 [details] [diff] [review] fix v3 with reftests (as landed) This is a regression from blocking1.9.1 bug 456219 and has baked for over a month now.

Comment on attachment 360841 [details] [diff] [review] fix v3 with reftests (as landed) a191=beltzner

Daniel, I tried to verify the fix on 1.9.1 but I'm not able to see this behavior with a build before the patch was checked in. Even the reftests are fine for me without this patch. Is there anything I miss?

Afaics, the patch for bug 456219 went into the 1.9.1 tree at the same time as this patch was, so this regression was never to be seen on the 1.9.1 branch.

That's true. Thanks Martijn. This can also be seen in the pushlog: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?changeset=b410d21c51e3 Verified fixed based on the passed reftests on OS X and Windows on my box.