Closed Bug 485501 Opened 15 years ago Closed 14 years ago

image overflows the rounded border when using -moz-border-radius

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: stick, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

User-Agent:       Opera/9.64 (X11; Linux x86_64; U; en) Presto/2.1.1
Build Identifier: 

The behaviour and the workaround is described here:
http://stick.gk2.sk/blog/2009/03/image-with-rounded-corners-using-css3/

I'm attaching the testcase. Also I'm not sure if the behaviour is a bug or a feature according to CSS 3 specification.

Reproducible: Always

Steps to Reproduce:
1. unpack attachment
2 [review]. open index.html
3. compare two images
Attached file testscase (obsolete) —
So I think this is actually only supposed to work according to the spec if the image has 'overflow:hidden'.  I'm not sure if we'd do it given that, though.

http://dev.w3.org/csswg/css3-background/#the-border-radius does say:
  # It is recommended that the UA style sheet apply overflow: hidden to
  # elements (such as the <img> element in HTML) that are expected to be
  # replaced elements so that their corners automatically trim to the
  # border radius. 
but I'm pretty scared of the effects that would have on performance (since overflow:hidden implies scrollability), and I'm not particularly happy with the idea of the spec saying that.
Hardware: x86_64 → All
Summary: image overflows the border when using border-radius → image overflows the rounded border when using -moz-border-radius
I agree, we don't really want images to become scrollable, it would be a significant perf hit to wrap them all in scrollframes.
This is not the sort of thing that's worth taking a major perf hit for, but I wonder if it's really that bad.

Independent of whether the default behavior of images changes, is there an objection to having explicit overflow!=visible clip to the curve (as modified by the special license for visible scrollbars)?  That's bug 459144.

How much will compositor reduce the overhead of wrapping a scrollframe around an element?

<img> -- or more precisely nsImageFrame -- is special: as far as I can tell, it never overflows (in the absence of border-radius) and therefore the programmatic scrolling that overflow:hidden allows never actually does anything.  Could we perhaps make nsImageFrame do its own overflow:hidden clipping and provide a stub version of the programmatic scrolling interface, then?
(In reply to comment #7)
> Independent of whether the default behavior of images changes, is there an
> objection to having explicit overflow!=visible clip to the curve (as modified
> by the special license for visible scrollbars)?  That's bug 459144.

That sounds OK to me.

> How much will compositor reduce the overhead of wrapping a scrollframe around
> an element?

Not much. We already don't create a native widget for overflow:hidden content.

> <img> -- or more precisely nsImageFrame -- is special: as far as I can tell, it
> never overflows (in the absence of border-radius) and therefore the
> programmatic scrolling that overflow:hidden allows never actually does
> anything.  Could we perhaps make nsImageFrame do its own overflow:hidden
> clipping and provide a stub version of the programmatic scrolling interface,
> then?

We don't need to provide a stub implementation of anything, the default behaviour of scroll* properties for non-scrollable elements should be fine. Perhaps we can just special-case nsImageFrame and certain other frame types so that overflow:hidden doesn't create scrollframes for them.
I'm going to confirm this, because I'm experiencing it, too, and it's kinda a pain.

Also, I don't know if any of this speculation has actually been tested, but this doesn't just happen when the image itself is styled with rounded borders. If you have an image inside of a <p>, for example, and the <p> is the one styled with curved borders and hidden overflow, the image still shows through.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Attached file Expanded testcase
It may be that the <p> has to be styled to be the same size as the image, but I've expanded the original testcase to try to demonstrate both problems in the same file.
Attachment #369650 - Attachment is obsolete: true
Attachment #382504 - Attachment is obsolete: true
According to http://lists.w3.org/Archives/Public/www-style/2009Aug/0589.html this has been changed to a special case for replaced elements.

So we should probably just go ahead and fix this explicitly for nsImageFrame, nsHTMLCanvasFrame, nsHTMLVideoElement, and probably nsObjectFrame.  (I'm not sure whether we should touch nsFrameFrame or HTML form controls.)
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6a0b99dc03a9/clip-to-border-radius has work in progress (works, but no tests yet, and haven't tested anything other than images) for replaced elements.

(We also need to do the clipping for overflow-not-visible; that's an entirely separate patch.)
(In reply to comment #12)
> (We also need to do the clipping for overflow-not-visible; that's an entirely
> separate patch.)

...which should go in bug 459144.
Hello, any news on this issue? Sorry if “bumping” a bug is against Bugzilla etiquette. I don't have much to add, except that I regret this could not be improved in Firefox4 (unless feature freeze has not been reached yet?), and that web designers wanting to design with CSS3—an increasing trend—might not think this issue is "minor". ;)
Blocks: 451134
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
blocking2.0: --- → beta6+
Target Milestone: --- → mozilla2.0b6
Attached patch patch (obsolete) — Splinter Review
This applies on top of the patches in bug 459144, where the bulk of the work for this bug is.
Attachment #472878 - Flags: review?(roc)
+nsIFrame::DisplayReplacedContentInBorderRadius(nsDisplayListBuilder* aBuilder,

I think existing conventions would favour a name starting with BuildDisplayList ... say BuildDisplayListForReplacedContent?

But another way of doing this would be to have the replaced elements just create an nsDisplayList containing the replaced-content, and then have a method (possibly in nsLayoutUtils) WrapReplacedContentForBorderRadius(nsIFrame* aFrame, nsDisplayList* aList). I think that would be a bit cleaner.
Attachment #472878 - Attachment is obsolete: true
Attachment #472944 - Flags: review?(roc)
Attachment #472878 - Flags: review?(roc)
Depends on: 596889
Depends on: 599368
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.