CSS 2.1 failure: inline-box-002.htm fails

• Component: CSS
• Status: RESOLVED FIXED

[Robert Hogan]

See http://lists.w3.org/Archives/Public/public-css-testsuite/2011Sep/0070.html

[Robert Hogan]

The render tree for the relevant part of this test looks like this:

layer at (8,52) size 192x192
  RenderBlock (relative positioned) {DIV} at (0,36) size 192x192 [bgcolor=#FFFF00]
    RenderBlock (anonymous) at (0,0) size 192x20
    RenderBlock (anonymous) at (0,20) size 192x20
      RenderBlock {DIV} at (0,0) size 192x20 [bgcolor=#FFA500]
        RenderText {#text} at (0,0) size 61x19
          text run at (0,0) width 61: "Filler Text"
    RenderBlock (anonymous) at (0,40) size 192x20
      RenderText {#text} at (0,0) size 0x0
layer at (8,244) size 61x19
  RenderInline (relative positioned) {DIV} at (0,0) size 61x19 [bgcolor=#0000FF]
    RenderText {#text} at (0,0) size 61x19
      text run at (0,0) width 61: "Filler Text"
layer at (8,284) size 61x19
  RenderInline (relative positioned) {DIV} at (0,0) size 61x19 [bgcolor=#0000FF]
    RenderText {#text} at (0,0) size 61x19
      text run at (0,0) width 61: "Filler Text"

The RenderInlines have the first and third anonymous block as their containingBlock(). In order to pass the RenderBlock {DIV} with color #FFA500 needs to treat the RenderInline as its parent.

[Robert Hogan]

It's not clear to me how:

    RenderBlock (anonymous) at (0,20) size 192x20
      RenderBlock {DIV} at (0,0) size 192x20 [bgcolor=#FFA500]

can reparent to 

RenderInline (relative positioned) {DIV} at (0,0) size 61x19 [bgcolor=#0000FF]

Or even if reparenting to the RenderInline's anonymous containing block would be sufficient - I suspect it would not.

[Dave Hyatt]

(1) Make the anonymous block relatively positioned if any split inlines are relatively positioned and give it a layer.

(2) Patch the relativePositionOffset methods to crawl up the split inlines accumulating an offset to use.

[Robert Hogan]

Created attachment 122570 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122570 [details]
Patch

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

New failing tests:
fast/css/relative-positioned-block-with-inline-parent.html

[Robert Hogan]

Created attachment 122576 [details]
Patch

[Dave Hyatt]

Comment on attachment 122576 [details]
Patch

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

One comment:

> Source/WebCore/rendering/RenderInline.cpp:251
> +        if (isRelPositioned())
> +            newStyle->setPosition(RelativePosition);

Don't you also need to set the block to be relatively positioned if an ancestor inline is relatively positioned? Seems like you might need to do that when splitting ancestors.

Make a test case with <span style="position:relative; left:50px;top:50px"><span><i><b>etc.etc. <div>HAHA!

to see what I mean.

[Robert Hogan]

Created attachment 123023 [details]
Patch

[Dave Hyatt]

Comment on attachment 123023 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:411
> +    if (isAnonymousBlock() && isRelPositioned()) {
> +        RenderInline* p = toRenderBlock(this)->inlineElementContinuation();
> +        while (p) {
> +            offset += p->relativePositionOffsetX();
> +            RenderObject* parent = p->parent();
> +            p = parent->isRenderInline() ? toRenderInline(parent) : 0;
> +        }
> +    }

All this code is duplicated in relativePositionOffsetY as well. Pull it into a little static helper function please.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:437
> +        while (p) {

Seems easier to just write while (p && p->isRenderInline()) and then just p = p->parent();

> Source/WebCore/rendering/RenderInline.cpp:246
> +static bool hasRelPositionedAncestor(RenderInline* p)
> +{
> +    while (p) {
> +        if (p->isInline() && p->isRelPositioned())
> +            return true;
> +        RenderObject* parent = p->parent();
> +        p = parent->isRenderInline() ? toRenderInline(parent) : 0;
> +    }
> +    return false;
> +}

Seems like this could be more easily written (assuming p is just a RenderObject as):

while (p && p->isRenderInline()) {
    if (p->isRelPositioned())
        return true;
    p = p->parent();
}
return false;

> Source/WebCore/rendering/RenderInline.cpp:265
> +        if (hasRelPositionedAncestor(this))
> +            newStyle->setPosition(RelativePosition);

You can't just do this at creation time. You're assuming a static state as far as all the enclosing RenderInlines at the time the splitting of the inlines happens. However, it's possible that much later on, someone could come along and make one of the RenderInlines relative positioned (or the opposite, take away relative positioning).

You need a test case that starts off with nothing relatively positioned, and then waits and converts one of the inlines in the chain to be relatively positioned later to see what I mean.

Might be easier to just chat on IRC about how to handle the dynamic situation.

Note that I will still r+ the non-dynamic case since it is an improvement, so if you want to land this initially I'm cool with it.

[Robert Hogan]

Created attachment 124575 [details]
Patch

[Dave Hyatt]

Comment on attachment 124575 [details]
Patch

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

> Source/WebCore/rendering/RenderInline.cpp:140
> +static bool containsDescendantRenderBlock(RenderObject* child, RenderObject* ancestor)
> +{
> +    ASSERT(child->firstChild() && child->firstChild()->isRenderBlock());
> +    
> +    RenderObject* p = toRenderBlock(child)->inlineElementContinuation();
> +    while (p && p->isRenderInline()) {
> +        if (p == ancestor)
> +            return true;
> +        p = p->parent();
> +    }
> +    return false;
> +}

This function is not necessary. You can just cut it.

> Source/WebCore/rendering/RenderInline.cpp:159
> +        if (block && block->isAnonymousBlock() && containsDescendantRenderBlock(block, currCont)) {

The containing block's next sibling will always be an anonymous block, and its contents will always be descendants of this inline. So I think you could just change the block && block->isAnonymousBlock() to be an assert instead and you can just remove containsDescendantRenderBlock.

> Source/WebCore/rendering/RenderInline.cpp:167
> +    for (; currCont; currCont = currCont->inlineElementContinuation()) {

One issue I see is it looks like you update only one of the anonymous blocks? An inline can be split multiple times and thus there can be a whole chain of descendant blocks that need updating. It looks like your code only updates one.

The other issue I see is that you are assuming that your style turns relative positioning on and off for the block, but what if multiple inlines are contributing relative positioning? For example:

<span><span style="position:relative; left:10px;top:10px"><div>Hello</div>

In the above example, turning relative positioning off and on for the outermost span has no impact, since the inner span is still position:relative.

I think you are on the right track, but you probably need to find a way to just :"dirty" the blocks somehow so that the positioning change update is done by the block one time and it's done by crawling up the inlines chain examining their position values.

You might need some kind of state for this.

[Robert Hogan]

Created attachment 124600 [details]
Patch

[Dave Hyatt]

Comment on attachment 124600 [details]
Patch

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

Getting closer.

> Source/WebCore/rendering/RenderInline.cpp:139
> +static void updateStyleOfDescendantBlocks(RenderObject* block, const RenderStyle* newStyle)

I don't think "Descendant" is the best name here. updateStyleForAnonymousBlockContinuations might be better.

> Source/WebCore/rendering/RenderInline.cpp:142
> +        if (block->firstChild()->isRenderBlock()) {

A better way of asking this question would be "Is this block a continuation?" There's a method for that!

if (block->isAnonymousBlockContinuation())

I'd also add a check to see whether or not the block's current style is already relatively positioned. If it is, then there's no need to create a new style.

if (block->isAnonymousBlockContinuation() && block->style()->position() != newStyle->position())

> Source/WebCore/rendering/RenderInline.cpp:169
> +        && (newStyle->position() == RelativePosition || (oldStyle->position() == RelativePosition && !hasRelPositionedAncestor(this)))) {

This check still doesn't seem good enough. If you lose relative positioning but a descendant inline has relative positioning still, it seems like your check here will still turn off relative positioning on the anonymous block descendant. In other words, this case:

<span style="position:relative"><span style="position:relative">Hello world

If you turn off the outer one, it looks like your code will turn off relative positioning, even though the inner one has relative positioning still. Your hasRelPositionedAncestor check needs to start from the anonymous block continuations and work its way up. It can't start from the current inline whose style you happened to change.

[Robert Hogan]

Created attachment 124797 [details]
Patch

[Dave Hyatt]

Comment on attachment 124797 [details]
Patch

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

Really close, but not quite! ;)

> Source/WebCore/rendering/RenderInline.cpp:129
> +static bool hasRelPositionedAncestor(RenderObject* p)

I'd probably put the word Inline in this method... e.g., hasRelPositionedInlineAncestor

> Source/WebCore/rendering/RenderInline.cpp:145
> +    if (oldStyle->position() == RelativePosition && hasRelPositionedAncestor(cont))
> +        return;

You have to do this per-block. You can't only do it for the |block| argument. Consider this testcase:

<span style="position:relative"><span>One <div>First block</div> <span style="position:relative">Two <div>Second block</div></span></span></span>

If you remove the position:relative on the outermost span and only check starting at |block|'s continuation, then you'll only examin the outermost two spans. However a later anonymous block continuation actually is split around a deeper nesting stack, and it has a third span that has position:relative that you won't see.

Basically you can fix this by turning this into part of the condition inside the loop instead, so that you do it for each block.

[Robert Hogan]

Created attachment 124804 [details]
Patch

[Dave Hyatt]

Comment on attachment 124804 [details]
Patch

There's one more issue that I see here,  and it's that the relative positioning information will be lost if the anonymous block has it style changed via some other mechanism. Basically RenderBlock::styleDidChange needs to ask if (isAnonymousBlockContinuation()) and make sure the newStyle preserves the position of the oldStyle.

Here is a test case. Add height/width/whatever to make it all visible:

<div style="color:red"><span style="position:relative">Hello <div>Block</div></span></div>

If you alter the outermost div's background color from red to green, the anonymous block continuation is going to update its style. At that point it will lose the knowledge that it needed to be relatively positioned. You basically just have to preserve that across the change from oldStyle to newStyle.

[Dave Hyatt]

Sorry, I meant "color from red to green" not "background color"

[Robert Hogan]

Created attachment 124829 [details]
Patch

[Dave Hyatt]

Comment on attachment 124829 [details]
Patch

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

r=me

> Source/WebCore/rendering/RenderObject.cpp:1952
> +        if (toRenderBlock(child)->isAnonymousBlockContinuation())
> +            newStyle->setPosition(child->style()->position());

I think this would read more cleanly as:

if (child->isRelPositioned() && toRenderBlock(child)->isAnonymousBlockContinuation())
    newStyle->setPosition(RelativePosition);

If the child already wasn't relatively positioned (the common case), then you don't need to mutate newStyle, since it won't be relatively positioned either.

[Robert Hogan]

*** Bug 53140 has been marked as a duplicate of this bug. ***

[Robert Hogan]

Committed r108185: <http://trac.webkit.org/changeset/108185>