Last Comment Bug 704399 - Gap between background and border when using border-radius (due to antialiasing?)
: Gap between background and border when using border-radius (due to antialiasi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
http://jsfiddle.net/edelman/NJHLU/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 20:06 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-02-01 13:58 PST (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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. (4.15 KB, patch)
2011-11-23 10:33 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter 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. (8.21 KB, patch)
2011-11-29 13:56 PST, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
Details | Diff | Splinter 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. (8.74 KB, patch)
2011-12-01 17:23 PST, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2011-11-21 20:06:30 PST
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) 2011-11-21 20:46:00 PST
Yes!
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-11-23 10:33:31 PST
Created attachment 576540 [details] [diff] [review]
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 [:bz] (still a bit busy) 2011-11-23 11:18:47 PST
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) 2011-11-23 14:08:26 PST
Expanding out by one pixel sounds good.
Comment 5 Zack Weinberg (:zwol) 2011-11-29 12:27:47 PST
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 [:bz] (still a bit busy) 2011-11-29 13:11:44 PST
> 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) 2011-11-29 13:42:25 PST
(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 [:bz] (still a bit busy) 2011-11-29 13:56:55 PST
Created 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.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-29 14:01:16 PST
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 [:bz] (still a bit busy) 2011-11-29 14:02:23 PST
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 [:bz] (still a bit busy) 2011-11-29 14:04:31 PST
And in fact, I can then just use aAppUnitsPerPixel.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-12-01 16:34:07 PST
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 [:bz] (still a bit busy) 2011-12-01 17:23:11 PST
Created attachment 578451 [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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-12-05 21:01:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/85358854403f
Comment 15 Ed Morley [:emorley] 2011-12-06 10:05:43 PST
https://hg.mozilla.org/mozilla-central/rev/85358854403f

Note You need to log in before you can comment on or make changes to this bug.