CSS 2.1 failure: abspos-non-replaced-width-margin-000, abspos-replaced-width-margin-000

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 69741 [details]
Screenshot

These tests fail:

html4/abspos-non-replaced-width-margin-000	fail
html4/abspos-replaced-width-margin-000	fail

note that these are ref tests, and should be compared to:
abspos-non-replaced-width-margin-000-ref.html
abspos-replaced-width-margin-000-ref.htm
respectively.

[Simon Fraser (smfr)]

Created attachment 69742 [details]
abspos-non-replaced-width-margin-000-ref.htm

[Simon Fraser (smfr)]

Created attachment 69743 [details]
abspos-non-replaced-width-margin-000.htm

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

Created attachment 78011 [details]
Reduction

This issue seems to be related to the positioning of absolutely positioned elements whose containing block is RTL. Relevant spec is <http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-width>

[Simon Fraser (smfr)]

See RenerBox::computePositionedLogicalWidth()

[Robert Hogan]

This seems to be the reason:

         *              OMIT RULE 2 AS IT SHOULD NEVER BE HIT
         * ------------------------------------------------------------------
         * 2. 'left' and 'right' are 'auto' and 'width' is not 'auto', then if
         *    the 'direction' property of the containing block is 'ltr' set
         *    'left' to the static position, otherwise set 'right' to the
         *    static position. Then solve for 'left' (if 'direction is 'rtl')
         *    or 'right' (if 'direction' is 'ltr').
         * ------------------------------------------------------------------

[Robert Hogan]

The code asks the question:

    // QUESTIONS
    // FIXME 1: Which RenderObject's 'direction' property should used: the
    // containing block (cb) as the spec seems to imply, the parent (parent()) as
    // was previously done in calculating the static distances, or ourself, which
    // was also previously done for deciding what to override when you had
    // over-constrained margins?  Also note that the container block is used
    // in similar situations in other parts of the RenderBox class (see computeLogicalWidth()
    // and computeMarginsInContainingBlockInlineDirection()). For now we are using the parent for quirks
    // mode and the containing block for strict mode.

When calculating the static distance for non-replaced elements in computeInlineStaticDistance() the answer seems to be the parent() - that allows abspos-non-replaced-width-margin-000.htm to pass. It also improves the result for abspos-non-replaced-width-margin-000.htm, but there is still a problem with one or cases there.

[Robert Hogan]

Created attachment 102458 [details]
Patch

[Robert Hogan]

Created attachment 102683 [details]
Patch

[Robert Hogan]

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

This patch makes WebKit match the reference test results exactly. There is one part to the fix that I don't fully understand, and that is the need the fix has to treat any replaced element in an RTL containing block as 'over-constrained' in computePositionedLogicalWidthReplaced()

[Robert Hogan]

Created attachment 103651 [details]
Patch

[Dave Hyatt]

Comment on attachment 103651 [details]
Patch

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

I guess I'll trust the quirks mode elimination, but why remove the strict mode test case? Is it actually wrong?

> Source/WebCore/rendering/RenderBox.cpp:2193
> -        int staticPosition = child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock->borderLogicalRight();
> +        int staticPosition = enclosingBox->borderAndPaddingStart() + containerLogicalWidth + containerBlock->borderLogicalLeft();

I do not understand this change. Using left borders when the offset is designed to be from the right edge does not make any sense to me.

[Dave Hyatt]

staticInlinePosition is not a hardcoded offset "from the left." It actually matches the directionality of the content you're looking at when you do the static computation.

Check out:

setStaticPositions

and take a look. It always uses start offsets.

Now maybe you're running into trouble when you try to mix directionality, but I'd take a look at that code to try to figure out what went wrong. The offset is very much supposed to be from the right side for RTL content.

[Robert Hogan]

Created attachment 106350 [details]
Patch

[Robert Hogan]

Created attachment 106479 [details]
Patch

[Robert Hogan]

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

So the updates here are:

- Change RenderBlock::startOffsetForLine() to count from the right for RTL blocks, rather than the left.

- Explain why computeInlineStaticDistance() needs to account for the container block's borderLogicalLeft() even when the block is RTL: "In the context of this function, the logicalRight value is just an element that will be used later to calculate the correct logicalLeft position for the RTL block. So whereas an LTR block can subtract containerBlock->borderLogicalLeft() in this function directly, an RTL block has to add it here so that will be later subtracted from availableSpace to get logicalLeft in computePositionedLogicalWidthUsing()."

- Retain absolute-position-direction-quirk.html and rename it to absolute-position-direction.html

[Dave Hyatt]

Comment on attachment 106479 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.h:132
> -    LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine) const { return style()->isLeftToRightDirection() ? logicalLeftOffsetForLine(position, firstLine) : logicalRightOffsetForLine(position, firstLine); }
> +    LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine) const;
>      LayoutUnit startAlignedOffsetForLine(RenderBox* child, LayoutUnit position, bool firstLine);

