Closed
Bug 459144
Opened 16 years ago
Closed 14 years ago
Border radius should clip content in rounded corners when overflow != visible
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: cmtalbert, Assigned: dbaron)
References
(Blocks 1 open bug)
Details
Attachments
(16 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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•16 years ago
|
||
So is there a reason we should add code to prevent authors from doing this if they want?
Comment 2•16 years ago
|
||
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?
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•16 years ago
|
||
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•16 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)?
Comment 6•16 years ago
|
||
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: border-radius
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.
Alternatively, if it's possible, you could reduce the length of the scrollbars until they fit along the straight part of the border.
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 12•15 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.
Comment 14•15 years ago
|
||
http://frohsenwebdesign.com/projects/firefox-border-radius-bug/index.html Additional test case available online.
Assignee | ||
Comment 15•15 years ago
|
||
Note also that some of the code in the patch in bug 485501 might be useful here.
Comment 16•15 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•15 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•15 years ago
|
||
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•15 years ago
|
||
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.)
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
blocking2.0: --- → beta6+
Target Milestone: --- → mozilla2.0b6
Assignee | ||
Comment 21•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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 25•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Like the skip sides handling, all callers want this (including one current one).
Attachment #472088 -
Flags: review?(roc)
Assignee | ||
Comment 28•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
The display item I implement later needs this.
Attachment #472095 -
Flags: review?(roc)
Assignee | ||
Comment 32•14 years ago
|
||
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•14 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•14 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•14 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•14 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 43•14 years ago
|
||
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 45•14 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+
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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•