Bug 124919 - The overflow border of a relatively positioned element inside a region is not painted
Summary: The overflow border of a relatively positioned element inside a region is not...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords:
Depends on:
Blocks: 57312 116295
  Show dependency treegraph
 
Reported: 2013-11-27 02:09 PST by Radu Stavila
Modified: 2013-12-03 11:39 PST (History)
5 users (show)

See Also:


Attachments
Test-case (941 bytes, text/html)
2013-11-27 02:09 PST, Radu Stavila
no flags Details
Patch (6.78 KB, patch)
2013-11-28 06:40 PST, Radu Stavila
mihnea: review-
Details | Formatted Diff | Diff
Updated patch (9.32 KB, patch)
2013-12-03 06:58 PST, Radu Stavila
mihnea: review+
Details | Formatted Diff | Diff
Patch for landing (9.48 KB, patch)
2013-12-03 09:40 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 2013-11-27 02:09:25 PST
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).
Comment 1 Radu Stavila 2013-11-28 06:40:51 PST
Created attachment 217999 [details]
Patch
Comment 2 Andrei Bucur 2013-11-28 07:56:58 PST
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.
Comment 3 Radu Stavila 2013-11-28 08:00:21 PST
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.
Comment 4 Mihnea Ovidenie 2013-12-03 02:03:23 PST
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).
Comment 5 Radu Stavila 2013-12-03 06:58:59 PST
Created attachment 218293 [details]
Updated patch
Comment 6 Mihnea Ovidenie 2013-12-03 07:59:54 PST
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.
Comment 7 Radu Stavila 2013-12-03 09:40:53 PST
Created attachment 218299 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2013-12-03 10:26:06 PST
Comment on attachment 218299 [details]
Patch for landing

Clearing flags on attachment: 218299

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