Bug 47148

Summary: CSS 2.1 failure: abspos-non-replaced-width-margin-000, abspos-replaced-width-margin-000
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Layout and RenderingAssignee: Robert Hogan <robert>
Severity: Normal CC: bdakin, hamaji, hyatt, robert, sam, simon.fraser, tabatkins
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 47141, 19124    
Description Flags
Patch hyatt: review+

Description Simon Fraser (smfr) 2010-10-04 21:31:47 PDT
Created attachment 69741 [details]

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:
Comment 1 Simon Fraser (smfr) 2010-10-04 21:34:06 PDT
Created attachment 69742 [details]
Comment 2 Simon Fraser (smfr) 2010-10-04 21:34:23 PDT
Created attachment 69743 [details]
Comment 3 Simon Fraser (smfr) 2010-12-01 09:55:07 PST
*** Bug 50330 has been marked as a duplicate of this bug. ***
Comment 4 Simon Fraser (smfr) 2011-01-05 09:25:16 PST
Created attachment 78011 [details]

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>
Comment 5 Simon Fraser (smfr) 2011-01-25 12:02:26 PST
See RenerBox::computePositionedLogicalWidth()
Comment 6 Robert Hogan 2011-07-26 12:17:59 PDT
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').
         * ------------------------------------------------------------------
Comment 7 Robert Hogan 2011-07-30 11:00:02 PDT
The code asks the question:

    // 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.
Comment 8 Robert Hogan 2011-07-31 07:28:15 PDT
Created attachment 102458 [details]
Comment 9 Robert Hogan 2011-08-02 12:34:17 PDT
Created attachment 102683 [details]
Comment 10 Robert Hogan 2011-08-02 12:38:56 PDT
(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()
Comment 11 Robert Hogan 2011-08-11 11:53:26 PDT
Created attachment 103651 [details]
Comment 12 Dave Hyatt 2011-08-22 12:18:42 PDT
Comment on attachment 103651 [details]

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.
Comment 13 Dave Hyatt 2011-08-22 12:25:15 PDT
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:


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.
Comment 14 Robert Hogan 2011-09-05 12:05:18 PDT
Created attachment 106350 [details]
Comment 15 Robert Hogan 2011-09-06 13:49:26 PDT
Created attachment 106479 [details]
Comment 16 Robert Hogan 2011-09-06 13:56:35 PDT
(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
Comment 17 Dave Hyatt 2011-09-15 14:18:21 PDT
Comment on attachment 106479 [details]

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.
Comment 18 Dave Hyatt 2011-09-15 14:52:42 PDT
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


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.
Comment 19 Robert Hogan 2011-09-17 06:28:28 PDT
Created attachment 107768 [details]
Comment 20 Robert Hogan 2011-09-20 04:27:37 PDT
(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.
Comment 21 Dave Hyatt 2011-09-20 12:09:51 PDT
Comment on attachment 107768 [details]

Comment 22 Robert Hogan 2011-09-22 10:17:22 PDT
Committed r95726: <http://trac.webkit.org/changeset/95726>