Bug 91665 – When a block element is made inline positioned and has static left and right, it does not follow inline formatting context

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Pravin D]

An inline positioned object(absolute/fixed) with static inline positions(static left and right) follows inline formatting context. However when the display property of a block-level element(positioned or otherwise) having static inline positions, is changed to inline(inline-block etc...) , the element is not re-laid using inline formatting context.

This bug is related to https://bugs.webkit.org/show_bug.cgi?id=18090 . However the root causes are different.

[Pravin D]

Created attachment 153083 [details]
TestCase

The test is purely from the  Comment #3 https://bugs.webkit.org/show_bug.cgi?id=18090 by  Tab Atkins.

[Pravin D]

Created attachment 153092 [details]
Patch

[Pravin D]

Internally the display of all positioned objects is set to block. Original display is the display property set its style.

When a we make display:inline (inline-block etc...) and position:abs or fixed, 
the element's original display is set to inline. However as it is a positioned object its display properly is set as block. 

When we reset the display:block to inline , style recalc happens. During recalc based on the elements previous style and current style a new renderer is created for the element. On of the criteria is that if the display properties differ then a new renderer (reattach) is created for the element.

However in our case as the display property for a positioned element is always block, no reattach happens when display:block is changed to display:inline. So the position object will not become part of inline formatting context. The positioned element reqs to be in a inline formatting context when its left, right are static.

Thus in Node:diff we to also req to check the original display of the positioned element against it old display to decide if we need to reattach its renderer or not

[WebKit Review Bot]

Comment on attachment 153092 [details]
Patch

Attachment 153092 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13292005

New failing tests:
compositing/rtl/rtl-iframe-relative.html
compositing/rtl/rtl-iframe-absolute-overflow.html
compositing/rtl/rtl-iframe-fixed-overflow.html
animations/cross-fade-webkit-mask-image.html
compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html
compositing/rtl/rtl-iframe-fixed.html
compositing/rtl/rtl-iframe-absolute.html
animations/cross-fade-webkit-mask-box-image.html
compositing/iframes/fixed-position-iframe.html
compositing/rtl/rtl-iframe-fixed-overflow-scrolled.html

[WebKit Review Bot]

Created attachment 153112 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pravin D]

Created attachment 153297 [details]
Patch

[Robert Hogan]

Comment on attachment 153297 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:372
> +    bool isOriginalDisplay1InlineType = s1 ? s1->isOriginalDisplayInlineType() : false;

Maybe 's1 && s1->isOriginalDisplayInlineType()' ?

> Source/WebCore/dom/Node.cpp:374
> +    if (!isOriginalDisplay1InlineType && isOriginalDisplay2InlineType && s2->isOutOfFlowPositioned())

You need better variable names than variable1 and variable2 here.

> Source/WebCore/dom/Node.cpp:375
> +        ch= Detach;

whitespace nit 'ch ='

> LayoutTests/ChangeLog:9
> +        * fast/inline/absolute-positioned-object-from-block-flow-to-inline-flow-expected.html: Added.
> +        * fast/inline/absolute-positioned-object-from-block-flow-to-inline-flow.html: Added.

Oh - you probably shouldn't use reference results with tests that contain javascript/setTimeOuts. It may work most of the time but is likely to be flakey or give false positives. I think you should make the results of this test plain text. The guys can correct me if I'm wrong here.

[Pravin D]

Created attachment 153562 [details]
Patch

[Pravin D]

