Bug 5909 - overlapping element leaves trail when scrolling iframe (affects Google Docs)
Summary: overlapping element leaves trail when scrolling iframe (affects Google Docs)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Critical
Assignee: Antti Koivisto
URL:
Keywords: GoogleBug, HasReduction, YahooBug
: 4378 16282 18838 25054 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-12-01 13:04 PST by mitz
Modified: 2010-03-04 22:54 PST (History)
16 users (show)

See Also:


Attachments
testcase (308 bytes, text/html)
2005-12-01 13:05 PST, mitz
no flags Details
testcase2 (316 bytes, text/html)
2006-01-11 14:34 PST, Geoffrey Garen
no flags Details
Prevent fast scrolling for subframes (4.29 KB, patch)
2006-01-23 07:52 PST, mitz
darin: review-
Details | Formatted Diff | Diff
check layer hierarchy for overlap (6.20 KB, patch)
2007-02-06 15:11 PST, Antti Koivisto
hyatt: review-
Details | Formatted Diff | Diff
Test for overlapping layers during painting and enable/disable blitting accordingly (21.33 KB, patch)
2009-04-24 13:50 PDT, mitz
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2005-12-01 13:04:44 PST
Summary: A positioned element in a layer above an iframe that has non-transparent background leaves 
trails behind when the iframe is scrolled. This happens with the "sender name" tags at the bottom-right 
corner of Gmail message views.

To reproduce: open the testcase and scroll down.
Comment 1 mitz 2005-12-01 13:05:32 PST
Created attachment 4896 [details]
testcase
Comment 2 Eric Seidel (no email) 2005-12-27 14:07:51 PST
It most certainly still does.  Ouch.
Comment 3 Joost de Valk (AlthA) 2005-12-27 14:12:59 PST
Making this one critical since it affects gmail...
Comment 4 Alice Liu 2006-01-09 17:21:12 PST
<rdar://problem/3954238>
Comment 5 Geoffrey Garen 2006-01-11 14:34:57 PST
Created attachment 5610 [details]
testcase2

Mitz's testcase is more true to the GMail setup, but this one reproduces the
problem more readily.

Just to be clear: the iframe's background color is essential to reproducing the
bug; the div's background color is not -- it's just there to make the bug more
apparent.
Comment 6 Darin Adler 2006-01-22 20:30:22 PST
This is easy to fix. It's caused by the setting of copiesOnScroll, which is set by calling QScrollView::setStaticBackground. I believe that the sense of the setStaticBackground boolean is backwards, by the way, because when it's true we do the slow scrolling and when it's false we do the fast scrolling. But the calling code in FrameView also looks backwards so it cancels out -- the useSlowRepaints field has a logical name.

Anyway, FrameView::clear and FrameView::layout both call setStaticBackground and ask for fast scrolling in some cases. But they can't do that if this frame overlaps any other element in front of it.

Since there's no fast way to find out if a frame has anything in front of it, we're in a bit of a quandry. We can change all the callers to never set the fast scrolling mode for a view for a subframe, but that will make scrolling slower for frames on frameset pages.

I think that's probably the best fix.

It's as simple as changing this:

    setStaticBackground(d->useSlowRepaints);

to this:

    setStaticBackground(d->useSlowRepaints && !m_frame->parentFrame());

Although I also think we should reverse the sense of setStaticBackground, and possible change its name to setUseSlowRepaints for clarity.
Comment 7 mitz 2006-01-23 07:52:07 PST
Created attachment 5875 [details]
Prevent fast scrolling for subframes

Essentially what Darin suggested.
Comment 8 Darin Adler 2006-01-23 08:47:49 PST
Comment on attachment 5875 [details]
Prevent fast scrolling for subframes

Looks great. r=me
Comment 9 Darin Adler 2006-01-23 11:37:29 PST
Comment on attachment 5875 [details]
Prevent fast scrolling for subframes

Dave said that this will slow things down too much for too many real pages. He doesn't have another short-term suggestion for fixing the bug, but he says this change is unacceptable.
Comment 10 Dave Hyatt 2006-01-23 11:42:00 PST
I don't think we can do this for frameset frames.  The performance regression would be way too large.  A better idea might be to do this only if you're an iframe.  Even then, this isn't really the correct long-term fix, since we really want an efficient way to know if something is obscured.

