472769 - Painting bug with select drop down menu when hovering over options

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase

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

Comment 1

[Martijn Wargers (dead)]

Attachment: Screenshot of issue

The problem only seems to happen in strict mode.

Comment 2

[Nochum Sossonko [:Natch]]

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

Comment 4

[Nochum Sossonko [:Natch]]

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

Comment 5

[Martijn Wargers (dead)]

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

Comment 6

[Martijn Wargers (dead)]

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

Comment 7

[u88484]

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.

Comment 8

[skierpage]

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

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Zack Weinberg (:zwol)]

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.

Comment 11

[Martijn Wargers (dead)]

(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?

Comment 12

[Zack Weinberg (:zwol)]

(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.

Comment 14

[Daniel Holbert [:dholbert]]

(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).

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: simpler testcase from bug 474817

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.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: simpler testcase from bug 474817, quirks mode

(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.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: screencast of bug (using quirks-mode testcase)

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.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: fix?

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?

Comment 19

[Zack Weinberg (:zwol)]

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 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Daniel Holbert [:dholbert]]

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

Comment 22

[Zack Weinberg (:zwol)]

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?

Comment 23

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

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

Comment 24

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

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

Comment 25

[Daniel Holbert [:dholbert]]

(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}.

Comment 26

[Daniel Holbert [:dholbert]]

(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."

Comment 27

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

I see.

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

Comment 28

[Daniel Holbert [:dholbert]]

(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.

Comment 29

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 30

[Daniel Holbert [:dholbert]]

>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)

Comment 31

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

(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.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: fix v3

(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.

Comment 33

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

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

Comment 34

[Daniel Holbert [:dholbert]]

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. :)

Comment 35

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

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.

Comment 36

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

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 :-) ).

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 39

[Daniel Holbert [:dholbert]]

(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.

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: reftest v1

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.

Comment 41

[Daniel Holbert [:dholbert]]

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.

Comment 42

[Daniel Holbert [:dholbert]]

Attachment: regression caused by patch when zooming: test1

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.

Comment 43

[Daniel Holbert [:dholbert]]

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)

Comment 44

[Daniel Holbert [:dholbert]]

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?

Comment 45

[Daniel Holbert [:dholbert]]

(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.

Comment 46

[Daniel Holbert [:dholbert]]

Attachment: reftest v2

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.)

Comment 47

[Daniel Holbert [:dholbert]]

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)

Comment 48

[Daniel Holbert [:dholbert]]

Attachment: fix v3 with reftests (as landed)

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.

Comment 49

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

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.

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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 51

[Markus Stange [:mstange]]

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 52

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 53

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9d06e2be1e19

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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?

Comment 55

[Martijn Wargers (dead)]

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.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.