Last Comment Bug 459144 - Border radius should clip content in rounded corners when overflow != visible
: Border radius should clip content in rounded corners when overflow != visible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: mozilla2.0b7
Assigned To: David Baron :dbaron: ⌚️UTC-10
:
: Jet Villegas (:jet)
Mentors:
: 409547 536629 537433 575252 (view as bug list)
Depends on: 485501 595842 596205 596271 613905
Blocks: border-radius 451134 458131
  Show dependency treegraph
 
Reported: 2008-10-08 15:47 PDT by cmtalbert
Modified: 2010-12-13 18:17 PST (History)
30 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
Testcase showing interaction of border-radius and scrolling (5.00 KB, text/html)
2008-10-08 15:47 PDT, cmtalbert
no flags Details
curvy_scroll.png (3.76 KB, image/png)
2010-02-14 20:39 PST, Biju
no flags Details
patch 1: move GetBorderRadiusTwips to nsIFrame (11.93 KB, patch)
2010-09-03 18:40 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords (7.78 KB, patch)
2010-09-03 18:42 PDT, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch 1: move GetBorderRadiusTwips to nsIFrame (11.87 KB, patch)
2010-09-03 18:48 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords (7.76 KB, patch)
2010-09-03 18:49 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 3: move skip-sides handling from ComputePixelRadii to ComputeBorderRadii (14.45 KB, patch)
2010-09-03 18:50 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 4: support subpixel border radius (1.40 KB, patch)
2010-09-03 18:51 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 5: move clamping of radii into ComputeBorderRadii (15.38 KB, patch)
2010-09-03 18:52 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 6: add border radius helper methods to nsIFrame (5.71 KB, patch)
2010-09-03 18:54 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 7: use GetBorderRadii so we pick up when frames override it (5.36 KB, patch)
2010-09-03 18:55 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 8: reduce border radius when scrollbars are present (8.33 KB, patch)
2010-09-03 19:17 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 9: expose nsCSSRendering::ComputePixelRadii (2.67 KB, patch)
2010-09-03 19:18 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 10: expose RectToGfxRect on nsLayoutUtils (4.46 KB, patch)
2010-09-03 19:19 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 11: remove unneeded nsDisplayClip::mClippingFrame (9.67 KB, patch)
2010-09-07 18:20 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 12: remove unneeded nsAbsPosClipWrapper::mContainer (2.87 KB, patch)
2010-09-07 18:21 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 13: add nsDisplayClipRoundedRect display item and handle it in FrameLayerBuilder (29.31 KB, patch)
2010-09-07 18:25 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review
patch 14: clip overflow!=visible to the border radius (13.17 KB, patch)
2010-09-07 18:27 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
Details | Diff | Splinter Review

Description cmtalbert 2008-10-08 15:47:32 PDT
Created attachment 342331 [details]
Testcase showing interaction of border-radius and scrolling

Borders styled using border radius can fall on the inside edge of the content they encompass and inside their scrollbars.  It seems like this is a known issue per the specification[1] where it alludes to the problem: "This example adds appropriate padding, so that the contents do not overflow the corners." 

However, note in the example, even with padding, the scrollbars are still displayed oddly.  I think we should be following the specification's direction and make the border radius 0 when a scrollbar is involved.  Here is their direction on this matter: "The UA may reduce or treat as zero the border-radius for a given corner if a scrolling mechanism is present in that corner."

A testcase is attached.
Comment 1 David Baron :dbaron: ⌚️UTC-10 2008-10-08 16:41:10 PDT
So is there a reason we should add code to prevent authors from doing this if they want?
Comment 2 Zack Weinberg (:zwol) 2008-10-08 18:02:33 PDT
seems to me there's two separate questions:

 - should curved borders clip the content as well as the background?
 - should curved borders be disabled in the presence of scrollbars?

In both cases, I can see the "let the author shoot themselves in the foot" argument against, but I can also see a "this looks really horrible" argument for, especially for content clipping.

