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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: cmtalbert, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(16 attachments, 2 obsolete attachments)

5.00 KB, text/html
Details
3.76 KB, image/png
Details
11.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.45 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.38 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.46 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.67 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
29.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.17 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
So is there a reason we should add code to prevent authors from doing this if they want?
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?
(Reporter)

Comment 3

9 years ago
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.
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

8 years ago
>  - 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)?
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).
Blocks: 431176

Comment 7

8 years ago
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

8 years ago
Alternatively, if it's possible, you could reduce the length of the scrollbars until they fit along the straight part of the border.
cc:ing faaborg for UI input.
Keywords: uiwanted
(Assignee)

Updated

8 years ago
Depends on: 485501
Keywords: uiwanted
Summary: Border radius causes borders to be drawn inside content → Border radius should clip content in rounded corners when overflow != visible
(Assignee)

Updated

8 years ago
Duplicate of this bug: 537433
(Assignee)

Updated

8 years ago
Duplicate of this bug: 409547
(Assignee)

Comment 12

8 years ago
(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.

Updated

8 years ago
Duplicate of this bug: 536629

Comment 14

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

Additional test case available online.
(Assignee)

Comment 15

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

Comment 16

8 years ago
(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

7 years ago
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

7 years ago
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
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.)

Updated

7 years ago
Duplicate of this bug: 575252

Updated

7 years ago
Blocks: 451134
(Assignee)

Updated

7 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
blocking2.0: --- → beta6+
Target Milestone: --- → mozilla2.0b6
(Assignee)

Comment 21

7 years ago
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.)
Attachment #472074 - Flags: review?(roc)
(Assignee)

Comment 22

7 years ago
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.
Attachment #472075 - Flags: review?(roc)
(Assignee)

Comment 23

7 years ago
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.
Attachment #472074 - Attachment is obsolete: true
Attachment #472075 - Attachment is obsolete: true
Attachment #472082 - Flags: review?(roc)
Attachment #472074 - Flags: review?(roc)
Attachment #472075 - Flags: review?(roc)
(Assignee)

Comment 24

7 years ago
Created attachment 472083 [details] [diff] [review]
patch 2: make ComputeBorderRadius take an nsSize instead of two nscoords

and a revised patch 2
Attachment #472083 - Flags: review?(roc)
(Assignee)

Comment 25

7 years ago
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).
Attachment #472086 - Flags: review?(roc)
(Assignee)

Comment 26

7 years ago
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.
Attachment #472087 - Flags: review?(roc)
(Assignee)

Comment 27

7 years ago
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).
Attachment #472088 - Flags: review?(roc)
(Assignee)

Comment 28

7 years ago
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.
Attachment #472090 - Flags: review?(roc)
(Assignee)

Comment 29

7 years ago
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.
Attachment #472091 - Flags: review?(roc)
(Assignee)

Comment 30

7 years ago
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.)
Attachment #472094 - Flags: review?(roc)
(Assignee)

Comment 31

7 years ago
Created attachment 472095 [details] [diff] [review]
patch 9: expose nsCSSRendering::ComputePixelRadii

The display item I implement later needs this.
Attachment #472095 - Flags: review?(roc)
(Assignee)

Comment 32

7 years ago
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.
Attachment #472096 - Flags: review?(roc)
Attachment #472082 - Flags: review?(roc) → review+
Attachment #472083 - Flags: review?(roc) → review+
Attachment #472086 - Flags: review?(roc) → review+
Attachment #472087 - Flags: review?(roc) → review+
Attachment #472088 - Flags: review?(roc) → review+
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 :-).
Attachment #472090 - Flags: review?(roc) → review+
(Assignee)

Comment 34

7 years ago
(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).
Attachment #472091 - Flags: review?(roc) → review+
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.
Attachment #472094 - Flags: review?(roc) → review+
Attachment #472095 - Flags: review?(roc) → review+
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?
Attachment #472096 - Flags: review?(roc) → review+
(Assignee)

Comment 37

7 years ago
(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).
Might as well change it then, I think.
(Assignee)

Comment 39

7 years ago
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
(Assignee)

Comment 40

7 years ago
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
(Assignee)

Comment 41

7 years ago
Created attachment 472869 [details] [diff] [review]
patch 11: remove unneeded nsDisplayClip::mClippingFrame
Attachment #472869 - Flags: review?(roc)
(Assignee)

Comment 42

7 years ago
Created attachment 472871 [details] [diff] [review]
patch 12: remove unneeded nsAbsPosClipWrapper::mContainer
Attachment #472871 - Flags: review?(roc)
(Assignee)

Comment 43

7 years ago
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.
Attachment #472875 - Flags: review?(roc)
(Assignee)

Comment 44

7 years ago
Created attachment 472877 [details] [diff] [review]
patch 14: clip overflow!=visible to the border radius
Attachment #472877 - Flags: review?(roc)
(Assignee)

Comment 45

7 years ago
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.
Attachment #472869 - Flags: review?(roc) → review+
> I'm not crazy about some of the naming of the in FrameLayerBuilder.  (Something

The naming of what? :-)
Attachment #472871 - Flags: review?(roc) → review+
(Assignee)

Comment 47

7 years ago
Clip, RoundedRect.  (Also, what they're nested classes within.)
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.
Attachment #472875 - Flags: review?(roc) → review+
Attachment #472877 - Flags: review?(roc) → review+
(Assignee)

Comment 49

7 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 596271
(Assignee)

Updated

7 years ago
Depends on: 596205
(Assignee)

Updated

7 years ago
Depends on: 595842

Updated

7 years ago
Depends on: 613905
You need to log in before you can comment on or make changes to this bug.