Last Comment Bug 372037 - CSS clip property should clip the overflow rect as well as the paint rect
: CSS clip property should clip the overflow rect as well as the paint rect
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: R & A Pos (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Eli Friedman
:
Mentors:
Depends on: 375508
Blocks: 350231 372036
  Show dependency treegraph
 
Reported: 2007-02-27 16:07 PST by Eli Friedman
Modified: 2007-03-27 01:36 PDT (History)
7 users (show)
jwalden+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (384 bytes, text/html)
2007-02-27 16:07 PST, Eli Friedman
no flags Details
Patch (11.25 KB, patch)
2007-02-27 20:50 PST, Eli Friedman
no flags Details | Diff | Splinter Review
Patch v2 (11.50 KB, patch)
2007-03-24 03:31 PDT, Eli Friedman
no flags Details | Diff | Splinter Review
Patch v3 (12.24 KB, patch)
2007-03-25 04:57 PDT, Eli Friedman
roc: review+
roc: superreview+
Details | Diff | Splinter Review
Patch with updated comments + reftest (18.45 KB, patch)
2007-03-25 18:46 PDT, Eli Friedman
no flags Details | Diff | Splinter Review

Description Eli Friedman 2007-02-27 16:07:23 PST
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.
Comment 1 Eli Friedman 2007-02-27 20:50:35 PST
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.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-02-28 01:41:21 PST
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 2007-02-28 06:43:36 PST
(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 2007-02-28 08:34:59 PST
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) (Exited; email my personal email if necessary) 2007-02-28 10:35:06 PST
Good catch. WG input would certainly be useful here...
Comment 6 Eli Friedman 2007-02-28 11:24:41 PST
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 2007-03-13 17:47:07 PDT
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) (Exited; email my personal email if necessary) 2007-03-13 17:52:09 PDT
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+2 (mostly busy through August 4; review requests must explain patch) 2007-03-13 18:08:51 PDT
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 2007-03-24 03:31:12 PDT
Created attachment 259504 [details] [diff] [review]
Patch v2
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-24 03:48:10 PDT
+  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 2007-03-24 04:01:28 PDT
(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) (Exited; email my personal email if necessary) 2007-03-24 04:05:41 PDT
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 2007-03-25 04:57:08 PDT
Created attachment 259573 [details] [diff] [review]
Patch v3

I think this addresses all your comments so far.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-03-25 15:36:30 PDT
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 2007-03-25 18:46:08 PDT
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.)
Comment 17 Eli Friedman 2007-03-26 21:36:03 PDT
Checked in.

Note You need to log in before you can comment on or make changes to this bug.