Bug 124919 – The overflow border of a relatively positioned element inside a region is not painted

• Component: CSS
• Status: RESOLVED FIXED

[Radu Stavila]

Created attachment 217930 [details]
Test-case

The overflow of a relatively positioned element inside a region is not painted. See attached test-case. 

Expected: the green rectangle should be fully visible
Actual: the lower and right parts of the green part are clipped by the amount of it's top and left relative positioning (most likely it's relative positioning is not taken into consideration when computing its overflow).

[Radu Stavila]

Created attachment 217999 [details]
Patch

[Andrei Bucur]

Comment on attachment 217999 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:1287
> +    if (box.isRelPositioned() && box.layer()) {

isRelPos == has layer. The box.layer() if should be an ASSERT.

> Source/WebCore/rendering/RenderFlowThread.cpp:1290
> +    } else {

I suppose this is an else because the rel pos elements have self-painting layers and don't propagate visual overflow, right? Maybe a comment could be helpful.

[Radu Stavila]

Comment on attachment 217999 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlowThread.cpp:1290
>> +    } else {
> 
> I suppose this is an else because the rel pos elements have self-painting layers and don't propagate visual overflow, right? Maybe a comment could be helpful.

For non-relative positioned elements, the clipping rectangle of their box declarations is obtained by iterating the containing block chain. Relative positioned elements on the other hand already have this information in the layer's location, including their relative position offset.

[Mihnea Ovidenie]

Comment on attachment 217999 [details]
Patch

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

Thanks for taking a look at this one. I think you need another round to address the comments.

> LayoutTests/fast/regions/relative-borders-overflow.html:2
> +    #container {

What is the role of #container in this test? From what i can tell, if you take it out, you are able to show the problem but i may miss something.

> LayoutTests/fast/regions/relative-borders-overflow.html:13
> +        border: 1px solid red;

I prefer you use a color that is different from red for the region border.

> LayoutTests/fast/regions/relative-borders-overflow.html:31
> +    <p>The test passes if all borders are completely visible and the text <span style="color:red"><b>THE END</b></span> is visible</p>

I find using red for "THE END" confusing, please use a different color.

> Source/WebCore/ChangeLog:6
> +        For relatively positioned elements, the layer's position should be used when determining

As Andrei mentioned before, you are using the layer's position because a relatively positioned element is a self painting layer that don't propagate the visual overflow. This should go into the changelog description, i find it important.

> Source/WebCore/rendering/RenderFlowThread.cpp:1286
> +    // FIXME: This may not work properly with different writing modes.

Please file a follow up bug for this if we do not already have one.

>> Source/WebCore/rendering/RenderFlowThread.cpp:1287
>> +    if (box.isRelPositioned() && box.layer()) {
> 
> isRelPos == has layer. The box.layer() if should be an ASSERT.

I agree. There is no need to test for layer() and i don't think you need an assert here for box.layer().

Also because for relatively positioned element you are skipping the loop below, what happens when you have say an absolutely positioned parent for the relative positioned element? Is the box decorations clipping rect computed properly?

<div style="-webkit-flow-into: flow2; position: absolute; top: 50px; left: 50px; border: 5px solid magenta">
    <!-- article from your test without the flow into declaration -->
    <div id="article">
    </div>
</div>

>>> Source/WebCore/rendering/RenderFlowThread.cpp:1290
>>> +    } else {
>> 
>> I suppose this is an else because the rel pos elements have self-painting layers and don't propagate visual overflow, right? Maybe a comment could be helpful.
> 
> For non-relative positioned elements, the clipping rectangle of their box declarations is obtained by iterating the containing block chain. Relative positioned elements on the other hand already have this information in the layer's location, including their relative position offset.

The important comment from Andrei's review should go here or in the changelog (i prefer the changelog).

[Radu Stavila]

Created attachment 218293 [details]
Updated patch

[Mihnea Ovidenie]

Comment on attachment 218293 [details]
Updated patch

r=me but you need to regenerate the changelogs before landing to take the new added test into account.

[Radu Stavila]

Created attachment 218299 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 218299 [details]
Patch for landing

Clearing flags on attachment: 218299

Committed r160014: <http://trac.webkit.org/changeset/160014>