Bug 124887 - [CSS Regions] Fix painting when the composited region has overflow:hidden
Summary: [CSS Regions] Fix painting when the composited region has overflow:hidden
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Maerean
URL:
Keywords: AdobeTracked
Depends on:
Blocks: 84900
  Show dependency treegraph
 
Reported: 2013-11-26 06:16 PST by Mihai Maerean
Modified: 2014-01-16 22:17 PST (History)
10 users (show)

See Also:


Attachments
layout test (949 bytes, text/html)
2013-11-26 06:39 PST, Mihai Maerean
no flags Details
layout test expected result (937 bytes, text/html)
2013-11-26 06:40 PST, Mihai Maerean
no flags Details
patch (11.58 KB, patch)
2013-12-04 07:32 PST, Mihai Maerean
achicu: review-
achicu: commit-queue-
Details | Formatted Diff | Diff
patch. incorporates the feedback. (13.51 KB, patch)
2013-12-05 08:17 PST, Mihai Maerean
no flags Details | Formatted Diff | Diff
patch. incorporates the feedback. (13.96 KB, patch)
2014-01-10 09:08 PST, Mihai Maerean
no flags Details | Formatted Diff | Diff
patch. incorporates the feedback. (13.89 KB, patch)
2014-01-10 09:21 PST, Mihai Maerean
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.44 MB, application/zip)
2014-01-10 14:18 PST, Build Bot
no flags Details
patch. incorporates the feedback. (13.71 KB, patch)
2014-01-14 05:15 PST, Mihai Maerean
achicu: review+
achicu: commit-queue-
Details | Formatted Diff | Diff
patch for landing (15.11 KB, patch)
2014-01-15 22:19 PST, Mihai Maerean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Maerean 2013-11-26 06:16:34 PST
The left and top part of the content flowed in the (composited and with overflow:hidden) region is missing.
Comment 1 Mihai Maerean 2013-11-26 06:39:40 PST
Created attachment 217875 [details]
layout test
Comment 2 Mihai Maerean 2013-11-26 06:40:24 PST
Created attachment 217876 [details]
layout test expected result
Comment 3 Mihai Maerean 2013-12-04 07:32:44 PST
Created attachment 218410 [details]
patch
Comment 4 Alexandru Chiculita 2013-12-04 10:34:29 PST
Comment on attachment 218410 [details]
patch

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

> LayoutTests/compositing/regions/paint-inside-composited-region-overflow-hidden-versus-div-expected.html:30
> +			<div class="content">a b c 1. a b c 2. a b c 3. a b c 4. a b c 5. a b c 6. a b c 7. a b c 8. a b c 9. a b c A. a b c B. a b c C. a b c D. a b c E. a b c F. a b c 0. a b c 1. a b c 2. a b c 3. </div>

Is there any reason for using the "a b c" sequences? I think it makes it hard to follow. Just use plain text.

> Source/WebCore/rendering/RenderLayer.cpp:6768
> +        regionClipRect = backgroundClipRect(clipRectsContext); // Region coordinates.

I think we need to be more explicit about explaining the difference between region / fragment / fragment container.

> Source/WebCore/rendering/RenderLayer.cpp:6778
>          regionClipRect = regionContentBox;

I think that this whole function needs some rework. For example, this extra step will overwrite this line from above "regionClipRect = dirtyRect".

> Source/WebCore/rendering/RenderLayer.cpp:6782
> +        // When the layer of the region is composited, the region receives a GraphicsLayer of its own
> +        // so the clipping coordinates (caused by overflow:hidden) must be relative to the
> +        // GraphicsLayer coordinates in which the region gets painted.
> +        if (!backing()) {

The comment is confusing, especially because you have a !backing() just after it. I think the term "region" is now confusing as you have a "fragmentContainer" and a "fragment". Which one are you refering to as a "region".

Also, there is a phase where all the layers are flatten even though they have a backing. It's used to draw the "drag" image when you drag a portion of the page. That's why I don't think it's a good idea to check just for !backing().

> Source/WebCore/rendering/RenderLayer.cpp:6785
> +            regionClipRect.moveBy(regionOffsetFromRoot); // Screen coordinates.

I don't understand how this could be "screen" coordinates.
Comment 5 Mihai Maerean 2013-12-05 07:59:26 PST
Comment on attachment 218410 [details]
patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:6782
>> +        if (!backing()) {
> Also, there is a phase where all the layers are flatten even though they have a backing. It's used to draw the "drag" image when you drag a portion of the page. That's why I don't think it's a good idea to check just for !backing().

What else to check for?
Shouldn't this issue (about dragging of a page portion) be tracked as a different bug?
Comment 6 Mihai Maerean 2013-12-05 08:17:21 PST
Created attachment 218516 [details]
patch. incorporates the feedback.
Comment 7 Mihai Maerean 2014-01-10 09:08:07 PST
Created attachment 220848 [details]
patch. incorporates the feedback.

The dragged selected text that is composited in a region is not being rendered in Nightly at the moment. I have added https://bugs.webkit.org/show_bug.cgi?id=126755 to track this bug.
Comment 8 Mihai Maerean 2014-01-10 09:21:56 PST
Created attachment 220849 [details]
patch. incorporates the feedback.

The dragged selected text that is composited in a region is not being rendered in Nightly at the moment. I have added https://bugs.webkit.org/show_bug.cgi?id=126755 to track this bug.
Comment 9 Build Bot 2014-01-10 14:18:50 PST
Comment on attachment 220849 [details]
patch. incorporates the feedback.

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

New failing tests:
fast/regions/overflow-first-and-last-regions-in-container-hidden.html
fast/regions/overflow-scrollable-1.html
fast/regions/overflow-scrollable-3.html
fast/regions/shape-inside/shape-inside-on-first-region-inline-content.html
fast/regions/overflow-scrollable-fit.html
fast/regions/shape-inside/shape-inside-on-first-region-block-content.html
fast/regions/overflow-scrollable-varying-width-1.html
Comment 10 Build Bot 2014-01-10 14:18:53 PST
Created attachment 220887 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 11 Mihai Maerean 2014-01-14 05:15:10 PST
Created attachment 221145 [details]
patch. incorporates the feedback.
Comment 12 Mihai Maerean 2014-01-15 08:55:30 PST
please review
Comment 13 Alexandru Chiculita 2014-01-15 13:53:20 PST
Comment on attachment 221145 [details]
patch. incorporates the feedback.

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

> LayoutTests/compositing/regions/paint-inside-composited-region-overflow-hidden-versus-div-expected.html:4
> +		<title>Expected Result - Bug #124887: [CSS Regions] Fix painting when the composited region has overflow:hidden</title>

nit: Use 4 sapces instead of tabs.

r=me
Comment 14 Mihai Maerean 2014-01-15 22:19:17 PST
Created attachment 221332 [details]
patch for landing
Comment 15 WebKit Commit Bot 2014-01-15 22:55:54 PST
Comment on attachment 221332 [details]
patch for landing

Clearing flags on attachment: 221332

Committed r162115: <http://trac.webkit.org/changeset/162115>