704399 - Gap between background and border when using border-radius (due to antialiasing?)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The testcase is at the url, but here's a pretty minimal version:

  <style>
  div {
    width: 100px;
    height: 100px;
    border: 10px solid black;
    background: black;
    border-radius: 60px;
  }
  </style>
  <div></div>

There is a gray line separating the black ring from the black circle.

I expect that this is due to the fact that we draw the background to the padding area, with antialiasing, then draw the border with antialiasing, so we get a strip of not-fully-painted-on pixels...

In particular, if I change this code in nsCSSRendering:

     currentBackgroundClip = bg->BottomLayer().mClip;
     isSolidBorder =
       (aFlags & PAINTBG_WILL_PAINT_BORDER) && IsOpaqueBorder(aBorder);
     if (isSolidBorder && currentBackgroundClip == NS_STYLE_BG_CLIP_BORDER)
       currentBackgroundClip = NS_STYLE_BG_CLIP_PADDING;
 
to also require !haveRoundedCorners to change currentBackgroundClip to PADDING, then the problem goes away...  Should we just do that in general?

Comment 1

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

Yes!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Treat curved borders as not fully opaque for purposes of enabling the 'restrict background painting to the padding area' optimization, because the curved edges end up antialasing into semitranslucency.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hrm.  That patch causes some of the other border-radius tests to fail since we now paint the background out to the edge of the rounded area.

Most of them are not that important (and I can explicitly set padding-box styling on them to fix them, in fact), but 456219-2.html shows that we're getting a few pixels of aqua ghosting at the very corners of the outermost part of the border.  That's not unexpected, but seems undesirable...  So I'm not quite sure how best to update that testcase or whether it indicates that we need a new approach to this bug (e.g. only expanding out by one pixel, not all the way out to the border edge.  roc?

Comment 4

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

Expanding out by one pixel sounds good.

Comment 5

[Zack Weinberg (:zwol)]

I tried expanding out by only one pixel in bug 466572.  It is possible for that to not cover the entire seam, and it is also possible for that to *create* ugly fringes where there weren't any before, if the border is semitransparent.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> It is possible for that to not cover the entire seam

In what situations?

> it is also possible for that to *create* ugly fringes where there weren't any before, if
> the border is semitransparent.

I'm restricting this to the case of fully opaque borders.  That is, it's a modification of our existing "optimize the background clip area to the padding area when it's the border area to start with and the border is opaque" code.

Comment 7

[Zack Weinberg (:zwol)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > It is possible for that to not cover the entire seam
> 
> In what situations?

I don't remember exactly, but I think it involved large radii and/or high eccentricity.

> I'm restricting this to the case of fully opaque borders.

Ok.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: When doing the 'restrict background painting to the padding area' optimization, actually restrict it to padding area plus 1px on each side to deal with seams caused by curved borders better.

Comment 9

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

Comment on attachment 577746 [details] [diff] [review]
When doing the 'restrict background painting to the padding area' optimization, actually restrict it to padding area plus 1px on each side to deal with seams caused by curved borders better.

Review of attachment 577746 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +1580,5 @@
> +    if (aBackgroundClip == NS_STYLE_BG_CLIP_MOZ_ALMOST_PADDING) {
> +      // Reduce |border| by 1px on all sides, if possible, so that we
> +      // don't get antialiasing seams between the background and
> +      // border.
> +      nscoord onePixel = nsPresContext::CSSPixelsToAppUnits(1);

DevPixelsToAppUnits I think

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, high eccentricity could do it, yeah.  I'm not going to worry about that case for now.

> DevPixelsToAppUnits I think

Makes sense.  Will do that.

Comment 11

[Boris Zbarsky [:bzbarsky]]

And in fact, I can then just use aAppUnitsPerPixel.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, I tried doing that.  The test for this bug then fails on windows, because with only 1px of overlap we end up with some pixels that are rgb(1,1,1) insteadof rgb(0,0,0).

I'm just going to mark the test as failing on Windows, I think... :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: When doing the 'restrict background painting to the padding area' optimization, actually restrict it to padding area plus 1px on each side to deal with seams caused by curved borders better.

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85358854403f

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85358854403f