Bug 189513 – Wrong static position for out-of-flow positioned element with different writing-mode than its containing block

• Component: CSS
• Status: RESOLVED FIXED

[Oriol Brufau]

Run this code: http://jsfiddle.net/4pqcexn5/

```css
#container {
  position: relative;
  border: 5px solid;
  padding-left: 100px;
}
#container > * {
  writing-mode: vertical-lr;
}
#abspos {
  position: absolute;
}
```
```html
<div id="container">
  <div id="abspos">abspos</div>
  <div id="static">static</div>
</div>
```

The abspos element has all 'top', 'left', 'bottom' and 'right' properties set to 'auto', so it should appear at the static position.
However, instead, of being shifted 100px to the right due to padding-left, it's shifted 100px downwards.
The text 'abspos' should overlap 'static'.

This happens when an absolutely (or fixed) positioned element has a different writing mode than its containing block.
Chromium is also affected, Firefox does it right.

[Carlos Alberto Lopez Perez]

Chromium was fixed in M76 according to http://crbug.com/883574

This is tested by the following WPT tests:
 css/css-grid/abspos/positioned-grid-descendants-???.html
 css/css-grid/abspos/orthogonal-positioned-grid-descendants-???.html

Created attachment 427332 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 427332 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3997
> +            staticLogicalTop += child->isHorizontalWritingMode() ? staticLogicalTop : renderBox.logicalLeft();

Shouldn't it be "? renderBox.logicalTop() : renderBox.logicalLeft()" ?

Created attachment 427339 [details]
Patch

(In reply to Sergio Villar Senin from comment #3)
> Comment on attachment 427332 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427332&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:3997
> > +            staticLogicalTop += child->isHorizontalWritingMode() ? staticLogicalTop : renderBox.logicalLeft();
> 
> Shouldn't it be "? renderBox.logicalTop() : renderBox.logicalLeft()" ?

Yes. It should be. I must have pressed wrong button while view my changes locally... Thank you!

Created attachment 427653 [details]
Patch

Created attachment 427657 [details]
Patch

Created attachment 427766 [details]
Patch

Created attachment 427795 [details]
Patch

Created attachment 427861 [details]
Patch

Testing. Ignore.

Created attachment 427863 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 427863 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3573
> +        if (!child->isHorizontalWritingMode() && parent->isHorizontalWritingMode() && child->style().isFlippedLinesWritingMode() && !parent->style().isFlippedBlocksWritingMode())

We should refactor this condition in  a new inline method with a more descriptive name so we could use it in both this method and in the one bellow as well (in computeBlockStaticDistance() you'll still have to add && parentDirection == TextDirection::LTR but that's fine).

I was thinking about a very descriptive name like childIsVLRinHTBParent(). WDTY?

Created attachment 428364 [details]
Patch

Created attachment 428366 [details]
Patch

[Oriol Brufau]

Comment on attachment 428366 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:3556
> +static inline bool childIsVLRParentInHTB(const RenderBox* child)

The name seems confusing to me, sounds like "child is parent". Maybe isVerticalLrChildInHorizontalTbParent?

> Source/WebCore/rendering/RenderBox.cpp:3561
> +    if (!child->isHorizontalWritingMode() && parent->isHorizontalWritingMode() && child->style().isFlippedLinesWritingMode() && !parent->style().isFlippedBlocksWritingMode())
> +        return true;
> +    return false;

Instead of

  if (cond)
    return true;
  return false;

this could just be

  return cond;

> Source/WebCore/rendering/RenderBox.cpp:3598
>              staticPosition += renderBox.logicalLeft();

Missing indentation.

Created attachment 428387 [details]
Patch

Created attachment 428389 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 428389 [details]
Patch

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

Looking better, I have a few other comments.

> Source/WebCore/rendering/RenderBox.cpp:3564
> +static inline bool childIsVLRParentInHTB(const RenderBox* child)

I think the name should be isVLRChildInHTBParent()

Also we know that the child parameter cannot be nullptr, so use a reference instead of a pointer.

Last but not least, I think we should pass a reference to the parent as well, that way the caller is responsible of providing non null values

> Source/WebCore/rendering/RenderBox.cpp:3568
> +}

supernit: it might be easier to understand what this condition is doing if we put the child checks first and the two parent ones later (that way we'd be matching the method's name).

> Source/WebCore/rendering/RenderBox.cpp:3604
> +                staticPosition += renderBox.logicalLeft();

We can use a ternary operator here
staticPosition += isVLRChildInHTBParent(child) ? renderBox.logicalTop() : renderBox.logicalLeft()

> Source/WebCore/rendering/RenderBox.cpp:4017
> +    TextDirection parentDirection = parent->style().direction();

We only use this to compare to TextDirection::LTR. Maybe we can just do

bool isParentDirectionLTR = parent->style().direction() == TextDirection::LTR;

> Source/WebCore/rendering/RenderBox.cpp:4033
> +                staticLogicalTop += renderBox.logicalTop();

Same here with regard to using a ternary operator

Created attachment 428477 [details]
Patch

Created attachment 428515 [details]
Patch

[EWS]

Committed r277497 (237729@main): <https://commits.webkit.org/237729@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428515 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78022749>