[webkit-reviews] review canceled: [Bug 91665] When a block element is made inline positioned and has static left and right, it does not follow inline formatting context : [Attachment 157754] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 91665: When a block element is made inline positioned and has static left
and right,  it does not follow inline formatting context
https://bugs.webkit.org/show_bug.cgi?id=91665

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157754&action=review


Sorry for the long delay.

> Source/WebCore/rendering/RenderObject.cpp:1644
> +		   RenderObject* afterChild = this->previousSibling() ?
this->previousSibling() : parent()->lastChild();

The rule to pick an anonymous block seems completely arbitrary. This is super
dangerous to reuse an existing anonymous as it's easy to leave a dangling
pointer then. I feared having |afterChild| == |this| but I think this would not
happen in practice.

What are you trying to achieve here by adding this logic?

> Source/WebCore/rendering/RenderObject.cpp:1805
> +    bool didFloatOrPositionedChildToStatic =
child->isFloatingOrOutOfFlowPositioned()
> +					   && (!newStyle->isFloating() &&
!newStyle->isOutOfFlowPositioned());
> +
> +    bool staticBlockToInlinePositioned = !child->isOutOfFlowPositioned()
> +					       &&
newStyle->isOutOfFlowPositioned()

If you look at those 2 variables, they seem pretty close and I would bet we
could simplify the logic to:

bool wasFloatingOrOutOfFlowPositioned =
child->isFloatingOrOutOfFlowPositioned();
bool isFloatingOrOutOfFlowPositioned = newStyle->isFloating() ||
newStyle->isOutOfFlowPositioned();

return wasFloatingOrOutOfFlowPositioned != isFloatingOrOutOfFlowPositioned &&
parent() && (parent()->isBlockFlow() || parent()->isRenderInline());

(note that it doesn't check the display but I think we are safer with a check
that is a little broader)

If you could test the float case and confirm if it is also impacted (which is
pretty likely) then we could make this simplification.

> LayoutTests/fast/dynamic/absolute-positioned-to-inline-static-block.html:73
> +	  
shouldBeTrue('currentTestNode[\"'+testElems[i].tagName+'\"].getBoundingClientRe
ct().left == \
>
+previousSibling[\"'+testElems[i].previousSibling.tagName+'\"].getBoundingClien
tRect().left','0');
> +	  
shouldBeTrue('currentTestNode[\"'+testElems[i].tagName+'\"].getBoundingClientRe
ct().top == \

missing space before / after '+'.