190383 - Fixed position elements blit incorrectly when scrolling

Status: VERIFIED FIXED

(Camino Graveyard :: Page Layout, defect)

Description

[Brian Ryner (not reading)]

Go to http://www.w3.org/Style/CSS/ and scroll down.  Note that the fixed
position menu in the upper-right redraws incorrectly as you scroll.

This doesn't happen in Carbon trunk builds, so it looks like it's a Cocoa widget
bug.

Comment 1

[Chris Petersen]

There was a previous bug written up regarding this problem.

Comment 2

[Chris Petersen]

*** Bug 190261 has been marked as a duplicate of this bug. ***

Comment 3

[ender_mb]

I don't think this is a Cocoa bug--it happens in Netscape 7.01 (Mozilla 1.0.2),
but not in more recent Mozilla builds (1.3a, for example), so it's probably an
issue with the Mozilla branch Chimera uses.

Comment 4

[Boris Zbarsky [:bzbarsky]]

See bug 93526 for more info... this is indeed fixed on trunk.

Comment 5

[Brian Ryner (not reading)]

Before y'all jump the gun on duping this...

There is a Cocoa-specific widget bug that causes this problem to appear even on
trunk builds.  The problem comes from the way we do scrolling in Cocoa, which is
where we just bitblt the entire NSView, which moves the bits without any regard
for sibling views that may be in the scrolled area.  I don't see any way to
change Cocoa's behavior in this regard, at least not without converting
wholesale to using NSScrollViews for scrolling.

So, I think the only thing we can do about it is to invalidate the rect that the
sibling view was blitted into (that is, the rect the height of the scroll delta,
for vertical scrolling, above or below the view in question).  I have a patch
which does this and it seems to fix the problem, at least on the w3.org site. 
I'm going on the assumption that we should avoid over-invalidating at all costs;
if it turns out that the view traversal and calculations are more expensive than
the invalidate then I'll have to come up with some sort of compromise.

I'd _love_ for someone to tell me that there's an easier way to fix this.

Comment 6

[Brian Ryner (not reading)]

Attachment: patch to nsChildView::Scroll()

Comment 7

[Brian Ryner (not reading)]

Attachment: patch to nsChildView::Scroll()

just a small cleanup in the while() loop

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 112672 [details] [diff] [review]
patch to nsChildView::Scroll()

pink, simon, any thoughts?

Comment 9

[Brian Ryner (not reading)]

Attachment: one more patch

Fixed vertInvalid.size.width and horizInvalid.size.height not being set
properly, also did a small bit of cleanup to make things more efficient.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 112720 [details] [diff] [review]
one more patch

I made the small change locally of not null-checking the return from [view
subviews], since it appears to always return an array, but I'm not going to
bother attaching a new patch.

Comment 11

[Simon Fraser [no longer active]]

Hrm, I thought this bug existed on the 1.0 branch, but not the trunk (which
would suggest that it wasn't in widget code). However, since this is a cocoa
widget fix, we might as well take it.

+      // To do this, start at the root Gecko NSView, and walk down along
+      // our ancestor view chain, looking at all the subviews in each level
+      // of the hierarchy.  If we find a non-ancestor view that overlaps
+      // this view, invalidate the area around it.
+
...
+      NSView* view = mParentView;

Are you assuming that mParentView is the root gecko view? I don't think that's
necessarily the case; mParentView is just this view's parent (kept in a member
var so that we can take this out of the view hierarchy, but still keep track of
its parent).

+        NSView* nextAncestorView;

It's not obvious that nextAncestorView will always get assigned to in this loop.
Probably better to initialize it with NULL.

I worry about how this will affect scrolling performace. Is there a way we can
avoid it, maybe by keeping a bit around to say that there are non-scrolling views?

Comment 12

[Brian Ryner (not reading)]

If you look at how mParentView is initialized... the toplevel nsChildView will
have a NSView passed in as the native parent.  All other nsChildViews inherit
mParentView from their parent.  So, as I'm reading it, all of the nsChildViews
for a given gecko instance will have mParentView pointing to the same NSView -
the parent view of the root nsChildView.

I'd prefer not to have to initialize nextAncestorView to null for each level of
the widget hierarchy, since there's no way that I can see for it not to get
assigned into.

The view system could note whether there are any sibling widgets covering this
widget (I think), but if an app had any sort of non-gecko NSView that
intersected the gecko content area if would have the same problem.

Comment 13

[Mike Pinkerton (not reading bugmail)]

this patch fixes the above case, but breaks the "post" page for a Blogger.com
blog. The top toolbar doesn't draw (just white space) and the "post & publish"
button bar doesn't draw when the iframe scrolls as the text gets larger.

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 112720 [details] [diff] [review]
one more patch

my bad about the patch not working. i tested the wrong version.

looks ok, but like simon, i wish there was a way we didn't have to do this
every time. however in my tests (long tbox page and w3c css page), the
scrolling speed didn't change.

r=pink

Comment 15

[Brian Ryner (not reading)]

checked in.

Comment 16

[Chris Petersen]

Marking verified with the 2003-01-30-07 NB under OS X 10.2.3.

Comment 17

[Greg K.]

*** Bug 192208 has been marked as a duplicate of this bug. ***