Also, what if we added something like "-moz-scrollbar-position: outside" that caused the scrollbars to change places with the border?  What would that do to your answers?
Comment 3 cmtalbert 2008-10-09 11:57:31 PDT
I don't see any(In reply to comment #2)
> seems to me there's two separate questions:
> 
>  - should curved borders clip the content as well as the background?
>  - should curved borders be disabled in the presence of scrollbars?

Right, those are the questions.  In both cases my main argument is that it looks really awful, that and the fact that I (perhaps naively) expected the border to clip content. The question of whether or not we should prevent web authors from making ugly pages is a whole other question of philosophy.  And in those sorts of questions, I usually feel like the more flexibility we give people the better. So, I'm undecided.  

We could leave the content issue as is, but I do like the idea of using -moz-scrollbar-position: outside so that authors have some control over this behavior.
Comment 4 Zack Weinberg (:zwol) 2008-10-09 12:03:03 PDT
note that I just made up "-moz-scrollbar-position:outside", I don't know if we have that feature already, or how hard it would be to add if not.
Comment 5 Brian Polidoro 2009-03-27 11:50:19 PDT
>  - should curved borders clip the content as well as the background?

Bug 485501 covers this for images.  Should that be a separate bug or be part of this bug (dupe)?
Comment 6 Zack Weinberg (:zwol) 2009-06-03 18:26:15 PDT
I'm making this directly block the border-radius tracking bug (431176) because the spec's been revised to say that overflow:hidden is supposed to clip content to the curve (whichever curve is selected by background-clip).
Comment 7 fantasai 2009-06-12 15:29:51 PDT
The spec has said overflow:hidden clips to the curve for awhile, it's just more explicit now.

