448193 - with border inset/outset, 1px, and -moz-border-radius, bottom and right sides are drawn in wrong color

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Richard Marti (:Paenglab)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008072703 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008072703 Minefield/3.1a2pre

When the border has a 3D effect style with 1px and -moz-border-radius, no 3D effect is drawn. If the border is more than 1px, it's drawn correctly.

Regression window: okay in nightly from 2008-07-23-03-mozilla-central, not okay in nightly from 2008-07-24-03-mozilla-central

I assume Bug 424423 has regressed this.

Reproducible: Always

Steps to Reproduce:
1. See URL

Comment 1

[j.j.]

Attachment: testcase

Comment 3

[Alfred Kayser]

Attachment: Another testcase showing more clearly the specific problem

Comment 4

[Alfred Kayser]

Requesting Wanted 1.9.1 as this is a clear regression in layout rendering (border inset radius) (note radius is becoming a web standard, as also webkit/opera/safari/chrome support it, so FF should not regress here...)

Comment 5

[Alfred Kayser]

Requesting Blocking 1.9.1 as this is a clear regression in layout rendering
(border inset radius) (note radius is becoming a web standard, as also
webkit/opera/safari/chrome support it, so FF should not regress here...)

This bug causes content and chrome to render border with 1px inset/outset/ridge/groove combined with radius wrong.

Comment 6

[Zack Weinberg (:zwol)]

I've looked at this a little.  First, the bug only affects inset and outset, not groove and ridge.  (Groove and ridge are intentionally degraded to solid at 1px, and have been for some time.  There might should be some code to adjust the colors so they're not so blatantly different from what's drawn at width 2px, but that would be a separate bug.)  I've adjusted the bug summary.  Second, this has nothing to do with the CSS3 (elliptical) border-radius patches, so I've dropped bug 431176 from the blocks list, but it would be good to include this case in the test suite being written in bug 458131, so this still blocks that, and I've added ctalbert to the cc for this one.

As for the actual cause of the bug, I think it's a clipping error, probably introduced in the patches for bug 368247 and/or bug 424423.  I think this because if you compare the rendering for 'border: 1px outset red' with 'border: 1px solid; border-color: #ff7f7f #b30000 #b30000 #ff7f7f' with nonzero border radius, you will see that the former is ever so slightly thicker; this is especially visible at the corner curves.  I do not see the fix immediately; vlad may have more luck.

Comment 7

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

Attachment: fix

Fix. What was actually happening was that the full border was being drawn twice, once with each color for the 3d effect.  I don't know how to do a reftest for this; I can create one that will check whether inset is different from solid, but that would pass even without this patch due to the double-painting.

Comment 8

[Zack Weinberg (:zwol)]

(In reply to comment #7)
> Fix. What was actually happening was that the full border was being drawn
> twice, once with each color for the 3d effect.

I thought that must have been what was going on.

> I don't know how to do a
> reftest for this; I can create one that will check whether inset is different
> from solid, but that would pass even without this patch due to the
> double-painting.

I have one.  I'll merge it and confirm.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: reftest

so here's the reftest I have, which demonstrates that your patch works, Vlad; however, I'm getting a failure due to two lousy pixels.  As I currently get about 20 reftest failures due to single-pixel differences (on two different computers, yet) I am gonna post it anyway and ask that you try it.

Comment 10

[Zack Weinberg (:zwol)]

Comment on attachment 342131 [details] [diff] [review]
fix

anyway, the patch works, so.  (Procedurally speaking, am I supposed to be able to r+ things, and if so, when?)

Comment 11

[Zack Weinberg (:zwol)]

(yes, I did read it and check that it *ought* to fix the bug, as well as that it does.)

Comment 12

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

Comment on attachment 342176 [details] [diff] [review]
reftest

Hm, yeah, that reftest should work.  I'll add it to my patch when I check in.

Comment 13

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

I take that back, the reftest fails due to the small pixel differences; I'll check it in as commented out with a comment to that effect

Comment 14

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

Pushed this out by accident as part of my patch set; it's a simple enough patch.  I pushed out the reftest as well, but it's commented out, ideally until we can add some variation in reftests...

Comment 15

[Alfred Kayser]

Verified as fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081010 Minefield/3.1b2pre.

Thanks!

Comment 16

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

(In reply to comment #14)
> Pushed this out by accident as part of my patch set; it's a simple enough
> patch.  I pushed out the reftest as well, but it's commented out, ideally until
> we can add some variation in reftests...

FWIW I don't think adding some kind of tolerance threshold to reftests is a good way to go. We'll never be able to find the "right" tolerance algorithm or threshold value.

I've had some success at tracking down tiny pixel variations and eliminating them by fixing code or changing the test. Where genuine variation is allowed, you can usually cover up the varying pixels with censoring shapes.

Comment 17

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

Comment on attachment 342131 [details] [diff] [review]
fix

sr=dbaron

Comment 18

[Phil Ringnalda (:philor)]

Since we did wind up adding fuzzy(), I added the reftest files (which must have missed an hg add originally) and uncommented the manifest line and made it fuzzy for those two pixels in https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0f5bf7a824.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f0f5bf7a824