52735 2011-01-19 13:00:03 -0800 [Qt] painting of windowed plugins faulty on certain scroll events 2011-02-22 21:31:15 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED Qt, QtTriaged P2 Normal --- 1 robert webkit-unassigned commit-queue girish hausmann hyatt kenneth vestbo oldest_to_newest 336730 0 robert 2011-01-19 13:00:03 -0800 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. 342750 1 robert 2011-01-31 12:35:34 -0800 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. 342764 2 hyatt 2011-01-31 12:51:22 -0800 Just a guess but make sure the dirty rect you pass in is correct. There's nothing wrong with the cross-platform code there. 342789 3 robert 2011-01-31 13:37:03 -0800 (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. 346196 4 robert 2011-02-05 14:48:46 -0800 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? 346288 5 81403 robert 2011-02-06 07:45:34 -0800 Created attachment 81403 Patch 350260 6 robert 2011-02-12 05:03:41 -0800 (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. 355153 7 81403 kling 2011-02-22 02:40:16 -0800 Comment on attachment 81403 Patch r=me, thanks for digging into this! 355219 8 81403 menard 2011-02-22 06:17:19 -0800 Comment on attachment 81403 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. 355833 9 81403 commit-queue 2011-02-22 21:31:10 -0800 Comment on attachment 81403 Patch Clearing flags on attachment: 81403 Committed r79397: <http://trac.webkit.org/changeset/79397> 355834 10 commit-queue 2011-02-22 21:31:15 -0800 All reviewed patches have been landed. Closing bug. 81403 2011-02-06 07:45:34 -0800 2011-02-22 21:31:10 -0800 Patch bug-52735-20110206154534.patch text/plain 1872 robert ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA1NzEwNGY3MmI0YjAyYzkxN2M5ZTVhZTRkMzE3MTRlMTc0MDA0MDZlLi4y ZGQzMDk0MTk1Mjk5ZjhiYjY3ZjkxYjJiNmRmOTFmMTkzYmY2NWY5IDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0x LDMgKzEsMTggQEAKKzIwMTEtMDItMDYgIFJvYmVydCBIb2dhbiAgPHJvYmVydEB3ZWJraXQub3Jn PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtRdF0g cGFpbnRpbmcgb2Ygd2luZG93ZWQgcGx1Z2lucyBmYXVsdHkgb24gY2VydGFpbiBzY3JvbGwgZXZl bnRzCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUy NzM1CisKKyAgICAgICAgSW52YWxpZGF0ZSB0aGUgcGx1Z2ludmlldydzIHJlbGF0aXZlIHJlY3Qg cmF0aGVyIHRoZW4gdGhlIGZyYW1lUmVjdCgpLiBUaGlzIGlzIGJlY2F1c2UgUVdlYkZyYW1lOjpy ZW5kZXJSZWxhdGl2ZUNvb3JkcygpCisgICAgICAgIGltaXRhdGVzIFNjcm9sbFZpZXcgYW5kIGFk ZHMgdGhlIHNjcm9sbCBvZmZzZXQgYmFjayBvbiB0byB0aGUgcmVjdCB3ZSBkYW1hZ2UgaGVyZSAo bWFraW5nIHRoZSBjby1vcmRpbmF0ZXMgYWJzb2x1dGUKKyAgICAgICAgdG8gdGhlIGZyYW1lIGFn YWluKSBiZWZvcmUgcGFzc2luZyBpdCB0byBGcmFtZVZpZXcuCisKKyAgICAgICAgKiBwbHVnaW5z L3F0L1BsdWdpblZpZXdRdC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpQbHVnaW5WaWV3Ojp1cGRh dGVQbHVnaW5XaWRnZXQpOgorCiAyMDExLTAyLTA2ICBBbmRyZWFzIEtsaW5nICA8a2xpbmdAd2Vi a2l0Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBEaXJrIFNjaHVsemUuCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9wbHVnaW5zL3F0L1BsdWdpblZpZXdRdC5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9wbHVnaW5zL3F0L1BsdWdpblZpZXdRdC5jcHAKaW5kZXggODIxY2NiN2M3MjRlOTM3YWM2 Y2U2NTUxZjI5YTUxY2YyYzkyOWJiOS4uNDUxOWI5NTc0MWU0ZDhjMjhlMzdiYjUxZGFlNGE3NjJj YjQ0OTg0MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGx1Z2lucy9xdC9QbHVnaW5WaWV3 UXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsdWdpbnMvcXQvUGx1Z2luVmlld1F0LmNwcApA QCAtMTcxLDcgKzE3MSwxMCBAQCB2b2lkIFBsdWdpblZpZXc6OnVwZGF0ZVBsdWdpbldpZGdldCgp CiAgICAgaWYgKCFtX3BsYXRmb3JtTGF5ZXIpIHsKICAgICAgICAgLy8gTWFrZSBzdXJlIHdlIGdl dCByZXBhaW50ZWQgYWZ0ZXJ3YXJkcy4gVGhpcyBpcyBuZWNlc3NhcnkgZm9yIGRvd253YXJkCiAg ICAgICAgIC8vIHNjcm9sbGluZyB0byBtb3ZlIHRoZSBwbHVnaW4gd2lkZ2V0IHByb3Blcmx5Lgot ICAgICAgICBpbnZhbGlkYXRlKCk7CisgICAgICAgIC8vIE5vdGUgdGhhdCB3ZSBkb24ndCBpbnZh bGlkYXRlIHRoZSBmcmFtZVJlY3QoKSBoZXJlLiBUaGlzIGlzIGJlY2F1c2UgUVdlYkZyYW1lOjpy ZW5kZXJSZWxhdGl2ZUNvb3JkcygpCisgICAgICAgIC8vIGltaXRhdGVzIFNjcm9sbFZpZXcgYW5k IGFkZHMgdGhlIHNjcm9sbCBvZmZzZXQgYmFjayBvbiB0byB0aGUgcmVjdCB3ZSBkYW1hZ2UgaGVy ZSAobWFraW5nIHRoZSBjby1vcmRpbmF0ZXMgYWJzb2x1dGUKKyAgICAgICAgLy8gdG8gdGhlIGZy YW1lIGFnYWluKSBiZWZvcmUgcGFzc2luZyBpdCB0byBGcmFtZVZpZXcuCisgICAgICAgIGZyYW1l Vmlldy0+aW52YWxpZGF0ZVJlY3QobV93aW5kb3dSZWN0KTsKICAgICB9CiB9CiAK