[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 153630] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled  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 153630: Patch
https://bugs.webkit.org/attachment.cgi?id=153630&action=review

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


Holding the patch until the mentioned use cases are investigated. More testing
is definitely needed before we land this change.

>>> Source/WebCore/dom/Node.cpp:374
>>> +	 if (!isS1originalDisplayTypeInline && isS2originalDisplayTypeInline &&
s2->isOutOfFlowPositioned())
>> 
>> This whole patch looks totally reasonable to me.  I was not aware of the
"original display inline type" accessor here.  I also am not 100% sure that
this is the correct way to test for what you're going for.
>> 
>> Why do we need to check the original display type here?  Why not just check
isOutofFlowPositioned() != isOutOfFlowPositioned() between teh two styles? 
That would be a broader net, but it seems it would catch thsi, no?
> 
> About the issue:
> We have a container of only block elements(for simplicity)... 
> When all the elements are converted to inline elements, new renderers are
created for each element and are made to follow inline context...
> 
> If all the elements are already inline, except the last one. And we change he
last element's display to inline, then its reattached and added to the inline
flow of elements(here I'm imagining the inline flow to be a list of contiguous
inline elements) so that the last element also be laid out as an inline.
> 
> The function Node::diff() is the function which, based on the old and new
styles decides if a node needs a its renderer reattached(attach a new
renderer). 
> 
> In short when a element's display(the one set by style) is changed from block
to inline its renderer must be reattached.
> 
> In case of positioned elements the display(internal) property is always set
as block... However OriginalDisplay() contains the actual display value set in
style. OriginalDisplay() must also be used by Node::diff() to decide if an
element's renderer reqs to be reattached or not.

I don't think we want to detach whenever 'position' changes as it should only
be at layout-only operation.

The issue I have with the change is that it's very much set up on this very
narrow case. For example, I don't believe those transitions are properly
handled:
* inline positioned to static block
* inline positioned to block positioned

The issue seems to be that whenever we are out of flow positioned,
RenderStyle::display() will return 'block' as it is overriden. One way to fix
that is just to patch the display checks above to use original display. Another
one would be to use the proper display (RenderStyle's original vs overriden
display) based on whether you are in normal flow or out of flow.

Maybe somebody may know about which one is best. The safest is to always use
the original display and would be my pick.

>
LayoutTests/fast/inline/absolute-positioned-object-from-block-flow-to-inline-fl
ow.html:39
> +var previousSibling = '';
> +var currentTestNode = '';

Why do you initialize those 2 variables to be strings vs just 'null'?

shouldBe() requires variable that have a global scope as it calls eval. You can
do that by not defining those variables. And just set them below.

>
LayoutTests/fast/inline/absolute-positioned-object-from-block-flow-to-inline-fl
ow.html:47
> +	   testElems[i].className += " positioned"

Missing semicolon here (possibly unneeded but let's avoid gray areas)

>
LayoutTests/fast/inline/absolute-positioned-object-from-block-flow-to-inline-fl
ow.html:55
> +		   children[j].className += " inline-block"

Same comment.