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

RESOLVED FIXED

Status

()

Core
Layout: R & A Pos
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Eli Friedman, Assigned: Eli Friedman)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 256720 [details]
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.
(Assignee)

Comment 1

11 years ago
Created attachment 256752 [details] [diff] [review]
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.
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Attachment #256752 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Blocks: 372036
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.
(Assignee)

Comment 3

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

Comment 4

11 years ago
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.
Good catch. WG input would certainly be useful here...
(Assignee)

Comment 6

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

Updated

11 years ago
Blocks: 350231
(Assignee)

Comment 7

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

Comment 10

11 years ago
Created attachment 259504 [details] [diff] [review]
Patch v2
Attachment #256752 - Attachment is obsolete: true
Attachment #259504 - Flags: review?(roc)
+  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.
(Assignee)

Comment 12

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

Comment 14

11 years ago
Created attachment 259573 [details] [diff] [review]
Patch v3

I think this addresses all your comments so far.
Attachment #259504 - Attachment is obsolete: true
Attachment #259573 - Flags: review?(roc)
Attachment #259504 - Flags: review?(roc)
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?
Attachment #259573 - Flags: superreview+
Attachment #259573 - Flags: review?(roc)
Attachment #259573 - Flags: review+
(Assignee)

Comment 16

11 years ago
Created attachment 259649 [details] [diff] [review]
Patch with updated comments + reftest

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

Comment 17

11 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Depends on: 375508
You need to log in before you can comment on or make changes to this bug.