This can still be inlined. You just needed to change it to this:

LayoutUnit startOffsetForLine(LayoutUnit position, bool firstLine) const { return style()->isLeftToRightDirection() ? logicalLeftOffsetForLine(position, firstLine) : width() - logicalRightOffsetForLine(position, firstLine); }

You can patch startAlignedOffsetForLine the same way... just do width() - (final answer).

Seems like we need to make sure we have tests that cover this so we don't break it again.

> Source/WebCore/rendering/RenderBox.cpp:2214
> -        LayoutUnit staticPosition = child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock->borderLogicalRight();
> +        LayoutUnit staticPosition = child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock->borderLogicalLeft();

I still don't understand why a left offset is being added in when the static position is from the right side. I'm just not following this part. Can you maybe make sure you have startOffsetForLine right and double-check? Or explain this to me, because I don't get it. It's not clear to me why you add in a left hand side coordinate to an offset from a right edge.

[Dave Hyatt]

For this code:


LayoutUnit staticPosition = child->layer()->staticInlinePosition() + containerLogicalWidth + containerBlock->borderLogicalRight();
staticPosition -= enclosingBox->logicalWidth();
for (RenderObject* curr = enclosingBox; curr && curr != containerBlock; curr = curr->container()) {
    if (curr->isBox())
        staticPosition -= toRenderBox(curr)->logicalLeft();
}

I think this could be written to be less confusing.

Your offset is supposed to be from the right padding edge. So what you're trying to do is the same thing the left code did.

LayoutUnit staticPosition = child->layer()->staticInlinePosition() - containerBlock->borderLogicalLeft();
        for (RenderObject* curr = child->parent(); curr && curr != containerBlock; curr = curr->container()) {
            if (curr->isBox())
                staticPosition += toRenderBox(curr)->logicalLeft();
        }
        logicalLeft.setValue(Fixed, staticPosition);

If I were to take the LTR code and write it abstractly it would be like this:

LayoutUnit staticPosition = (child offset from start border edge);

This puts the staticPosition into the correct coordinate space (the padding box of the containing block) and has it offset from the start edge.

Now you loop up adding in the offset for the start edge. In left hand side coordinates. This is represented as logicalLeft for LTR and as parent box width - (logicalLeft + logicalWidth) in RTL.

At the very end you will have an offset from the right border edge of your containing block. Now you have to subtract out the borderStartWidth to be an offset from the padding edge instead.

What the current RTL code does instead seems like a way more confusing way to write the code. It seems like you could just do something like this instead:

LayoutUnit staticPosition = child->layer()->staticInlinePosition() - containerBlock->borderStart();
for (RenderObject* curr = child->parent(); curr && curr != containerBlock; curr = curr->container()) {
    if (curr->isBox())
        staticPosition += toRenderBox(curr)->startOffset();
}

Where startOffset is a new function that returns:

logicalLeft() for LTR

and

containingBlock()->width() - logicalRight() for RTL.

I suppose repeated calls to containingBlock() might be slower than what the current code does, so I guess I won't require a change, but I think the current way the RTL loop is written is super confusing.

[Robert Hogan]

Created attachment 107768 [details]
Patch

[Robert Hogan]

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

I added a new test, left-right-center-inline-alignment-in-ltr-and-rtl-blocks.html, which tests the alignment of inlines for both LTR and RTL. Neither FF nor Opera 'pass' this test.

[Dave Hyatt]

Comment on attachment 107768 [details]
Patch

r=me

[Robert Hogan]

Committed r95726: <http://trac.webkit.org/changeset/95726>