Bug 93965 – [CSS Regions] Region.regionOverset and visual overflow are incorrectly computed when content in the named flow is relatively positioned

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Mihai Balan]

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'

[Mihai Balan]

Created attachment 158323 [details]
Ref test highlighting the problem

[Arpita Bahuguna]

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'.

[Arpita Bahuguna]

Created attachment 158814 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Arpita Bahuguna]

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.

[Johannes Wilm]

*** Bug 92637 has been marked as a duplicate of this bug. ***

[Mihnea Ovidenie]

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.

[Alexandru Chiculita]

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).

[Mihnea Ovidenie]

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.

[Mihai Balan]

Bug doesn't reproduce anymore. Adding the attached ref-tests to prevent regressions.

[Mihai Balan]

OK, actually the second part of the bug (the one with position: relative and overflow) *DOES* reproduce.

Waiting for fix.

[Mihai Maerean]

The bug regarding the case when the content as a CSS Transform (rotation) will be handled as bug 110198.

[Mihai Maerean]

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.

[Arpita Bahuguna]

(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.

[Arpita Bahuguna]

Created attachment 191477 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Early Warning System Bot]

Comment on attachment 191477 [details]
Patch

Attachment 191477 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17009105

[Early Warning System Bot]

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

[Arpita Bahuguna]

Created attachment 191481 [details]
Patch

[WebKit Review Bot]

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

[Arpita Bahuguna]

Created attachment 191693 [details]
Patch

[Mihnea Ovidenie]

(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.

[Dave Hyatt]

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.

[Dave Hyatt]

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.

[Andrei Bucur]

This is finally fixed :). Closing!