Bug 93965 - [CSS Regions] Region.regionOverset and visual overflow are incorrectly computed when content in the named flow is relatively positioned
Summary: [CSS Regions] Region.regionOverset and visual overflow are incorrectly comput...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Balan
URL:
Keywords: AdobeTracked
: 92637 (view as bug list)
Depends on:
Blocks: 57312 110198
  Show dependency treegraph
 
Reported: 2012-08-14 07:18 PDT by Mihai Balan
Modified: 2014-03-19 06:10 PDT (History)
17 users (show)

See Also:


Attachments
Ref test highlighting the problem (1.44 KB, application/x-zip-compressed)
2012-08-14 07:26 PDT, Mihai Balan
no flags Details
Patch (6.79 KB, patch)
2012-08-16 07:28 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (503.91 KB, application/zip)
2012-08-16 08:08 PDT, WebKit Review Bot
no flags Details
Patch (6.01 KB, text/plain)
2013-03-05 05:47 PST, Arpita Bahuguna
no flags Details
Patch (6.06 KB, patch)
2013-03-05 06:06 PST, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (6.91 KB, patch)
2013-03-06 02:17 PST, Arpita Bahuguna
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Balan 2012-08-14 07:18:44 PDT
If the content in a named flow is relatively positioned (position: relative), then both the visual overflow and Element.regionOverset are incorrectly computed, as follows (this use-case assumes a region that will cause the content to overflow):
  * the part of the named flow that overflows the region gets clipped (as if the region would have «region-overflow: auto; overflow: hidden» set) - while it should actually overflow
  * region.regionOverset returns 'fit' - while it should return 'overset'
Comment 1 Mihai Balan 2012-08-14 07:26:37 PDT
Created attachment 158323 [details]
Ref test highlighting the problem
Comment 2 Arpita Bahuguna 2012-08-16 05:55:13 PDT
The second part of the issue stated here i.e.:
* region.regionOverset returns 'fit' - while it should return 'overset'
seems to have now been fixed.
Verified on the latest nightly (windows) - r125742
The value for regionOverset now returned is 'overset' instead of 'fit'.
Comment 3 Arpita Bahuguna 2012-08-16 07:28:56 PDT
Created attachment 158814 [details]
Patch
Comment 4 WebKit Review Bot 2012-08-16 08:08:34 PDT
Comment on attachment 158814 [details]
Patch

Attachment 158814 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13515418

New failing tests:
fast/repaint/region-painting-via-layout.html
Comment 5 WebKit Review Bot 2012-08-16 08:08:38 PDT
Created attachment 158823 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 6 Arpita Bahuguna 2012-08-17 05:26:34 PDT
The failing test case: fast/repaint/region-painting-via-layout.html, even without this fix, results in the same issue if position: relative is not specified.

There seems to be a problem with repaint when the content overflows, and it happens even without the fix.
For example, if I introduce a lag before the repaint, this issue doesn't occur i.e. the repaint is proper (even for the overflowing 20px).

Can change the test case to have no overflow in region2 but that would also mean changing the expected results.

Would really appreciate any help in this regard as am unsure on how to proceed with this issue.
Comment 7 Johannes Wilm 2012-08-20 04:08:41 PDT
*** Bug 92637 has been marked as a duplicate of this bug. ***
Comment 8 Mihnea Ovidenie 2012-10-12 01:21:16 PDT
You do not need to have content be positioned to achieve the same result in the last region, it is enough to have the block that is collected into a flow thread have its own layer. For instance:

<div id="text" style="-webkit-flow-into: flow; opacity: 0.9">
	Text that overflows the region
</div>
<div class="region" style="-webkit-flow-from: flow; width: 150px; height: 100px;"></div>

will clip the text at the region box, similar to uploaded test.
Comment 9 Alexandru Chiculita 2012-10-12 11:42:52 PDT
Comment on attachment 158814 [details]
Patch

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

Thanks for your patch! This is an informal review on the approach, so feel free to ignore my comments :)

> Source/WebCore/rendering/RenderBox.cpp:3614
> +    if ((child->hasSelfPaintingLayer() && !inRenderFlowThread()) || hasOverflowClip())

I would better change RenderRegion::overflowRectForFlowThreadPortion method to use RenderLayer::calculateLayerBounds instead. That one should know everything about the descendant layers and should compute the "right" overflow.

I'm pretty sure that having your fix here, doesn't fix the overflow when you have a -webkit-transform: translate(1000px, 0px).
Moreover, I suppose the visual overflow is not correctly calculated here when you have a transform that changes the coordinate system (ie. rotate).
Comment 10 Mihnea Ovidenie 2012-10-12 12:48:24 PDT
Comment on attachment 158814 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +

This comment is not correct and i would prefer to be changed. From the changelog comment it should be clear about what visual overflow you are talking, for instance the visual overflow of the elements that are collected inside a flow thread and have an attached layer. Also, the bug is not about positioned regions.

> Source/WebCore/ChangeLog:19
> +     

Again, not the region is having the position: relative, but rather the content that is fragmented by the region.

> Source/WebCore/ChangeLog:23
> +

