[webkit-reviews] review denied: [Bug 124919] The overflow border of a relatively positioned element inside a region is not painted : [Attachment 217999] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 3 02:05:01 PST 2013


Mihnea Ovidenie <mihnea at adobe.com> has denied Radu Stavila
<stavila at adobe.com>'s request for review:
Bug 124919: The overflow border of a relatively positioned element inside a
region is not painted
https://bugs.webkit.org/show_bug.cgi?id=124919

Attachment 217999: Patch
https://bugs.webkit.org/attachment.cgi?id=217999&action=review

------- Additional Comments from Mihnea Ovidenie <mihnea at adobe.com>
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).


More information about the webkit-reviews mailing list