I think we should bite the bullet and fix this the right way.  We've need this "Is something in front of me?" function for some time now.  Such a function would be a sibling of paint and hitTest in the layer code.  It could walk the layers to find this information out (and then a further optimization would be to potentially cache such information for individual layers).

Fully occluded layers could even be culled out of painting and hit testing entirely.
Comment 11 Dave Hyatt 2006-01-23 11:43:28 PST
I would + a patch that did this only for iframes as long as we file a follow-up bug.
Comment 12 mitz 2006-02-02 03:36:24 PST
*** Bug 4378 has been marked as a duplicate of this bug. ***
Comment 13 David Kilzer (:ddkilzer) 2006-07-12 21:58:17 PDT
GMail is Google.

Comment 14 mitz 2006-07-12 22:32:28 PDT
I think at some point Google disabled those indicators in Safari.
Comment 15 Miles Bainbridge 2006-10-15 21:21:14 PDT
I read this bug last night and then had a thought this morning on it, in terms of an "is something in front of me" mechanism - a struct for each WebKit view that records a unique ID (numbered front-to-back) for each layer that is drawn in that view, and also records the dimensions and position of each layer.  This would allow for changing the position of a layer anywhere within the view, while maintaining separate information for each view.  Then, when layers are being dragged or scrolled, they would be able to check their layer ID in the struct to find out if there are any layers in front or behind, and then check the coordinates to see if there is overlap.  Of course, the re-drawing routine would have to be adjusted accordingly, but this may be better than re-doing the individual layers in OpenGL.

Hopefully that makes sense.  I know my programming skills aren't on par with you guys who will actually be fixing this. :)
Comment 16 Dave Hyatt 2006-10-15 21:41:50 PDT
We already have a sorted layer stack and yes, this could be used to determine if something overlaps and handle a lot of common cases.

However, there are a lot of ways to overlap an iframe that don't involve easily identifiable elements: text-shadow, negative margins, negative text-indent, floats, etc.  These cases aren't as important though.
Comment 17 Antti Koivisto 2007-02-06 07:25:32 PST
I'll take a look
Comment 18 Antti Koivisto 2007-02-06 15:11:00 PST
Created attachment 12985 [details]
check layer hierarchy for overlap

Crawl the layer hierarchy for potential frame overlaps, use slow repaints if something is found. It should handle common cases well and does not activate slow repaints unnecessarily.

Check is done from frame paint() which is not too nice. Don't know where else to put it, layer positioning is sort-of paint time thing. Caching could speed it up in expense of added compexity. Not sure if it is really worthwhile.
Comment 19 Dave Hyatt 2007-02-06 16:59:02 PST
Comment on attachment 12985 [details]
check layer hierarchy for overlap

I should start off by saying that I don't understand why this bug is a P1, since it's not a regression.