The child having itself a layer reproduces the problem, it does not need to be positioned.
Comment 11 Mihai Balan 2013-02-04 06:59:08 PST
Bug doesn't reproduce anymore. Adding the attached ref-tests to prevent regressions.
Comment 12 Mihai Balan 2013-02-04 07:06:11 PST
OK, actually the second part of the bug (the one with position: relative and overflow) *DOES* reproduce.

Waiting for fix.
Comment 13 Mihai Maerean 2013-02-19 03:12:58 PST
The bug regarding the case when the content as a CSS Transform (rotation) will be handled as bug 110198.
Comment 14 Mihai Maerean 2013-02-19 05:18:14 PST
Comment on attachment 158814 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:3614
>> +    if ((child->hasSelfPaintingLayer() && !inRenderFlowThread()) || hasOverflowClip())
> 
> I would better change RenderRegion::overflowRectForFlowThreadPortion method to use RenderLayer::calculateLayerBounds instead. That one should know everything about the descendant layers and should compute the "right" overflow.
> 
> I'm pretty sure that having your fix here, doesn't fix the overflow when you have a -webkit-transform: translate(1000px, 0px).
> Moreover, I suppose the visual overflow is not correctly calculated here when you have a transform that changes the coordinate system (ie. rotate).

1. calling RenderLayer from RenderRegion (a RenderBlock) would use outdated information (the RenderLayer tree is updated after the RenderObject tree).
The change in the this patch fixes having position:relative and also opacity on content.
I would go for pushing this patch in.

2. fixing the transform issue is now being tracked in bug 110198.
Comment 15 Arpita Bahuguna 2013-02-19 06:25:58 PST
(In reply to comment #14)
> (From update of attachment 158814 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158814&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:3614
> >> +    if ((child->hasSelfPaintingLayer() && !inRenderFlowThread()) || hasOverflowClip())
> > 
> > I would better change RenderRegion::overflowRectForFlowThreadPortion method to use RenderLayer::calculateLayerBounds instead. That one should know everything about the descendant layers and should compute the "right" overflow.
> > 
> > I'm pretty sure that having your fix here, doesn't fix the overflow when you have a -webkit-transform: translate(1000px, 0px).
> > Moreover, I suppose the visual overflow is not correctly calculated here when you have a transform that changes the coordinate system (ie. rotate).
> 
> 1. calling RenderLayer from RenderRegion (a RenderBlock) would use outdated information (the RenderLayer tree is updated after the RenderObject tree).
> The change in the this patch fixes having position:relative and also opacity on content.
> I would go for pushing this patch in.
> 
> 2. fixing the transform issue is now being tracked in bug 110198.
Thank-you for your comments Mihai. I shall try and give this patch another go.
Comment 16 Arpita Bahuguna 2013-03-05 05:47:56 PST
Created attachment 191477 [details]
Patch
Comment 17 WebKit Review Bot 2013-03-05 05:51:17 PST
Comment on attachment 191477 [details]
Patch

Attachment 191477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17049138
Comment 18 WebKit Review Bot 2013-03-05 05:51:48 PST
Comment on attachment 191477 [details]
Patch

Attachment 191477 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17043101
Comment 19 Early Warning System Bot 2013-03-05 05:53:16 PST
Comment on attachment 191477 [details]
Patch

Attachment 191477 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17009105
Comment 20 Early Warning System Bot 2013-03-05 05:54:18 PST
Comment on attachment 191477 [details]
Patch

Attachment 191477 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17009106
Comment 21 Arpita Bahuguna 2013-03-05 06:06:29 PST
Created attachment 191481 [details]
Patch
Comment 22 WebKit Review Bot 2013-03-05 06:51:12 PST
Comment on attachment 191481 [details]
Patch

Attachment 191481 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17049159

New failing tests:
fast/repaint/region-painting-via-layout.html
Comment 23 Arpita Bahuguna 2013-03-06 02:17:59 PST
Created attachment 191693 [details]
Patch
Comment 24 Mihnea Ovidenie 2013-03-25 08:00:22 PDT
(In reply to comment #23)
> Created an attachment (id=191693) [details]
> Patch

Maybe we should solve https://bugs.webkit.org/show_bug.cgi?id=76991 before this bug.
Comment 25 Dave Hyatt 2013-04-02 09:11:47 PDT
Comment on attachment 191693 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:4076
> -    if (child->hasSelfPaintingLayer() || hasOverflowClip())
> +    RenderFlowThread* flowThread = flowThreadContainingBlock();
> +    if ((child->hasSelfPaintingLayer() && !flowThread) || hasOverflowClip())

This is not correct. You should never ever propagate visual overflow across a self-painting layer boundary.
Comment 26 Dave Hyatt 2013-04-02 09:15:07 PDT
Isn't regionOverset supposed to be about layoutOverflow and not visualOverflow? I'm confused as to why you're messing with visual overflow. Layout overflow does propagate across layers and accounts for transforms and relative positioning. Visual overflow is something different. It's just about shadows and outlines, etc.
Comment 27 Andrei Bucur 2014-03-19 06:10:16 PDT
This is finally fixed :). Closing!