Bug 189513 - Wrong static position for out-of-flow positioned element with different writing-mode than its containing block
Summary: Wrong static position for out-of-flow positioned element with different writi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-11 12:09 PDT by Oriol Brufau
Modified: 2021-05-14 10:29 PDT (History)
15 users (show)

See Also:


Attachments
Patch (570.97 KB, patch)
2021-04-29 02:51 PDT, zsun
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (570.97 KB, patch)
2021-04-29 05:43 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (579.32 KB, patch)
2021-05-04 05:26 PDT, zsun
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (584.47 KB, patch)
2021-05-04 06:53 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (584.51 KB, patch)
2021-05-05 07:58 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (584.31 KB, patch)
2021-05-05 12:59 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (584.39 KB, patch)
2021-05-06 02:04 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (595.71 KB, patch)
2021-05-06 02:31 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (595.76 KB, patch)
2021-05-12 02:51 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (584.56 KB, patch)
2021-05-12 03:34 PDT, zsun
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (595.84 KB, patch)
2021-05-12 11:15 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (595.46 KB, patch)
2021-05-12 11:30 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (595.11 KB, patch)
2021-05-13 02:57 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (595.20 KB, patch)
2021-05-13 07:42 PDT, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oriol Brufau 2018-09-11 12:09:43 PDT
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.
Comment 1 Carlos Alberto Lopez Perez 2020-03-23 18:42:55 PDT
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
Comment 2 zsun 2021-04-29 02:51:41 PDT
Created attachment 427332 [details]
Patch
Comment 3 Sergio Villar Senin 2021-04-29 05:11:23 PDT
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()" ?
Comment 4 zsun 2021-04-29 05:43:24 PDT
Created attachment 427339 [details]
Patch
Comment 5 zsun 2021-04-29 05:45:30 PDT
(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!
Comment 6 zsun 2021-05-04 05:26:15 PDT
Created attachment 427653 [details]
Patch
Comment 7 zsun 2021-05-04 06:53:52 PDT
Created attachment 427657 [details]
Patch
Comment 8 zsun 2021-05-05 07:58:08 PDT
Created attachment 427766 [details]
Patch
Comment 9 zsun 2021-05-05 12:59:39 PDT
Created attachment 427795 [details]
Patch
Comment 10 zsun 2021-05-06 02:04:53 PDT
Created attachment 427861 [details]
Patch
Comment 11 zsun 2021-05-06 02:05:24 PDT
Testing. Ignore.
Comment 12 zsun 2021-05-06 02:31:29 PDT
Created attachment 427863 [details]
Patch
Comment 13 Sergio Villar Senin 2021-05-11 04:23:54 PDT
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?
Comment 14 zsun 2021-05-12 02:51:48 PDT
Created attachment 428364 [details]
Patch
Comment 15 zsun 2021-05-12 03:34:33 PDT
Created attachment 428366 [details]
Patch
Comment 16 Oriol Brufau 2021-05-12 07:26:19 PDT
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.
Comment 17 zsun 2021-05-12 11:15:15 PDT
Created attachment 428387 [details]
Patch
Comment 18 zsun 2021-05-12 11:30:41 PDT
Created attachment 428389 [details]
Patch
Comment 19 Sergio Villar Senin 2021-05-13 01:24:47 PDT
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
Comment 20 zsun 2021-05-13 02:57:35 PDT
Created attachment 428477 [details]
Patch
Comment 21 zsun 2021-05-13 07:42:23 PDT
Created attachment 428515 [details]
Patch
Comment 22 EWS 2021-05-14 10:28:40 PDT
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].
Comment 23 Radar WebKit Bug Importer 2021-05-14 10:29:19 PDT
<rdar://problem/78022749>