459144 - Border radius should clip content in rounded corners when overflow != visible

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[cmtalbert]

Attachment: 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-4, no longer working on Mozilla)]

So is there a reason we should add code to prevent authors from doing this if they want?

Comment 2

[Zack Weinberg (:zwol)]

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]

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

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]

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

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]

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]

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

cc:ing faaborg for UI input.

Comment 12

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

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

[Brandon Frohs]

http://frohsenwebdesign.com/projects/firefox-border-radius-bug/index.html

Additional test case available online.

Comment 15

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

Note also that some of the code in the patch in bug 485501 might be useful here.

Comment 16

[Brandon Frohs]

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

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]

Attachment: 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]]

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 21

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

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords

and a revised patch 2

Comment 25

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

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: patch 9: expose nsCSSRendering::ComputePixelRadii

The display item I implement later needs this.

Comment 32

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

Attachment: 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)]

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-4, no longer working on Mozilla)]

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

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

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-4, no longer working on Mozilla)]

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

Might as well change it then, I think.

Comment 39

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

Landed patch 1 through patch 10:
http://hg.mozilla.org/mozilla-central/rev/baa2fb78f457
http://hg.mozilla.org/mozilla-central/rev/ba996cfeb9f7
http://hg.mozilla.org/mozilla-central/rev/5c60f37c6534
http://hg.mozilla.org/mozilla-central/rev/fc14d7ba0703
http://hg.mozilla.org/mozilla-central/rev/be6a7ccc34fe
http://hg.mozilla.org/mozilla-central/rev/c15d3e99d3de
http://hg.mozilla.org/mozilla-central/rev/a118b42abad1
http://hg.mozilla.org/mozilla-central/rev/76358146c914
http://hg.mozilla.org/mozilla-central/rev/433efb14d970
http://hg.mozilla.org/mozilla-central/rev/6a28138050f5

Comment 40

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

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-4, no longer working on Mozilla)]

Attachment: patch 11: remove unneeded nsDisplayClip::mClippingFrame

Comment 42

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

Attachment: patch 12: remove unneeded nsAbsPosClipWrapper::mContainer

Comment 43

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

Attachment: 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-4, no longer working on Mozilla)]

Attachment: patch 14: clip overflow!=visible to the border radius

Comment 45

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

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

> I'm not crazy about some of the naming of the in FrameLayerBuilder.  (Something

The naming of what? :-)

Comment 47

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

Clip, RoundedRect.  (Also, what they're nested classes within.)

Comment 48

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

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.

Comment 49

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

http://hg.mozilla.org/mozilla-central/rev/c235307e637f
http://hg.mozilla.org/mozilla-central/rev/bec3c3b989c8
http://hg.mozilla.org/mozilla-central/rev/6e3abe4d1d2d
http://hg.mozilla.org/mozilla-central/rev/082bd0be8bc0
http://hg.mozilla.org/mozilla-central/rev/da15f373d499
http://hg.mozilla.org/mozilla-central/rev/94cf996b52d0