Bug 52735 – [Qt] painting of windowed plugins faulty on certain scroll events

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Robert Hogan]

I noticed this while fixing the specific issue in bug 52734. The patch at bug 52734 doesn't fix it.

If you open plugins/iframe-shims.html in QtTestBrowser and resize to narrow rectangle, then shorten the window so that you can scroll up and down about 100 pixels in one step, you'll notice that after scrolling up and down once or twice (single steps, gradual scrolling won't reproduce) that the blue plugins disappear from the first two boxes in the top row. Instead they get rendered over the plugins on the two rows beneath the top row. 

I can't take a screenshot of this because ksnapshot repaints the entire screen first, which results in the plugins getting repainted properly.

This alone suggests we need to trigger an extra paint event on the frame at some point, but I'm not sure when.

[Robert Hogan]

This is happening in:


void RenderWidget::paint(PaintInfo& paintInfo, int tx, int ty)
{
    if (!shouldPaint(paintInfo, tx, ty))
        return;

    tx += x();
    ty += y();

    if (hasBoxDecorations() && (paintInfo.phase == PaintPhaseForeground || paintInfo.phase == PaintPhaseSelection))
        paintBoxDecorations(paintInfo, tx, ty);

    if (paintInfo.phase == PaintPhaseMask) {
        paintMask(paintInfo, tx, ty);
        return;
    }

    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
        return;


Commenting out both:


    if (!shouldPaint(paintInfo, tx, ty))
        return;

and


    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
        return;

fixes it. Commenting out one or the other does not. It's very hard to debug paint events so I don't really stand a chance without a few pointers, but this 'fixes' it:

--- a/Source/WebCore/rendering/RenderReplaced.cpp
+++ b/Source/WebCore/rendering/RenderReplaced.cpp
@@ -157,18 +157,17 @@ void RenderReplaced::paint(PaintInfo& paintInfo, int tx, int ty)
 
 bool RenderReplaced::shouldPaint(PaintInfo& paintInfo, int& tx, int& ty)
 {
    if (paintInfo.phase != PaintPhaseForeground && paintInfo.phase != PaintPhaseOutline && paintInfo.phase != PaintPhaseSelfOutline
             && paintInfo.phase != PaintPhaseSelection && paintInfo.phase != PaintPhaseMask)
         return false;

     if (!paintInfo.shouldPaintWithinRoot(this))
         return false;
        
     // if we're invisible or haven't received a layout yet, then just bail.
     if (style()->visibility() != VISIBLE)
         return false;
 
-    int currentTX = tx + x();
     int currentTY = ty + y();
 
     // Early exit if the element touches the edges.
@@ -180,10 +179,9 @@ bool RenderReplaced::shouldPaint(PaintInfo& paintInfo, int& tx, int& ty)
         top = min(selTop, top);
         bottom = max(selBottom, bottom);
     }

     int os = 2 * maximalOutlineSize(paintInfo.phase);
-    if (currentTX + leftVisualOverflow() >= paintInfo.rect.right() + os || currentTX + rightVisualOverflow() <= paintInfo.rect.x() - os)
-        return false;
     if (top >= paintInfo.rect.bottom() + os || bottom <= paintInfo.rect.y() - os)
         return false;
 
diff --git a/Source/WebCore/rendering/RenderWidget.cpp b/Source/WebCore/rendering/RenderWidget.cpp
index 13b572d..8f07d40 100644
--- a/Source/WebCore/rendering/RenderWidget.cpp
+++ b/Source/WebCore/rendering/RenderWidget.cpp
@@ -260,7 +260,7 @@ void RenderWidget::paint(PaintInfo& paintInfo, int tx, int ty)
         return;
     }
 
-    if (!m_frameView || paintInfo.phase != PaintPhaseForeground || style()->visibility() != VISIBLE)
+    if (!m_frameView /*|| paintInfo.phase != PaintPhaseForeground */|| style()->visibility() != VISIBLE)
         return;


I'm not even sure what:


-    if (currentTX + leftVisualOverflow() >= paintInfo.rect.right() + os || currentTX + rightVisualOverflow() <= paintInfo.rect.x() - os)
-        return false;

is doing.

[Dave Hyatt]

Just a guess but make sure the dirty rect you pass in is correct.  There's nothing wrong with the cross-platform code there.

[Robert Hogan]

(In reply to comment #2)
> Just a guess but make sure the dirty rect you pass in is correct.  There's nothing wrong with the cross-platform code there.

Right enough, another 'fix' is:


--- a/Source/WebKit/qt/Api/qwebframe.cpp
+++ b/Source/WebKit/qt/Api/qwebframe.cpp
@@ -374,6 +374,7 @@ void QWebFramePrivate::renderRelativeCoords(GraphicsContext* context, QWebFrame:
             QRect rect = intersectedRect;
             context->translate(x, y);
             rect.translate(-x, -y);
+            view->paintContents(context, rect);
             context->translate(-scrollX, -scrollY);
             rect.translate(scrollX, scrollY);
             context->clip(view->visibleContentRect());

It looks like RenderWidget::paint() needs to test the rect using its absolute co-ordinates in the frame but QWebFrame only supplies relative co-ordinates. I imagine this is pretty unique to plugins. And Qt is the only port calling frameview->paintContents() directly when rendering the page.

So I think Qt needs to test the page for plugins/widgets and make sure they always get painted.

[Robert Hogan]

There is something I don't understand about RenderWidget::paint(). If a Scroll event has moved the ScrollView down by exactly or just slightly more than the height of the RenderWidget shouldPaint() in RenderWidget::paint() will never pass.

In the case of this bug the result is that the windowed plugin does not move when a single scroll event moves the viewport down by the same amount as (or slightly more than) the height of the plugin. 

So doesn't ScrollView need to take account of this somehow?

[Robert Hogan]

Created attachment 81403 [details]
Patch

[Robert Hogan]

(In reply to comment #5)
> Created an attachment (id=81403) [details]
> Patch

I've look at PluginViewSymbian and PluginViewWin - neither have the code I'm altering here. I don't have the set-up for testing either of them unfortunately.

[Andreas Kling]

Comment on attachment 81403 [details]
Patch

r=me, thanks for digging into this!

[Alexis Menard (darktears)]

Comment on attachment 81403 [details]
Patch

Sounds good to me. I agree with you conclusion investigating myself. It's really complex to reproduce, not sure we can do it through a tests. I ran the LayoutTests on my machine and didn't catch any regressions.

[WebKit Commit Bot]

Comment on attachment 81403 [details]
Patch

Clearing flags on attachment: 81403

Committed r79397: <http://trac.webkit.org/changeset/79397>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.