(1) The change to isStackingContext is not needed.  We already force the z-index of the root element to be 0 (and don't allow it to be auto).  This means it will always be a stacking context anyway.

(2) I'd prefer isRectangleOverlapped to rectangleIsOverlapped.

(3) if (l && l->rectangleIsOverlapped(IntRect(x, y, width(), height())))

You want contentWidth() and contentHeight() here.  The relevant box is the one that excludes border and padding.  Your x and y values are correct.

(4) I like the idea behind this, but I think this is too inefficient.  For pages with lots of layers and iframes, this will slow down painting.

(5) This approach will potentially put frames in the "penalty box" forever if a layer ever overlaps them.  A good example of this would be DHTML menus that come up on  top of an iframe.  If you open the menus and then close them, the iframe will now scroll slowly all the time just because a DHTML menu was opened on top of it at some point in the past.

My recommendation would be to defer this bug for now, since it really is a pretty high-risk feature to be implementing now.

Ultimately I think you need to know how to turn fast repaints back on (we manage to do this in the fixed positioned case as fixed positioned elements come and go, so you could look to that code as an example), and be smarter about caching knowledge of the overlap in the widget itself, only recomputing overlap when layers actually change positions.
Comment 20 Antti Koivisto 2007-02-06 17:50:16 PST
True, inability to turn fast mode back is actually a big problem. I'll leave this for now and fix later.
Comment 21 Dave Hyatt 2007-02-06 18:06:40 PST
I think the approach of checking the layers is a good one though.  Just need to cache the result and know how to get back into fast mode.
Comment 22 Maciej Stachowiak 2007-02-06 23:38:11 PST
If this no loger affects GMail, it doesn't need to be P1.
Comment 23 Brad 2007-09-23 14:16:51 PDT
I have an additional test case for this bug (I believe it is the same bug) for a project I was working on to convert a frameset with nested framesets into a page with a iframedframeset. One of the reasons I wanted to do this was to have positioned elements that overlap the content frame (now the IFRAME). I works in other browsers, and it works in Webkit when single pages are in the IFRAME, but it does not work correctly when a frameset is in the IFRAME in Safari/Webkit. Scolling the embedded frame also scrolls the positioned element, except where it overlaps the scrollbar. It's as though the positioned element was inside the embedded frame, instead of in front of all of it, except that scrolling back does not reveal it again.

Can I get rid of this by changing the background color of the iframe itself?

here is where you can see it yourself:

http://www.providentcu.org/goodiframesizing2.asp
Comment 24 mitz 2007-09-23 14:47:51 PDT
(In reply to comment #23)
> Can I get rid of this by changing the background color of the iframe itself?

No, I am afraid that to work around the bug you must change the document that is inside the <frame>, not the enclosing iframe. Changing the inner document's background color to transparent or adding a <div style="position:fixed"></div> to it should work.
Comment 25 Brad 2007-09-25 07:23:33 PDT
The document inside the iframe is a frameset. It only frames other other documents, so it cannot have any positioning or color of its own.
Comment 26 mitz 2007-09-25 07:29:23 PDT
(In reply to comment #25)
> The document inside the iframe is a frameset. It only frames other other
> documents, so it cannot have any positioning or color of its own.

I mean each one of those other documents (or those of them that might scroll, at least).
Comment 27 Brad 2007-09-25 07:33:41 PDT
Its pretty sad that of all the browsers I've tested, only Safari & Webkit have this problem. Even IE 6-7 don't have this problem, and they scroll perfectly well, and quickly too. IE, the crippled idiot of CSS can do something that Webkit can't! It is a sad day, indeed. And we've known about it since 2005?
Comment 28 Brad 2007-09-25 07:36:24 PDT
>I mean each one of those other documents (or those of them that might 
>scroll, at least).

Thanks, but that is just not a practical workaround, as the scrolling frame is for every single page of the site (several hundred) to appear in, and even a few pages from other sites.
Comment 29 mitz 2007-09-25 07:43:15 PDT
(In reply to comment #28)
> >I mean each one of those other documents (or those of them that might 
> >scroll, at least).
> 
> Thanks, but that is just not a practical workaround, as the scrolling frame is
> for every single page of the site (several hundred) to appear in, and even a
> few pages from other sites.

You can use JavaScript to make the changes dynamically (in fact, that is what I did to test the workaround before I suggested it), but I can see why this may not be desirable or even practical.
Comment 30 Brad 2007-09-26 08:46:01 PDT
>You can use JavaScript to make the changes dynamically (in fact, that is what I
did to test the workaround before I suggested it), but I can see why this may
not be desirable or even practical.

That doesn't help when the content is coming from another site (no cross-domain JS). We have several business partners that provide pages or Web applications to us as a service, where we have very limited control over how the documents are set up.

Its surprising that developers for Apple/Safari/Webkit don't consider themselves as good as IE developers in being able to fix this problem.
Comment 31 Dave Hyatt 2007-10-03 13:32:51 PDT
"Its surprising that developers for Apple/Safari/Webkit don't consider
themselves as good as IE developers in being able to fix this problem."

Yeah, we're just terrible.  Let's call in the IE developers to fix this.

Comment 32 mitz 2007-12-03 17:11:55 PST
*** Bug 16282 has been marked as a duplicate of this bug. ***
Comment 33 David Kilzer (:ddkilzer) 2007-12-04 01:46:22 PST
Adding YahooBug keyword for Bug 16282.
Comment 34 David Kilzer (:ddkilzer) 2008-06-19 18:18:01 PDT
Original Radar has been closed.  Changing keywords appropriately.
Comment 35 Simon Fraser (smfr) 2009-02-06 20:04:35 PST
Reproducible in latest nightly still.
Comment 36 Eric Seidel (no email) 2009-02-18 18:02:43 PST
This still affects http://docs.google.com/ when multiple people are viewing a doc, the "also editing: foobar, foobaz" blue dialog in the lower right hand corner leaves turds when scrolling.  docs add some CSS to work around the problem, but the CSS isn't working as far as I can tell.   This is tracked by the internal google bug 1157906.
Comment 37 Eric Seidel (no email) 2009-02-18 18:05:41 PST
Assuming this is the right bug (the scrolling bug which affects docs.google.com) then this was supposedly fixed by this change to Docs:

/* Prevent repaint errors when scrolling in Safari. This "Star-7" css hack
  82      targets Safari 3.1, but not WebKit nightlies and presumably Safari 4.
  83      That's OK because this bug is fixed in WebKit nightlies/Safari 4 :-). */
  84   html*#wys_frame::before {
  85     content: '\A0';
  86     position: fixed;
  87     overflow: hidden;
  88     width: 0;
  89     height: 0;
  90     top: 0;
  91     left: 0;
  92   }

As I said above though, I don't see this fix as working, at least not in tip of tree WebKit with a pre-release copy of Safari.
Comment 38 mitz 2009-04-24 13:33:17 PDT
*** Bug 25054 has been marked as a duplicate of this bug. ***
Comment 39 mitz 2009-04-24 13:50:48 PDT
Created attachment 29762 [details]
Test for overlapping layers during painting and enable/disable blitting accordingly

This patch lets a widgets (or in fact, any object, so in the future perhaps also overflow areas and media elements) make overlap-testing requests a paint time. The result is delivered by the end of the paint operation. There is no separate walk of the layer tree, so the results are guaranteed to reflect what was actually painted.

Transforms and reflections are not handled, but that is not a regression of course.

In the future, the interface can change so that setOverlapTestResult() takes two rects: the rect from the request and the overlapping layer's rect. This will allow the requester to cache its overlapped region and update it. Frames could use this to enable partial blitting, for example.
Comment 40 Dave Hyatt 2009-04-24 15:08:22 PDT
Comment on attachment 29762 [details]
Test for overlapping layers during painting and enable/disable blitting accordingly

This is great!  It looks very low risk to me as well, so I think it will be safe to take this patch now.  I have some minor comments, but you can just fix them and land with the changes:

>  
> +    if (overlapTestRequests) {
> +         Vector<OverlapTestRequester*> overlappedRequesters;

Fix indentation ^^^^^^

> +        RenderObject::OverlapTestRequestMap::iterator end = overlapTestRequests->end();
> +        for (RenderObject::OverlapTestRequestMap::iterator it = overlapTestRequests->begin(); it != end; ++it) {
> +            if (!layerBounds.intersects(it->second))
> +                continue;
> +
> +            it->first->setOverlapTestResult(true);
> +            overlappedRequesters.append(it->first);
> +        }
> +        for (size_t i = 0; i < overlappedRequesters.size(); ++i)
> +            overlapTestRequests->remove(overlappedRequesters[i]);
> +    }
> +

The above code should be moved into a separate helper function.


> Index: WebCore/rendering/RenderWidget.cpp
> ===================================================================
> --- WebCore/rendering/RenderWidget.cpp	(revision 42834)
> +++ WebCore/rendering/RenderWidget.cpp	(working copy)
> @@ -211,6 +211,11 @@ void RenderWidget::paint(PaintInfo& pain
>          // Tell the widget to paint now.  This is the only time the widget is allowed
>          // to paint itself.  That way it will composite properly with z-indexed layers.
>          m_widget->paint(paintInfo.context, paintInfo.rect);
> +
> +        if (m_widget->isFrameView() && paintInfo.overlapTestRequests) {
> +            ASSERT(!paintInfo.overlapTestRequests->contains(this));
> +            paintInfo.overlapTestRequests->set(this, m_widget->frameRect());
> +        }
>

Slow repaints are frequently already turned on for iframes (since they commonly have pages that don't specify a background in them).  You should avoid issuing an overlapTestRequest in this case if slow repaints (excluding overlap) are enabled already.

r=me!
Comment 42 Tony Chang 2010-03-04 22:54:45 PST
*** Bug 18838 has been marked as a duplicate of this bug. ***