(In reply to comment #7)
> (From update of attachment 153297 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153297&action=review
> 
> > Source/WebCore/dom/Node.cpp:372
> > +    bool isOriginalDisplay1InlineType = s1 ? s1->isOriginalDisplayInlineType() : false;
> 
> Maybe 's1 && s1->isOriginalDisplayInlineType()' ?
> 
> > Source/WebCore/dom/Node.cpp:374
> > +    if (!isOriginalDisplay1InlineType && isOriginalDisplay2InlineType && s2->isOutOfFlowPositioned())
> 
> You need better variable names than variable1 and variable2 here.
> 
> > Source/WebCore/dom/Node.cpp:375
> > +        ch= Detach;
> 
> whitespace nit 'ch ='
> 
> > LayoutTests/ChangeLog:9
> > +        * fast/inline/absolute-positioned-object-from-block-flow-to-inline-flow-expected.html: Added.
> > +        * fast/inline/absolute-positioned-object-from-block-flow-to-inline-flow.html: Added.
> 
> Oh - you probably shouldn't use reference results with tests that contain javascript/setTimeOuts. It may work most of the time but is likely to be flakey or give false positives. I think you should make the results of this test plain text. The guys can correct me if I'm wrong here.
> 

Have upload a new patch with suggested changes...

[WebKit Review Bot]

Comment on attachment 153562 [details]
Patch

Attachment 153562 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13312354

New failing tests:
compositing/rtl/rtl-iframe-relative.html
compositing/rtl/rtl-iframe-absolute-overflow.html
compositing/rtl/rtl-iframe-fixed-overflow.html
animations/cross-fade-webkit-mask-image.html
compositing/rtl/rtl-iframe-absolute-overflow-scrolled.html
compositing/rtl/rtl-iframe-fixed.html
compositing/rtl/rtl-iframe-absolute.html
animations/cross-fade-webkit-mask-box-image.html
compositing/iframes/fixed-position-iframe.html
compositing/rtl/rtl-iframe-fixed-overflow-scrolled.html

[WebKit Review Bot]

Created attachment 153597 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Seidel (no email)]

Looks liek this causes a bunch of test failures on chromium (and probably every port).

[Pravin D]

Created attachment 153630 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 153630 [details]
Patch

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

> 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?

[Pravin D]

(In reply to comment #14)
> (From update of attachment 153630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153630&action=review
> 
> > 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.
> 

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.



> 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?
> 

We need to reattach(return val of Node::diff is Detach) only if the element's actual display property(one set in style) changes from block to inline.

Just using isOutofFlowPositioned() != isOutOfFlowPositioned() will not be so efficient i guess...

I guess we can have 3 cases...
1) The previous style --- display:block and position:static 
    current style --- display: inline and position: abs
2) The previous style --- display:block and position:abs 
    current style --- display: inline and position: abs
3) The previous style --- display:inline and position:static 
    current style --- display: inline and position: abs
Need not worry about the (3) case... Only (1) and (2) are relevent(I guess)...

[Julien Chaffraix]

Comment on attachment 153630 [details]
Patch

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-flow.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-flow.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-flow.html:55
> +                children[j].className += " inline-block"

Same comment.

[Pravin D]

Created attachment 156162 [details]
Proposed Patch

[Pravin D]

(In reply to comment #16)
> (From update of attachment 153630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153630&action=review
> 
> 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
> 
There are 3 places where the various display,position scenario are being handled.
1)
Node::diff mainly handles the case where Inline, static is made block, positioned. 

2)
RenderObject::handleDynamicFloatPositionChange() handles the case where a positioned(both block and inline) changes to position:static(both block and inline). http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderObject.cpp#L1632

Related Change set
http://trac.webkit.org/changeset/4421/trunk/WebCore/khtml/rendering/render_object.cpp

3)
setStaticPositions() called from RenderBlock::layoutRunsAndFloatsInRange() handles the case inline to block and vice versa for positioned objects. 
http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp#L1347

Related Change set
http://trac.webkit.org/changeset/81992/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

------
The case static block to positioned inline is the only case that has been missed.

The possible places to patch the issue is at Node::diff() or at handleDynamicFloatPositionChange(). The 3rd location (setStaticPositions()) is ruled out as it reqs that the obj be in inline flow.

> 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.
> 
RenderStyle's original vs overriden display 
It depends on the context. For example inline positioned objects with static inline positions will behave as a inline-block so original display is req. On the other hand Legend tag will always behave as block. So original display is not req.

[Pravin D]

