Bug 5909 – overlapping element leaves trail when scrolling iframe (affects Google Docs)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

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.

Created attachment 4896 [details]
testcase

[Eric Seidel (no email)]

It most certainly still does.  Ouch.

[Joost de Valk (AlthA)]

Making this one critical since it affects gmail...

[Alice Liu]

<rdar://problem/3954238>

[Geoffrey Garen]

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.

[Darin Adler]

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.

Created attachment 5875 [details]
Prevent fast scrolling for subframes

Essentially what Darin suggested.

[Darin Adler]

Comment on attachment 5875 [details]
Prevent fast scrolling for subframes

Looks great. r=me

[Darin Adler]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

I would + a patch that did this only for iframes as long as we file a follow-up bug.

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

[David Kilzer (:ddkilzer)]

GMail is Google.

I think at some point Google disabled those indicators in Safari.

[Miles Bainbridge]

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. :)

[Dave Hyatt]

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.

[Antti Koivisto]

I'll take a look

[Antti Koivisto]

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.

[Dave Hyatt]

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.

[Antti Koivisto]

True, inability to turn fast mode back is actually a big problem. I'll leave this for now and fix later.

[Dave Hyatt]

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.

[Maciej Stachowiak]

If this no loger affects GMail, it doesn't need to be P1.

[Brad]

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

(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.

[Brad]

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.

(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).

[Brad]

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?

[Brad]

>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.

(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.

[Brad]

>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.

[Dave Hyatt]

"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.

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

[David Kilzer (:ddkilzer)]

Adding YahooBug keyword for Bug 16282.

[David Kilzer (:ddkilzer)]

Original Radar has been closed.  Changing keywords appropriately.

[Simon Fraser (smfr)]

Reproducible in latest nightly still.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

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

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.

[Dave Hyatt]

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!

Fixed in <http://trac.webkit.org/projects/webkit/changeset/42849> and <http://trac.webkit.org/projects/webkit/changeset/42850>.

[Tony Chang]

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