Closed Bug 232469 Opened 21 years ago Closed 20 years ago

fixed-position content can paint on top of right scrollbar

Categories

(Core :: Web Painting, defect, P2)

x86
All
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mjl+bmo, Assigned: roc)

References

()

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

spun off from bug 203086. in the testcase, the wide table and div paint on top
of the scrollbar.

bz apparently has this figured out and will add more detail shortly...
This seems to be a regression from bug 225811 -- backing that patch out locally
makes the testcase in bug 203086 happy again.

Sounds like we need a different way to handle clipping by scroll views?  Those
_should_ clip the abs/fixed pos kids....
*** Bug 232341 has been marked as a duplicate of this bug. ***
Oh, and as expected it's the first hunk of the patch on bug 225811 that causes
this bug.
The root scroll view is the only one that needs special treatment, right?
Not sure.... Should abs pos content really leak out of scrolling parents?
If the scrolling parent is not the containing block of the absolute content,
then I believe CSS2.1 currently says that the absolute content will leak out.

The root view is different because although in the frame hierarchy the
scrollframe is a sibling of the fixed position content, logically the
scrollframe is scrolling the viewport and thus should clip (although not scroll)
the fixed position content.
Ah, ok.  If it's not the containing block.  That makes sense.

Yeah, in that case this sounds like the root scroll view is what needs special
treatment.
I think we need to add a new view property NS_VIEW_FLAG_CLIPCONTENTCHILDREN, set
it on root scrolling views, and adjust nsView::GetClippedRect to take it into
account. That will probably require a second pass up the view ancestor chain
when non-containing-block ancestors are detected.
Attached patch patch (obsolete) — Splinter Review
Okay, here's the patch. Unfortunately it grew into a rework of most of the view
clipping code because of various issues that came up. Hopefully this code will
be less fragile than the old code turned out to be. This code identifies the
three types of clipping we need to support and makes them all explicit.

Thinking about this code exposed a latent bug in display list construction
which I have a patch for and will file separately.

One note: I moved the initialization of aTopView in CreateDisplayList() out to
the call site in BuildDisplayList, where it ought to have been all along.
Attached file test
This testcase illustrates the three types of clipping.
Comment on attachment 140344 [details] [diff] [review]
patch

Another views patch for you, David :-)
Attachment #140344 - Flags: superreview?(dbaron)
Attachment #140344 - Flags: review?(dbaron)
Attached file Another test (obsolete) —
Another test; I assume this patch handles this right?
that testcase is missing a </div> somewhere...
Attached file This is what I meant
Note that in current builds the text paints on top of the scrollbar, which
seems pretty bogus to me...
Attachment #140348 - Attachment is obsolete: true
There seems to be a reflow bug in current builds. I'd better take a look at
that. My patch is clipping the "Positioned" DIV though.
Attached patch revised patchSplinter Review
After sleeping on the original patch, I realized there were a couple of issues.
GetClippedView in the old patch optimized for the case when view->GetZParent()
is an ancestor of view->GetParent(), which is the wrong case ... the case we
should optimize for is when view->GetParent() is an ancestor of GetZParent(),
which is what the old #if 0 code did. So I fixed that up. Also, the storage of
clip rects in nsView has been made a bit simpler and more efficient.
Attachment #140344 - Flags: superreview?(dbaron)
Attachment #140344 - Flags: review?(dbaron)
Attachment #140393 - Flags: superreview?(dbaron)
Attachment #140393 - Flags: review?(dbaron)
I filed the reflow bug that causes text to overlap scrollbars as bug 232838.
Blocks: 183495
Blocks: 212376
Blocks: 186696
*** Bug 233648 has been marked as a duplicate of this bug. ***
Blocks: 234262
*** Bug 235950 has been marked as a duplicate of this bug. ***
*** Bug 155158 has been marked as a duplicate of this bug. ***
*** Bug 236488 has been marked as a duplicate of this bug. ***
Blocks: 236602
This bug has several dups even though it hasn't existed long, and it's a
regression, and the patch is nontrivial so I think it really needs 1.7b exposure.
Flags: blocking1.7b?
Attached file Testcase
Which of all those dependent bugs covers the case in the attachment (fixed pos
div overlapping the bottom scrollbar)?
*** Bug 236998 has been marked as a duplicate of this bug. ***
Comment on attachment 140393 [details] [diff] [review]
revised patch

r+sr=dbaron.  I may try to comment on the patch at a later date, but for now
we're probably better off just getting it in so it gets tested.
Attachment #140393 - Flags: superreview?(dbaron)
Attachment #140393 - Flags: superreview+
Attachment #140393 - Flags: review?(dbaron)
Attachment #140393 - Flags: review+
Comment on attachment 140393 [details] [diff] [review]
revised patch

layout regression, needs 1.7b exposure
Attachment #140393 - Flags: approval1.7b?
Comment on attachment 140393 [details] [diff] [review]
revised patch

a=chofmann for 1.7b
Attachment #140393 - Flags: approval1.7b? → approval1.7b+
checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Flags: blocking1.7b?
I think this checkin caused the DHTML problem in Bug 237426. With it removed the
testcase in that bug shows a smooth gradually expanding square, with it applied
the testcase jumps from a tiny square to a huge square.
Verified with firefox 0.10.1. My testcase from comment 23 is now fixed.
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: