If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

fixed-position content can paint on top of right scrollbar

RESOLVED FIXED

Status

()

Core
Layout: View Rendering
P2
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Michael Lefevre, Assigned: roc)

Tracking

({regression})

Trunk
x86
All
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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....
(Reporter)

Comment 2

14 years ago
*** 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.
Priority: -- → P2
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.
Created attachment 140344 [details] [diff] [review]
patch

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.
Created attachment 140345 [details]
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)
Created attachment 140348 [details]
Another test

Another test; I assume this patch handles this right?
that testcase is missing a </div> somewhere...
Created attachment 140371 [details]
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.
Created attachment 140393 [details] [diff] [review]
revised patch

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 - Attachment is obsolete: true
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.

Updated

14 years ago
Blocks: 183495

Updated

14 years ago
Blocks: 212376

Updated

14 years ago
Blocks: 186696
*** Bug 233648 has been marked as a duplicate of this bug. ***
Blocks: 234262

Comment 19

14 years ago
*** Bug 235950 has been marked as a duplicate of this bug. ***

Comment 20

14 years ago
*** Bug 155158 has been marked as a duplicate of this bug. ***

Comment 21

14 years ago
*** 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?

Comment 23

14 years ago
Created attachment 143484 [details]
Testcase

Which of all those dependent bugs covers the case in the attachment (fixed pos
div overlapping the bottom scrollbar)?

Comment 24

14 years ago
*** 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 27

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → FIXED
Flags: blocking1.7b?

Comment 29

14 years ago
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.

Comment 30

13 years ago
Verified with firefox 0.10.1. My testcase from comment 23 is now fixed.
You need to log in before you can comment on or make changes to this bug.