(In reply to comment #17)
> Created an attachment (id=156162) [details]
> Patch

This patch uses RenderObject::handleDynamicFloatPositionChange() to solve the issue. 

Positioned objects donot affect the parents. When object changes from block static to inline positioned, it must be put as part of inline flow just so that its static inline positions are calculated properly. An reattach() on such object might be a overkill. 

Uploaded a patch to check if the approach is acceptable or not. 

@Julien -- Please let me know your opinion on this...

[Julien Chaffraix]

Thanks for investigating this. I agree with your investigation, however I think the <legend> case is likely an oversight.

(In reply to comment #19)
> (In reply to comment #17)
> > Created an attachment (id=156162) [details] [details]
> > Patch
> 
> This patch uses RenderObject::handleDynamicFloatPositionChange() to solve the issue.

It makes sense to piggyback on the existing logic.

> Positioned objects donot affect the parents. When object changes from block static to inline positioned, it must be put as part of inline flow just so that its static inline positions are calculated properly. An reattach() on such object might be a overkill.

It's a trade-off. I would consider such transition to be rare but without data, it makes sense to be conservative and just patch handleDynamicFloatPositionChange to do the right thing. Could clean-up your patch (better names and less trailing white-spaces)? Also could make sure the other transitions are properly covered?

[Pravin D]

Created attachment 157479 [details]
Patch

[Pravin D]

(In reply to comment #20)
> Thanks for investigating this. I agree with your investigation, however I think the <legend> case is likely an oversight.
> 
> (In reply to comment #19)
> > (In reply to comment #17)
> > > Created an attachment (id=156162) [details] [details] [details]
> > > Patch
> > 
> > This patch uses RenderObject::handleDynamicFloatPositionChange() to solve the issue.
> 
> It makes sense to piggyback on the existing logic.
> 
> > Positioned objects donot affect the parents. When object changes from block static to inline positioned, it must be put as part of inline flow just so that its static inline positions are calculated properly. An reattach() on such object might be a overkill.
> 
> It's a trade-off. I would consider such transition to be rare but without data, it makes sense to be conservative and just patch handleDynamicFloatPositionChange to do the right thing. Could clean-up your patch (better names and less trailing white-spaces)? Also could make sure the other transitions are properly covered?
> 
have uploaded a new cleaner patch... patching handleDynamicFloatPositionChange()...

[WebKit Review Bot]

Comment on attachment 157479 [details]
Patch

Attachment 157479 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13464388

New failing tests:
fast/dynamic/absolute-positioned-inline-static-block.html

[WebKit Review Bot]

Created attachment 157507 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pravin D]

Created attachment 157754 [details]
Patch

[Pravin D]

(In reply to comment #23)
> (From update of attachment 157479 [details])
> Attachment 157479 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13464388
> 
> New failing tests:
> fast/dynamic/absolute-positioned-inline-static-block.html
> 
The failure is due to the use of float values for bounding rect values in Chrome.
Other browsers seem to use int values...
On chrome(windows) ---
FAIL currentTestNode["DIV"].getBoundingClientRect().top should be 0. Was 194.53334045410156.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 194.5333251953125.
FAIL currentTestNode["TABLE"].getBoundingClientRect().top should be 0. Was 306.51666259765625.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 306.5333251953125.
FAIL currentTestNode["IMG"].getBoundingClientRect().top should be 0. Was 419.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 418.5333251953125.

On Safari(windows)
FAIL currentTestNode["DIV"].getBoundingClientRect().top should be 0. Was 194.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 194.
FAIL currentTestNode["TABLE"].getBoundingClientRect().top should be 0. Was 306.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 306.
FAIL currentTestNode["IMG"].getBoundingClientRect().top should be 0. Was 418.
FAIL previousSibling["DIV"].getBoundingClientRect().bottom should be 0. Was 418.


Have uploaded a new patch which should not be affect by the use of float values in bounding rect.

[Pravin D]

Just a small demo of the expected and bug behavior...
http://jsfiddle.net/WX5Kf/4/

[Julien Chaffraix]

Comment on attachment 157754 [details]
Patch

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+'\"].getBoundingClientRect().left == \
> +previousSibling[\"'+testElems[i].previousSibling.tagName+'\"].getBoundingClientRect().left','0');
> +        shouldBeTrue('currentTestNode[\"'+testElems[i].tagName+'\"].getBoundingClientRect().top == \

missing space before / after '+'.

[Pravin D]

Created attachment 162349 [details]
Prototype Patch

[kov's GTK+ EWS bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13757747

[Gyuyoung Kim]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13768112

[Build Bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13761721

[Build Bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13755928

[Pravin D]

(In reply to comment #29)
> Created an attachment (id=162349) [details]
> Patch
> 

About the patch.

We have 8 cases:
d1 - initial display
d2 - final display

Considering only the positioned element cases(similar arg apply for floating)

When elements css position changes from static to absolute.

a) d1:inline - > d2:inline.
Is being Handled in Node:diff(). As p2:absolute results in overriding the d2 to block. A re-attach happens which uses the elements original display to decide if the element must be put a block flow or inline flow context. 

b) d1:inline - > d2:block.
Is being Handled in Node:diff(). Same as above.

c) d1:block - > d2:block.
No special handling req.

d)  d1:block - >  d2:inline.
Not handled and the reason for this bug.

When elements css position changes from absolute to static.

i) d1:inline - > d2:inline.
Handled in handleDynamicFloatPositionChange() , renderObject.cpp

ii) d1:inline - > d2:block.
Handled in Node:diff() and redundantly handled in handleDynamicFloatPositionChange() , renderObject.cpp

iii) d1:block - > d2:block.
Handled in handleDynamicFloatPositionChange() , renderObject.cpp

iv) d1:block - >  d2:inline.
Handled in Node:diff() and redundantly handled in handleDynamicFloatPositionChange() , renderObject.cpp

handleDynamicFloatPositionChange() was written to handle the cases when a element changes from positioned to static. However currently its only effective in case (i) and case (iii).

A re-attach does all that is done by handleDynamicFloatPositionChange() plus handles the new emerging cases of regions etc. 

If we are able to handle the case (i) and (iii) also in Node:diff(), handleDynamicFloatPositionChange() and its helper functions can be removed safely.

[Pravin D]

Created attachment 162363 [details]
Prototype Patch

[Pravin D]

(In reply to comment #35)
> Created an attachment (id=162363) [details]
> Patch
> 

Please see comment #34

[Early Warning System Bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13775067

[Early Warning System Bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13755952

[Peter Beverloo (cr-android ews)]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13774109

[WebKit Review Bot]

Comment on attachment 162349 [details]
Prototype Patch

Attachment 162349 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13765522

[WebKit Review Bot]

Comment on attachment 162363 [details]
Prototype Patch

Attachment 162363 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13769373

New failing tests:
fast/dynamic/002.html
fast/repaint/absolute-position-change-containing-block.html
fast/repaint/fixed-to-relative-position-with-absolute-child.html
fast/css/first-letter-removed-added.html

[Pravin D]

(In reply to comment #35)
> Created an attachment (id=162363) [details]
> Patch
> 
About this approach over attachment id=157754
1) Code duplication.
RenderObject::handleDynamicFloatPositionChange() currently handles the cases when an object's position changes from positioned to static or when the element changes from floating to non-floating and when the element's display mis-matches parent's flow context.

We get 2 cases here:
a) When the child is block.
childBecameNonInline() is called on the parent block. The impl of childBecameNonInline(), depending on whether the parent is renderInline or renderBlock is duplication of parts of RenderInline::addChildIgnoringContinuation() or RenderBlock::addChildIgnoringAnonymousColumnBlocks() resp.

b)When child is inline. 
It is assumed/known the parent has a block context. Thus creates an anonymous block and add the child. This logic is already present in addChildIgnoringAnonymousColumnBlocks().

If suppose we try to patch RenderObject::handleDynamicFloatPositionChange() for the current issue, we'll end up duplicating part of the code present in  addChildIgnoringAnonymousColumnBlocks() or RenderInline::addChildIgnoringContinuation()(as evident in attachment 157754 [details]).

2) A re-attach takes care of cases where the element is part of continuation, etc. 
When we do some style change say s1 -> s2 , the final state of the element(parent block, part of virtual children, etc) must be same as if we had only applied style s2. IMHO a re-attach would be better candidate for this.

[Julien Chaffraix]

You make a good point about code duplication between handleDynamicFloatPositionChange and the various methods.

The counter-argument to that would be that changing 'position' or 'float' shouldn't require a re-attachment and now will do. Reattaching is usually for style changes where we can't reuse the same renderer (position being an obvious example). This will likely cause those 2 style changes to take more time. Having some (even rough) performance numbers would help us to evaluate the impact of the change.

[Pravin D]

Created attachment 174646 [details]
Perf TestCases

Set of perf testcases to check the performance impact of patch in attachment https://bugs.webkit.org/attachment.cgi?id=162363

Some of the testcases may not work properly, please see bug https://bugs.webkit.org/show_bug.cgi?id=102070 .

[Pravin D]

Created attachment 174648 [details]
Perf TestCase results

The perf testcase results for code with and without the patch. 

Worst performance increase seems to 15+/- 4 ms in cases where float is converted to static(float:none) . However the results may not be very accurate as In the testcases we always force a layout whenever the style is changes before calculating the time taken for the task.  This is to ensure the style is applied(not sure any other approach). The patch is for the node attachment and style recalc phase and should not directly affect layout performance.

[Julien Chaffraix]

FWIW, I wouldn't have done a measurement one element at a time as this could put you close to Performance.now() accuracy - which is 1 us on non Windows platforms. Some of your tests take about 16 us per measurement which seem too close to the clock resolution to be valuable.

Based on the measurements taking enough time and not too noisy, there seems to be a slow-down after this patch around 1.1%, which is a perfectly acceptable trade-off. Could you post an updated patch passing the EWS?

[Pravin D]

(In reply to comment #46)
> FWIW, I wouldn't have done a measurement one element at a time as this could put you close to Performance.now() accuracy - which is 1 us on non Windows platforms. Some of your tests take about 16 us per measurement which seem too close to the clock resolution to be valuable.
> 
Was not very sure of how to compute the time measurement in this case as the correct way wud have been to use an universal style selector to change the style of 1000 elems. However using runner.js was unsure of creating a proper testcase without much noise.

Would surely be helpful if you give me some suggestion on this, for future ref :) .

> Based on the measurements taking enough time and not too noisy, there seems to be a slow-down after this patch around 1.1%, which is a perfectly acceptable trade-off. Could you post an updated patch passing the EWS?

> 
Will do it.

[Julien Chaffraix]

(In reply to comment #47)
> (In reply to comment #46)
> > FWIW, I wouldn't have done a measurement one element at a time as this could put you close to Performance.now() accuracy - which is 1 us on non Windows platforms. Some of your tests take about 16 us per measurement which seem too close to the clock resolution to be valuable.
> > 
> Was not very sure of how to compute the time measurement in this case as the correct way wud have been to use an universal style selector to change the style of 1000 elems. However using runner.js was unsure of creating a proper testcase without much noise.
> 
> Would surely be helpful if you give me some suggestion on this, for future ref :) .

Bottom line, you should try to avoid doing operations on one element, doing them on a group of elements enables you to modulate the time it takes to finish.

A couple of ways (from the top of my head, there is likely other ways):
* Avoid triggering a layout for each elements but force a layout after updating all your styles.
* Use descendant or child selector to change the style on all elements at once.

[Pravin D]

Created attachment 176333 [details]
Proposed Patch

[Build Bot]

Comment on attachment 176333 [details]
Proposed Patch

Attachment 176333 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15014335

New failing tests:
fullscreen/full-screen-fixed-pos-parent.html
fast/dynamic/002.html
fast/css/first-letter-removed-added.html

[WebKit Review Bot]

Comment on attachment 176333 [details]
Proposed Patch

Attachment 176333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15027287

New failing tests:
fullscreen/full-screen-fixed-pos-parent.html
fast/dynamic/002.html
fast/dynamic/absolute-positioned-to-static-positioned.html
fast/dynamic/floating-to-non-floating.html
fast/dynamic/non-floating-to-floating.html
fast/dynamic/static-positioned-to-absolute-positioned.html
fast/repaint/absolute-position-change-containing-block.html
fast/repaint/fixed-to-relative-position-with-absolute-child.html
fast/css/first-letter-removed-added.html

[Pravin D]

(In reply to comment #50)
> (From update of attachment 176333 [details])
> Attachment 176333 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15014335
> 
> New failing tests:
> fullscreen/full-screen-fixed-pos-parent.html
> fast/dynamic/002.html
> fast/css/first-letter-removed-added.html
> 
Checked the testcase on mac.. they seem to be expected failures(i.e rebaseline req.)

[Pravin D]

(In reply to comment #51)
> (From update of attachment 176333 [details])
> Attachment 176333 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15027287
> 
> New failing tests:
> fullscreen/full-screen-fixed-pos-parent.html
> fast/dynamic/002.html
> fast/dynamic/absolute-positioned-to-static-positioned.html
> fast/dynamic/floating-to-non-floating.html
> fast/dynamic/non-floating-to-floating.html
> fast/dynamic/static-positioned-to-absolute-positioned.html
> fast/repaint/absolute-position-change-containing-block.html
> fast/repaint/fixed-to-relative-position-with-absolute-child.html
> fast/css/first-letter-removed-added.html
> 
My chromium build is not up... will update on the failure when its done...

[Julien Chaffraix]

Comment on attachment 176333 [details]
Proposed Patch

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

> Source/WebCore/ChangeLog:21
> +          Return detach when either position or float property changes for an element.

This is not 100% true: you are checking is-out-of-flow-positioned not position (position: absolute != position: fixed, yet they are both positioned)

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:65
> +            if(elem.className.indexOf('testElem') != -1) {

Is it possible for elem.className.indexOf('testElem') to be -1 knowing that it's part of document.getElementsByClassName('testElem')?

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:85
> +    }

Most of this initialization code seems shared by the different tests, couldn't we share it somehow? At least you should move it into a separate function that runTest() calls.

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:92
> +        expectedPlaceHolderNode[testElems[i].id] = placeHolder;

I don't see the need for expectedPlaceHolderNode, it makes the output less readable and is pretty much unused.

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:94
> +        shouldBe('~~currentTestNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top',  //== \
> +'~~expectedPlaceHolderNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top');

This should be on one line.

Maybe better would be to just use offsetTop?

> LayoutTests/fast/dynamic/non-floating-to-floating.html:86
> +        shouldBe('~~currentTestNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top',  //== \

Even if using ~~ to floor is a neat trick, I find it very counter intuitive. We should probably just use Math.floor for clarity's sake (I don't think the loss in clarity justifies the improvement in speed, if any).

[Pravin D]

Created attachment 177799 [details]
Patch

[Pravin D]

(In reply to comment #54)
> (From update of attachment 176333 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176333&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +          Return detach when either position or float property changes for an element.
> 
> This is not 100% true: you are checking is-out-of-flow-positioned not position (position: absolute != position: fixed, yet they are both positioned)
> 
Done.

> > LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:65
> > +            if(elem.className.indexOf('testElem') != -1) {
> 
> Is it possible for elem.className.indexOf('testElem') to be -1 knowing that it's part of document.getElementsByClassName('testElem')?
> 
Have changed the js code. This condition is no longer used.
 
> > LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:85
> > +    }
> 
> Most of this initialization code seems shared by the different tests, couldn't we share it somehow? At least you should move it into a separate function that runTest() calls.
> 
Created a new JS file and stylesheet to hold the common functions and style.

> > LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:92
> > +        expectedPlaceHolderNode[testElems[i].id] = placeHolder;
> 
> I don't see the need for expectedPlaceHolderNode, it makes the output less readable and is pretty much unused.
> 
Done.

> > LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:94
> > +        shouldBe('~~currentTestNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top',  //== \
> > +'~~expectedPlaceHolderNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top');
> 
> This should be on one line.
>  
Done.

> Maybe better would be to just use offsetTop?
>  
Done.

> > LayoutTests/fast/dynamic/non-floating-to-floating.html:86
> > +        shouldBe('~~currentTestNode[\"'+testElems[i].id+'\"].getBoundingClientRect().top',  //== \
> 
> Even if using ~~ to floor is a neat trick, I find it very counter intuitive. We should probably just use Math.floor for clarity's sake (I don't think the loss in clarity justifies the improvement in speed, if any).
>

[WebKit Review Bot]

Comment on attachment 177799 [details]
Patch

Attachment 177799 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15153556

New failing tests:
fast/repaint/absolute-position-change-containing-block.html
fast/repaint/fixed-to-relative-position-with-absolute-child.html

[Pravin D]

Created attachment 178018 [details]
Patch

[Julien Chaffraix]

Comment on attachment 178018 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +

Please put the result of the performance measurement.

> Source/WebCore/dom/Node.cpp:381
> +    bool wasFloatingOrOutOfFlowPositioned = s1 && (s1->isFloating() || s1->hasOutOfFlowPosition());
> +    bool isFloatingOrOutOfFlowPositioned = s2 && (s2->isFloating() || s2->hasOutOfFlowPosition());

Nit: It would be more readable if you split the condition into floating and out-of-flow.

> LayoutTests/ChangeLog:31
> +           Rebaselined results.

That's not super useful: you are touching some baselines and I can't say without doing some research if it's a progression or not. Help me help you here by indicating what's changing.

Here is my interpretation:
* The 2 repaint tests are progressing: we used to repaint the whole page but now we only paint the old position and the new one.
* full-screen-fixed-pos-parent-expected.txt is orthogonal
* fast/dynamic/002-expected.txt seems that we remove the anonymous wrappers as we detach after the change.

Last point that we discussed on IRC, I would just drop the cover and migrate the tests to check-layout.js as it would mean less boiler plate code in your tests, thus increasing the readibility by a lot!

> LayoutTests/fast/css/first-letter-removed-added-expected.txt:37
> -FAIL document.getElementById('test7').offsetWidth == document.getElementById('ref7').offsetWidth should be true. Was false.
> +PASS document.getElementById('test7').offsetWidth == document.getElementById('ref7').offsetWidth is true

Woooot!

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:18
> +        shouldBe('document.getElementById(\"'+testElems[i].id+'\").offsetTop', 'document.getElementById(\"'+testElems[i].id+'_Cover'+'\").offsetTop');

Usually Javascript follows the same style as C++ (ie spaces before and after '+').

> LayoutTests/fast/dynamic/absolute-positioned-to-static-positioned.html:26
> +<h4> Testcase for <a href="https://bugs.webkit.org/show_bug.cgi?id=91665">Bug #91665. </a> <br> This testcase checks

There is tons of unneeded spaces in the test. They don't add much and make the test cases (already massive) less readable.

Also I would put the bug title but it's more a preference than a requirement.

> LayoutTests/fast/dynamic/non-floating-to-floating.html:28
> +<h4> Testcase for <a href="https://bugs.webkit.org/show_bug.cgi?id=91665">Bug #91665. </a> <br> This testcase checks
> +if an element is properly placed wrt to its previous sibling when  float:none changes to float:left for 
> +various combinations of display property toggeled b/w inline-block and block for both testElement and its previous sibling. </h4> 

Let's not abbreviate:
* wrt => with respect to
* b/w => between

> LayoutTests/fast/dynamic/resources/helper-bug91665.js:4
> +    if(sample.offsetTop != sampleCover.offsetTop) {

This is a definite WTF.

> LayoutTests/fast/dynamic/resources/style-bug91665.css:17
> +.adjustTop20 {
> +  top:-20px;
> +}

FWIW, this name while perfectly fine is tied to 'testElem' being 20px tall. A more appropriate name would be coverTestElement

> LayoutTests/fast/dynamic/resources/style-bug91665.css:22
> +  left:50px;

Again our regular style is to put a space after a colon.

> LayoutTests/fast/dynamic/resources/style-bug91665.css:24
> +.testElem {

Let's not abbreviate: testElement.

[Pravin D]

Created attachment 180385 [details]
Patch

[Julien Chaffraix]

Comment on attachment 180385 [details]
Patch

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

r=me, thanks for sticking to the end. I don't need to see the updated change.

It seems this could potentially impact the bots so I will let you cq it when you are ready.

> Source/WebCore/dom/Node.cpp:388
> +    if (wasFloating != isFloating || wasOutOfFlowPositioned != isOutOfFlowPositioned)

Shouldn't we check:

if ((s1 && s2) && (wasFloating != isFloating || wasOutOfFlowPositioned != isOutOfFlowPositioned))

which would be better written:

if ((s1 && s2) && (s1->isFloating() != s2->isFloating() || s1->hasOutOfFlowPosition() != s2->hasOutOfFlowPosition())

This would prevent the first recalcStyle to detach the renderer.

[Pravin D]

Created attachment 186627 [details]
Patch

[Build Bot]

Comment on attachment 186627 [details]
Patch

Attachment 186627 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16365932

[WebKit Review Bot]

Comment on attachment 186627 [details]
Patch

Clearing flags on attachment: 186627

Committed r142015: <http://trac.webkit.org/changeset/142015>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

It seems some tests failing after the patch landed, at least on GTK and Qt:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r142097%20%2847858%29/results.html

fast/dynamic/absolute-positioned-to-static-positioned.html
fast/dynamic/floating-to-non-floating.html
fast/dynamic/static-positioned-to-absolute-positioned.html

-PASS
+FAIL:
+Expected 23 for offsetTop, but got 22. 
+
+<div class="testElement position-static" data-offset-y="23"></div>



and fast/dynamic/002.html result's also changed:

-        RenderBlock (anonymous) at (0,0) size 784x18
-          RenderText {#text} at (0,0) size 393x19
-            text run at (0,0) width 195: "This text should be on the left. "
-            text run at (195,0) width 198: "The float should be to the right."
+        RenderText {#text} at (0,0) size 393x19
+          text run at (0,0) width 195: "This text should be on the left. "
+          text run at (195,0) width 198: "The float should be to the right."

Maybe this missing "RenderBlock (anonymous)" causes 22 instead of 23.

We skip the three tests until other action.

[Pravin D]

(In reply to comment #66)
> It seems some tests failing after the patch landed, at least on GTK and Qt:
> http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r142097%20%2847858%29/results.html
> 
> fast/dynamic/absolute-positioned-to-static-positioned.html
> fast/dynamic/floating-to-non-floating.html
> fast/dynamic/static-positioned-to-absolute-positioned.html
> 
> -PASS
> +FAIL:
> +Expected 23 for offsetTop, but got 22. 
> +
> +<div class="testElement position-static" data-offset-y="23"></div>
> 
> 
When the same tests are run on Qt5(linux) with a line-height:1.1(currently its 1) set in fast/dynamic/resouces/style-bug91665 seems to work properly.
One of the problems is that line-height cannot be set per platform. I presume the issue is due to the way the height is rounded off on Qt vs other ports... Or its just font specific... I'm not very sure. Need to check further.

> 
> and fast/dynamic/002.html result's also changed:
> 
> -        RenderBlock (anonymous) at (0,0) size 784x18
> -          RenderText {#text} at (0,0) size 393x19
> -            text run at (0,0) width 195: "This text should be on the left. "
> -            text run at (195,0) width 198: "The float should be to the right."
> +        RenderText {#text} at (0,0) size 393x19
> +          text run at (0,0) width 195: "This text should be on the left. "
> +          text run at (195,0) width 198: "The float should be to the right."
> 
> Maybe this missing "RenderBlock (anonymous)" causes 22 instead of 23.
> 
True. The missing anonymous block is causing the diff. Need to update the expected txt file for the same.

[Julien Chaffraix]

This change caused 2 regressions: bug 111091 and bug 111595.

Unless something is done promptly (within a day or 2), I am going to revert the change to prevent further breakage.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111904

[Brent Fulgham]

Safari, Chrome, and Firefox all agree on rendering for this test case. I don't believe there is any remaining compatibility issue.

[Radar WebKit Bug Importer]

<rdar://problem/96960466>