Bug 30366 – if the QWebPage is rendered by a QGraphicsWidget child of another widget with ItemClipsChildrenToShape the scrollbar is not clipped

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Marco Martin]

at line 150 of ScrollbarThemeQt.cpp there is the followin instruction: 
p.painter->setClipRect(opt->rect.intersected(damageRect));

this causes the following problem: 
a QGraphicsWidget renders a QWebPage 
is the children of another qgw wit the flag ItemClipsChildrenToShape set. 
the scrollbars will be painted not clipped to the parent shape because of the cliprect reset.

this patch intersect the rect, with the previous one, making the scrollbar to be correctly clipped

[Marco Martin]

Created attachment 41185 [details]
patch that corrects the bug

[Simon Hausmann]

(In reply to comment #1)
> Created an attachment (id=41185) [details]
> patch that corrects the bug

Good catch! Marco, your patch looks correct, but it needs to be adapted to the latest code changes in the trunk - the code is now in ScrollbarThemeQt.cpp - and your patch needs a ChangeLog entry. Please see http://webkit.org/coding/contributing.html

[Marco Martin]

Created attachment 44478 [details]
updated patch

updated the patch to the new filename and added a ChangeLog entry

[Kenneth Rohde Christiansen]

Is this supposed to be up for review? If so, please set the review flag to r?

[Marco Martin]

Comment on attachment 44478 [details]
updated patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 51868)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Marco Martin  <[e-mail]>
> +
> +        Review pending.
> +
> +        In the Scrollbar painting of the Qt theme integration,
> +        intersects any previous clip rects with the one needed to paint the scrollbar.
> +        This fixes the painting in QGraphicsview, when the parent of the widget that 
> +        paints the QWebPage has the QGraphicsItem::ItemClipsChildrenToShape set and
> +        a piece of the scrollbar should be cipped away
> +        https://bugs.webkit.org/show_bug.cgi?id=30366
> +
> +        No new tests.
> +
> +        * platform/qt/ScrollbarThemeQt.cpp:
> +        (WebCore::ScrollbarThemeQt::paint):
> +
>  2009-12-08  John Gregg  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> Index: WebCore/platform/qt/ScrollbarThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/ScrollbarThemeQt.cpp	(revision 51865)
> +++ WebCore/platform/qt/ScrollbarThemeQt.cpp	(working copy)
> @@ -147,7 +147,7 @@
>      p.painter->save();
>      QStyleOptionSlider* opt = styleOptionSlider(scrollbar, p.widget);
>  
> -    p.painter->setClipRect(opt->rect.intersected(damageRect));
> +    p.painter->setClipRect(opt->rect.intersected(damageRect), Qt::IntersectClip);
>  
>  #ifdef Q_WS_MAC
>      p.drawComplexControl(QStyle::CC_ScrollBar, *opt);

[Marco Martin]

@Kenneth:
yes, the patch should be final i think (apart the revieved by in the changelog :))

[Kenneth Rohde Christiansen]

Comment on attachment 44478 [details]
updated patch


> [phone]  Marco Martin  <[e-mail]>
> +
> +        Review pending.
> +

You need to leave the review line untouched, unless you want to land this manually, but as I believe you do not have commit rights, someone else could add it to the commit-queue.

Can you reupload?

[Eric Seidel (no email)]

Comment on attachment 44478 [details]
updated patch

Marco is not a reviewer, clearly r+.  I assume you meant to mark this as r? instead?

[Eric Seidel (no email)]

I meant "clearing" instead of "clearly", sorry.

[Marco Martin]

