Bug 47148

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

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

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

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>

See RenerBox::computePositionedLogicalWidth()

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'). * ------------------------------------------------------------------

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.

Created attachment 102458 [details] Patch

Created attachment 102683 [details] Patch

(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()

Created attachment 103651 [details] Patch

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.

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.

Created attachment 106350 [details] Patch

Created attachment 106479 [details] Patch

(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 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.

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.

Created attachment 107768 [details] Patch

(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 on attachment 107768 [details] Patch r=me

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