372037 - CSS clip property should clip the overflow rect as well as the paint rect

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Eli Friedman]

Attachment: Testcase

Per summary; testcase coming up, plus a patch sometime soon.  I think the testcase shouldn't show scrollbars, since there's nothing to scroll to, but it does in all current browsers.

This is a significant change in behavior, and will make us incompatible with IE and Opera; however, I think it makes a lot more sense than what we're doing now.

There might be a bug filed on this already, although I couldn't find it.

Comment 1

[Eli Friedman]

Attachment: Patch

This patch has the necessary changes plus some cleanup to the invalidation logic made possible by this change.

I'm not completely sure that we want to do this; it does seem logical, but it's not consistent with the way both Gecko and other browsers (I tested Opera and IE) deal with clip and overflow.

I believe this patch is safe... the places that use overflow rects for purposes other than painting won't be affected.

I'll check in the testcase attached to this bug as a reftest along with the patch.

Comment 2

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

Does this cause assertions to fire if you have an element that uses 'clip' and 'overflow:auto'? I guess not because it's the scrollframe that ends up clipped, not the scrolled frame.

Maybe we should get some other opinions on whether we should do this.

Comment 3

[Eli Friedman]

(In reply to comment #2)
> Does this cause assertions to fire if you have an element that uses 'clip' and
> 'overflow:auto'? I guess not because it's the scrollframe that ends up clipped,
> not the scrolled frame.

It doesn't assert, but it's broken on trunk, and has been for a long time.  Guess that's another bug to file.

> Maybe we should get some other opinions on whether we should do this.

Agreed... where should we ask?

Comment 4

[Eli Friedman]

Hmm, I tried searching www-style.  Found http://lists.w3.org/Archives/Public/www-style/2003Aug/0075, which seems to be this issue; unfortunately, nobody replied... I guess I should CC dbaron and bz at the very least.

Comment 5

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

Good catch. WG input would certainly be useful here...

Comment 6

[Eli Friedman]

Comment on attachment 256752 [details] [diff] [review]
Patch

Dropping the review request for the moment because there's an invalication bug switching from non-auto clip to auto clip.  (I have a fixed version I'll post sometime soon.)

Comment 7

[Eli Friedman]

Hmm, didn't really get much of a response off the www-style list; only one person seemed to have anything to say, and he was mostly asking for clarifications.  Is that an implicit approval?

Comment 8

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

I think so. Let's go for it. I can't think of any issues that this would cause with real content, off the top of my head.

Comment 9

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

Just make sure we don't clip things that the 'clip' property doesn't apply to.  (It looks like you're ok on that, but you might want to add tests for it to reftest.)

Comment 10

[Eli Friedman]

Attachment: Patch v2

Comment 11

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

+  hasAbsPosClip = GetAbsPosClipRect(GetStyleDisplay(), &absPosClipRect);

There's a GetStyleDisplay() earlier on which you should reuse ("disp"). Actually we should reuse it here too:
if (geometricOverflow &&
       GetStyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_CLIP) {
and in the NS_ASSERTION as well.

+  if (hasOutline || geometricOverflow || hasAbsPosClip) {

Why are you adding hasAbsPosClip here? If we didn't already overflow, we won't start overflowing just because there's a clip.

Comment 12

[Eli Friedman]

(In reply to comment #11)
> +  hasAbsPosClip = GetAbsPosClipRect(GetStyleDisplay(), &absPosClipRect);
> 
> There's a GetStyleDisplay() earlier on which you should reuse ("disp").
> Actually we should reuse it here too:
> if (geometricOverflow &&
>        GetStyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_CLIP) {
> and in the NS_ASSERTION as well.

Okay.

> +  if (hasOutline || geometricOverflow || hasAbsPosClip) {
> 
> Why are you adding hasAbsPosClip here? If we didn't already overflow, we won't
> start overflowing just because there's a clip.

If hasAbsPosClip is true, the overflow rect could be different from the frame boundaries.  Note the the overflow rect can end up being smaller than the frame's rect.

Comment 13

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

Ah, hrm. That doesn't sound good. We may find bugs where we're assuming that the overflow area contains the frame bounds. But I guess they are most likely to manifest in relatively innocuous ways.

Comment 14

[Eli Friedman]

Attachment: Patch v3

I think this addresses all your comments so far.

Comment 15

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

Comment on attachment 259573 [details] [diff] [review]
Patch v3

Can you add a comment to the definition of NS_FRAME_OUTSIDE_CHILDREN that it can be set when the "overflow" rect is *smaller* than the frame size, as well?

Comment 16

[Eli Friedman]

Attachment: Patch with updated comments + reftest

I'll check this in sometime soon.  (Note to self: make sure to check in the right nsIFrame.h.)

Comment 17

[Eli Friedman]

Checked in.