Comment on attachment 44478 [details]
updated patch

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 51868)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Marco Martin  <[e-mail]>
> +
> +        Review pending.
> +
> +        In the Scrollbar painting of the Qt theme integration,
> +        intersects any previous clip rects with the one needed to paint the scrollbar.
> +        This fixes the painting in QGraphicsview, when the parent of the widget that 
> +        paints the QWebPage has the QGraphicsItem::ItemClipsChildrenToShape set and
> +        a piece of the scrollbar should be cipped away
> +        https://bugs.webkit.org/show_bug.cgi?id=30366
> +
> +        No new tests.
> +
> +        * platform/qt/ScrollbarThemeQt.cpp:
> +        (WebCore::ScrollbarThemeQt::paint):
> +
>  2009-12-08  John Gregg  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> Index: WebCore/platform/qt/ScrollbarThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/ScrollbarThemeQt.cpp	(revision 51865)
> +++ WebCore/platform/qt/ScrollbarThemeQt.cpp	(working copy)
> @@ -147,7 +147,7 @@
>      p.painter->save();
>      QStyleOptionSlider* opt = styleOptionSlider(scrollbar, p.widget);
>  
> -    p.painter->setClipRect(opt->rect.intersected(damageRect));
> +    p.painter->setClipRect(opt->rect.intersected(damageRect), Qt::IntersectClip);
>  
>  #ifdef Q_WS_MAC
>      p.drawComplexControl(QStyle::CC_ScrollBar, *opt);

[Kenneth Rohde Christiansen]

Comment on attachment 44478 [details]
updated patch


> +        Review pending.

Don't change the line made by prepare-Changelog

> +        No new tests.

Would be good with a test, at least a manual one (goes in WebCore/manual-tests).

[Eric Seidel (no email)]

The Review Patch link is probably not the link you want.  "Details" is more useful.  Review Patch is kinda a lame hack that just adds the full diff as a comment and shows you the "Formatted Diff" view in an iframe.

[Marco Martin]

Created attachment 44821 [details]
second patch update

I've done the change in the changelog, hope is correct now.

[Marco Martin]

Comment on attachment 44821 [details]
second patch update

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 52117)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,19 @@
> [phone]  Marco Martin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        In the Scrollbar painting of the Qt theme integration,
> +        intersects any previous clip rects with the one needed to paint the scrollbar.
> +        This fixes the painting in QGraphicsview, when the parent of the widget that 
> +        paints the QWebPage has the QGraphicsItem::ItemClipsChildrenToShape set and
> +        a piece of the scrollbar should be cipped away
> +        https://bugs.webkit.org/show_bug.cgi?id=30366
> +
> +        No new tests.
> +
> +        * platform/qt/ScrollbarThemeQt.cpp:
> +        (WebCore::ScrollbarThemeQt::paint):
> +
>  2009-12-14  Alexey Proskuryakov  <[e-mail]>
>  
>          Reviewed by Dave Hyatt.
> Index: WebCore/platform/qt/ScrollbarThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/ScrollbarThemeQt.cpp	(revision 51865)
> +++ WebCore/platform/qt/ScrollbarThemeQt.cpp	(working copy)
> @@ -147,7 +147,7 @@
>      p.painter->save();
>      QStyleOptionSlider* opt = styleOptionSlider(scrollbar, p.widget);
>  
> -    p.painter->setClipRect(opt->rect.intersected(damageRect));
> +    p.painter->setClipRect(opt->rect.intersected(damageRect), Qt::IntersectClip);
>  
>  #ifdef Q_WS_MAC
>      p.drawComplexControl(QStyle::CC_ScrollBar, *opt);

[Kenneth Rohde Christiansen]

Comment on attachment 44821 [details]
second patch update

LGTM, but please look into adding a test. Thanks for the work Martin

[Marco Martin]

in manual-tests i see only html files, since this one is not a rendering problem it would be more useful a short program...
i'll look into that.
how they should be done?

[Kenneth Rohde Christiansen]

Ah ofcourse, sorry. Looking at too many patches :-)

I don't know the best way to actually add a test for this that can be tested automatically. The closest would be a Qt autotest, which we use for testing our APIs.

[WebKit Commit Bot]

Comment on attachment 44821 [details]
second patch update

Clearing flags on attachment: 44821

Committed r52122: <http://trac.webkit.org/changeset/52122>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.