Bug 116859 - REGRESSION (r143070): Overflow:scroll content does not get clipped properly when the parent box has CSS3 filter on.
Summary: REGRESSION (r143070): Overflow:scroll content does not get clipped properly w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar, Regression
Depends on: 117336
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-28 03:47 PDT by zalan
Modified: 2013-06-07 04:40 PDT (History)
6 users (show)

See Also:


Attachments
test case (935 bytes, text/html)
2013-05-28 03:48 PDT, zalan
no flags Details
Patch (6.33 KB, patch)
2013-05-29 12:42 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (542.12 KB, application/zip)
2013-05-29 19:26 PDT, Build Bot
no flags Details
Patch (9.06 KB, patch)
2013-06-03 04:38 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2013-05-28 03:47:48 PDT
the following code produces a broken content where the overflown content does not get clipped at the edge of the box.
    <style>
        .box {
            overflow: scroll;
            height: 150px;
            width: 150px;
            border: solid 1px red;
            white-space: nowrap;
        }

        .shadow {
            -webkit-filter: drop-shadow(100px 100px 1px blue);
        }
    </style>

<div class="shadow"> 
  <div class="before box">long text here........</div>
</div>
Comment 1 zalan 2013-05-28 03:48:27 PDT
Created attachment 203038 [details]
test case
Comment 2 zalan 2013-05-28 03:49:09 PDT
<rdar://problem/13845888>
Comment 3 zalan 2013-05-29 12:42:15 PDT
Created attachment 203260 [details]
Patch
Comment 4 Build Bot 2013-05-29 19:26:45 PDT
Comment on attachment 203260 [details]
Patch

Attachment 203260 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/742271

New failing tests:
fast/dom/location-new-window-no-crash.html
Comment 5 Build Bot 2013-05-29 19:26:47 PDT
Created attachment 203300 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 6 zalan 2013-05-30 03:27:08 PDT
Comment on attachment 203260 [details]
Patch

EWS:Unable to pass tests without patch (tree is red?) 

looks unrelated.
Comment 7 Simon Fraser (smfr) 2013-05-31 14:54:37 PDT
Comment on attachment 203260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203260&action=review

> Source/WebCore/rendering/RenderLayer.cpp:3921
> +        // fragment should paint. If the parent's filter dictates full repaint to ensure proper filter effect,
> +        // use the overflow clip as dirty rect, instead of no clipping. It maintains proper clipping for overflow::scroll.
> +        LayoutRect paintDirtyRect = localPaintingInfo.paintDirtyRect;
> +        if (!paintingInfo.clipToDirtyRect && renderer()->hasOverflowClip()) {
> +            // We can turn clipping back by requesting full repaint for the overflow area.
> +            localPaintingInfo.clipToDirtyRect = true;
> +            paintDirtyRect = selfClipRect();
> +        }

Does this do the correct thing if the overflow is on a descendant of the filtered element? Should that have its own testcase?

> LayoutTests/css3/filters/filter-repaint-shadow-on-parent-and-overflow-scroll-expected.html:9
> +            white-space: nowrap;

Is this relevant?

> LayoutTests/css3/filters/filter-repaint-shadow-on-parent-and-overflow-scroll-expected.html:11
> +            -webkit-filter: blur(0px);

I would change this to a non-zero blur, because I could imagine us optimizing hasFilterThatMovesPixels() to return false for zero-radius blurs.
Comment 8 zalan 2013-06-03 04:38:38 PDT
Created attachment 203574 [details]
Patch
Comment 9 zalan 2013-06-03 04:42:59 PDT
(In reply to comment #7)
> (From update of attachment 203260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203260&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:3921
> > +        // fragment should paint. If the parent's filter dictates full repaint to ensure proper filter effect,
> > +        // use the overflow clip as dirty rect, instead of no clipping. It maintains proper clipping for overflow::scroll.
> > +        LayoutRect paintDirtyRect = localPaintingInfo.paintDirtyRect;
> > +        if (!paintingInfo.clipToDirtyRect && renderer()->hasOverflowClip()) {
> > +            // We can turn clipping back by requesting full repaint for the overflow area.
> > +            localPaintingInfo.clipToDirtyRect = true;
> > +            paintDirtyRect = selfClipRect();
> > +        }
> 
> Does this do the correct thing if the overflow is on a descendant of the filtered element? Should that have its own testcase?
The original bug is actually about the overflow:scroll on the descendant of the filtered element. So the patch covers that case (too). The reftest was a little bit misleading and I changed it to cover both cases.

> 
> > LayoutTests/css3/filters/filter-repaint-shadow-on-parent-and-overflow-scroll-expected.html:9
> > +            white-space: nowrap;
> 
> Is this relevant?
No it is not (was just to force the scrollbar), I removed it.

> 
> > LayoutTests/css3/filters/filter-repaint-shadow-on-parent-and-overflow-scroll-expected.html:11
> > +            -webkit-filter: blur(0px);
> 
> I would change this to a non-zero blur, because I could imagine us optimizing hasFilterThatMovesPixels() to return false for zero-radius blurs.
Good point. I changed that too. Thanks!
Comment 10 WebKit Commit Bot 2013-06-03 06:37:32 PDT
Comment on attachment 203574 [details]
Patch

Clearing flags on attachment: 203574

Committed r151110: <http://trac.webkit.org/changeset/151110>
Comment 11 WebKit Commit Bot 2013-06-03 06:37:35 PDT
All reviewed patches have been landed.  Closing bug.