I think the border-radius should be reduced down to the border-width if there's a scrollbar in that corner. The argument for doing this automatically is that not doing it does look really ugly; I can't imagine an author ever wanting that. Argument for the author not expecting it to look ugly is that perhaps they're designing for a different device or layout engine that doesn't use scrollbars for scrolling.
Comment 8 fantasai 2009-06-12 15:33:43 PDT
Alternatively, if it's possible, you could reduce the length of the scrollbars until they fit along the straight part of the border.
Comment 9 Zack Weinberg (:zwol) 2009-08-27 14:07:25 PDT
cc:ing faaborg for UI input.
Comment 10 David Baron :dbaron: ⌚️UTC-10 2009-12-31 20:06:57 PST
*** Bug 537433 has been marked as a duplicate of this bug. ***
Comment 11 David Baron :dbaron: ⌚️UTC-10 2009-12-31 20:09:03 PST
*** Bug 409547 has been marked as a duplicate of this bug. ***
Comment 12 David Baron :dbaron: ⌚️UTC-10 2009-12-31 20:11:12 PST
(In reply to comment #7)
> I think the border-radius should be reduced down to the border-width if there's
> a scrollbar in that corner. The argument for doing this automatically is that

I think we should just do that, since it's simple, and it lets us move forward on fixing the case that authors actually care about, which is the case where there's no scrollbar.
Comment 13 Emil Ivanov 2009-12-31 21:08:09 PST
*** Bug 536629 has been marked as a duplicate of this bug. ***
Comment 14 Brandon Frohs 2010-01-01 00:12:50 PST
http://frohsenwebdesign.com/projects/firefox-border-radius-bug/index.html

Additional test case available online.
Comment 15 David Baron :dbaron: ⌚️UTC-10 2010-01-01 07:26:09 PST
Note also that some of the code in the patch in bug 485501 might be useful here.
Comment 16 Brandon Frohs 2010-01-01 09:56:55 PST
(In reply to comment #8)
> Alternatively, if it's possible, you could reduce the length of the scrollbars
> until they fit along the straight part of the border.

This would be impossible (or look horrible) in many cases. When a box is not tall (or wide) enough for the entire border-radius to be seen, the side will look like a circle. Ergo, there is no straight part.
Comment 17 nemo 2010-02-14 17:41:03 PST
WRT comment #16, presumably the DWIM thing to do then would be to shrink the internal text area to the largest box that fits inside the circle when scroll bars were needed.
For a circle that'd presumably be a square of height/width √2r.
Or clip to padding or inner fill, whichever was smaller, if no scroll bars?

No clue what the alg would be for any arbitrary elliptical border rounding though.
So glad I'm not the one having to solve these lovely challenges CSS poses :)
Comment 18 Biju 2010-02-14 20:39:47 PST
Created attachment 426943 [details]
curvy_scroll.png

WRT comment #16 I have few ideas
1. we can have curvy scroll bar, like curvy_scroll.png
   yeah, for circle it is little difficult, 
   may be make with height/width = √2r. (as suggested in comment #17)
2. for most practical case it may be possible to get a straight edge
   so show scroll bar for those boxes.
   for other cases dont show.
3. Avoid showing ScrollBar.Track and ScrollBar.Box, 
   just show only ScrollBar.(up|down|left|right) buttons when mouse hovers
Comment 19 WulfTheSaxon [:Wulf] 2010-03-21 02:22:28 PDT
Is it even possible to have a curvy scrollbar? I thought those were provided by the OS. (Either way, not a fan. Reminds me too much of some bad Flash sites I've seen.)
Comment 20 Kevin Brosnan 2010-06-28 06:36:37 PDT
*** Bug 575252 has been marked as a duplicate of this bug. ***
Comment 21 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:40:03 PDT
Created attachment 472074 [details] [diff] [review]
patch 1: move GetBorderRadiusTwips to nsIFrame

I have a series of patches that fix this bug and bug 485501.  I still need to sort out some display-list-related issues higher up in the patch series, but I may as well post the initial (and somewhat substantial) series of refactoring patches.

This moves nsCSSRendering::GetBorderRadiusTwips to nsIFrame.  I'm not sure whether to call it nsIFrame::ComputeBorderRadius or nsIFrame::ComputeBorderRadii.  (This patch uses ComputeBorderRadius, but in a later patch I used Radii for the same concept; I should probably pick one but I'm not sure which is better.  I'm probably leaning towards radii, though -- i.e., changing this one.)
Comment 22 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:42:03 PDT
Created attachment 472075 [details] [diff] [review]
patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords

This makes it harder to get the parameters backwards, and also saves some extra lines of code in some places.
Comment 23 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:48:57 PDT
Created attachment 472082 [details] [diff] [review]
patch 1: move GetBorderRadiusTwips to nsIFrame

Actually, I decided I prefer Radii to Radius, and fixed up patch 1 accordingly.
Comment 24 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:49:38 PDT
Created attachment 472083 [details] [diff] [review]
patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords

and a revised patch 2
Comment 25 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:50:17 PDT
Created attachment 472086 [details] [diff] [review]
patch 3: move skip-sides handling from ComputePixelRadii to ComputeBorderRadii

Move the skip-sides handling into ComputeBorderRadii since all callers want it (and I'll be adding more that do, but that don't use ComputePixelRadii).
Comment 26 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:51:22 PDT
Created attachment 472087 [details] [diff] [review]
patch 4: support subpixel border radius

This fixes some possibly-unintentional integer division and thus supports subpixel values of border-radius.  It's a prerequisite for the next patch, but I wanted to keep it separate.
Comment 27 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:52:06 PDT
Created attachment 472088 [details] [diff] [review]
patch 5: move clamping of radii into ComputeBorderRadii

Like the skip sides handling, all callers want this (including one current one).
Comment 28 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:54:34 PDT
Created attachment 472090 [details] [diff] [review]
patch 6: add border radius helper methods to nsIFrame

This adds a bunch of methods to nsIFrame, somewhat like we have GetUsedPadding, etc.

GetBorderRadii is virtual so that things that don't support border radius can opt out of all border radius handling and so that scroll frames can reduce their border radius when needed.
Comment 29 David Baron :dbaron: ⌚️UTC-10 2010-09-03 18:55:35 PDT
Created attachment 472091 [details] [diff] [review]
patch 7: use GetBorderRadii so we pick up when frames override it

This continues the idea in the previous comment about allowing frames to override GetBorderRadii by making the existing code care when they do.
Comment 30 David Baron :dbaron: ⌚️UTC-10 2010-09-03 19:17:35 PDT
Created attachment 472094 [details] [diff] [review]
patch 8: reduce border radius when scrollbars are present

This reduces the border radius appropriately when no scrollbars are present.

At each corner, it reduces the radius at any corners where the inside edge of the border would curve, maintaining the shape of that corner, until the inside corner of the border has no curvature.

(I just realized that I was reducing more than needed and modified the patch.  I only need one of the radii to be less than the side of the border, not both.)
Comment 31 David Baron :dbaron: ⌚️UTC-10 2010-09-03 19:18:42 PDT
Created attachment 472095 [details] [diff] [review]
patch 9: expose nsCSSRendering::ComputePixelRadii

The display item I implement later needs this.
Comment 32 David Baron :dbaron: ⌚️UTC-10 2010-09-03 19:19:29 PDT
Created attachment 472096 [details] [diff] [review]
patch 10: expose RectToGfxRect on nsLayoutUtils

This changes us from having two copies to having one.  The alternative would have been to go from two to three in the display list patch.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-05 15:39:39 PDT
Comment on attachment 472090 [details] [diff] [review]
patch 6: add border radius helper methods to nsIFrame

+nsIFrame::MoveBorderRadiiIn(nscoord aRadii[8], const nsMargin &aOffsets)
+nsIFrame::MoveBorderRadiiOut(nscoord aRadii[8], const nsMargin &aOffsets)

I think InsetBorderRadii and OutsetBorderRadii would be better, more consistent with other methods.

+  /*
+   * Given a set of border radii for one box (e.g., border box), convert
+   * it to the equivalent set of radii for another box (e.g., in to
+   * padding box, out to outline box) by reducing radii or increasing
+   * nonzero radii as appropriate.

Probably worth mentioning that Inset can be lossy so people should go from the outside in if possible.

I actually can't think of anywhere we need OutsetBorderRadii or GetContentBoxBorderRadii, but I guess I'll find out :-).
Comment 34 David Baron :dbaron: ⌚️UTC-10 2010-09-05 15:44:15 PDT
(In reply to comment #33)
> I actually can't think of anywhere we need OutsetBorderRadii or
> GetContentBoxBorderRadii, but I guess I'll find out :-).

GetContentBoxBorderRadii is needed for the clipping of replaced elements.

(GetPaddingBoxBorderRadii is for the clipping of 'overflow'!='visible')

OutsetBorderRadii is for bug 593717, which I'm actually probably not going to do now (although I was thinking I would).
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-05 16:01:16 PDT
Comment on attachment 472094 [details] [diff] [review]
patch 8: reduce border radius when scrollbars are present

+  // Hmmm.  Do we want GetActualScrollbarSizes or GetDesiredScrollbarSizes?

GetBorderRadii doesn't affect reflow I guess, so GetActualScrollbarSizes should
be fine.

+  if (sb.left > 0 || sb.top > 0)
+    ReduceRadii(border.left, border.top,
+                aRadii[NS_CORNER_TOP_LEFT_X],
+                aRadii[NS_CORNER_TOP_LEFT_Y]);

I think we still are trying to put {} around most if bodies.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-05 16:02:59 PDT
Comment on attachment 472096 [details] [diff] [review]
patch 10: expose RectToGfxRect on nsLayoutUtils

+static inline gfxRect
+RectToGfxRect(const nsRect& aRect, nscoord aAppUnitsPerDevPixel)
 {
-  return gfxRect(gfxFloat(rect.x) / twipsPerPixel,
-                 gfxFloat(rect.y) / twipsPerPixel,
-                 gfxFloat(rect.width) / twipsPerPixel,
-                 gfxFloat(rect.height) / twipsPerPixel);
+  return nsLayoutUtils::RectToGfxRect(aRect, aAppUnitsPerDevPixel);

Why didn't you just change the callers to call nsLayoutUtils::RectToGfxRect directly?
Comment 37 David Baron :dbaron: ⌚️UTC-10 2010-09-05 18:29:46 PDT
(In reply to comment #36)
> Why didn't you just change the callers to call nsLayoutUtils::RectToGfxRect
> directly?

I think I didn't want to have to deal with wrapping lots of lines, but maybe it wouldn't be an issue.  I'm happy changing it; it might have less risk of performance loss too (if the inlining isn't perfect).
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-05 19:43:41 PDT
Might as well change it then, I think.
Comment 40 David Baron :dbaron: ⌚️UTC-10 2010-09-07 17:04:28 PDT
Oh, I needed to make a few adjustments as a result of try server testing:
 * in patch 8, I couldn't test rounded border against rounded background; that only matches on Linux.  So I tested rounded border against rounded border, and then rounded background against rounded background
 * the second test pointed out an occurrence that I missed in patch 7; I fixed that missed occurrence and also the FIXME comments in the patch
Comment 41 David Baron :dbaron: ⌚️UTC-10 2010-09-07 18:20:24 PDT
Created attachment 472869 [details] [diff] [review]
patch 11: remove unneeded nsDisplayClip::mClippingFrame
Comment 42 David Baron :dbaron: ⌚️UTC-10 2010-09-07 18:21:02 PDT
Created attachment 472871 [details] [diff] [review]
patch 12: remove unneeded nsAbsPosClipWrapper::mContainer
Comment 43 David Baron :dbaron: ⌚️UTC-10 2010-09-07 18:25:48 PDT
Created attachment 472875 [details] [diff] [review]
patch 13: add nsDisplayClipRoundedRect display item and handle it in FrameLayerBuilder

This is the most interesting patch, and also the one where I'm writing code in areas I know least well, so it deserves careful review.

I'm not crazy about some of the naming of the in FrameLayerBuilder.  (Something more descriptive than just |Clip|?)  I also went back and forth about whether ClippedDisplayItem should inherit from Clip.  If you have better ideas, I'm all ears.

I need to follow up about making hit testing follow the border radius.
Comment 44 David Baron :dbaron: ⌚️UTC-10 2010-09-07 18:27:23 PDT
Created attachment 472877 [details] [diff] [review]
patch 14: clip overflow!=visible to the border radius
Comment 45 David Baron :dbaron: ⌚️UTC-10 2010-09-07 18:32:58 PDT
Patch 15 lives in bug 485501.

I'm still working on the reftests for this bug; hopefully I'll finish them up tomorrow morning.  My current tests are:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/c68f3c465468/border-radius-clipping-reftests
but I'm seeing some failures due to pixels around the edges; I think they're problems with the tests rather than the patches, but I'll investigate further tomorrow.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-07 19:51:13 PDT
> I'm not crazy about some of the naming of the in FrameLayerBuilder.  (Something

The naming of what? :-)
Comment 47 David Baron :dbaron: ⌚️UTC-10 2010-09-07 20:05:20 PDT
Clip, RoundedRect.  (Also, what they're nested classes within.)
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-09-07 20:13:27 PDT
Comment on attachment 472875 [details] [diff] [review]
patch 13: add nsDisplayClipRoundedRect display item and handle it in FrameLayerBuilder

         aContext->NewPath();
+        PRInt32 A2D = presContext->AppUnitsPerDevPixel();
+        gfxRect clip = nsLayoutUtils::RectToGfxRect(currentClip.mClipRect, A2D);
         aContext->Rectangle(clip, PR_TRUE);
         aContext->Clip();
+
+        for (PRUint32 j = 0, jEnd = currentClip.mRoundedClipRects.Length();
+             j < jEnd; ++j) {
+          const Clip::RoundedRect &rr = currentClip.mRoundedClipRects[j];
+
+          gfxCornerSizes pixelRadii;
+          nsCSSRendering::ComputePixelRadii(rr.mRadii, A2D, &pixelRadii);
+
+          clip = nsLayoutUtils::RectToGfxRect(rr.mRect, A2D);
+          clip.Round();
+          clip.Condition();
+          // REVIEW: This might make contentArea empty.  Is that OK?
+
+          aContext->NewPath();
+          aContext->RoundedRectangle(clip, pixelRadii);
+          aContext->Clip();
+        }

I'd factor this out into a helper method Clip::ApplyTo(gfxContext*, nsPresContext*).

+    PRBool mHaveClipRect;

I'd put this last and make it PRPackedBool just on principle...

I think this is fine, actually I'm quite pleased it worked out neatly.

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