New issue
Advanced search Search tips

Issue 848496 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 636993



Sign in to add a comment

[LayoutNG] RTL abspos inline static position should be flow-relative to the given direction, not line-relative

Project Member Reported by atotic@chromium.org, May 31 2018

Issue description

<div style="width:7em; direction:rtl;">
  SS<span style="position:absolute;">PA</span>
</div>

SS is positioned over PA, instead on the side.
 

Comment 1 by atotic@chromium.org, May 31 2018

Cc: kojii@chromium.org
@kojii, I think the static position is incorrect.
Inside NGInlineLayoutAlgorithm::PlaceOutOfFlowObjects, the <span> gets placed into static position 0,0:
 type = blink::NGStaticPosition::kTopRight,
  offset = {
    left = 0px,
    top = 0px
  }
I think the static position should be the point to the right of "SS" (20,0)

What do you think?

Comment 2 by atotic@chromium.org, May 31 2018

Components: Blink>Layout>WritingMode

Comment 3 by kojii@chromium.org, Jun 1 2018

Blocking: 636993

Comment 4 by kojii@chromium.org, Jun 1 2018

Agree with your assessment. There were some refactoring in bidi reorder, so maybe I broke?
Owner: kojii@chromium.org
Reassigning to kojii, since I do not know how to fix inline offsets.
Owner: atotic@chromium.org
Summary: [LayoutNG] RTL abspos inline static position is not correct (was: [LayoutNG] abspos inline static position problem)
Hmm...looking into this in debugger, now it looks the static position is correct to me?

The line:
  SS<span style="position:absolute;">PA</span>
is reordered to:
  <span style="position:absolute;">PA</span>SS
then the line box is aligned to right because it's RTL.

The right-top corner of "PA" should be at the left-top corner of "SS", which is (0, 0), so it renders "PASS", no? If we set it to (20, 0), it can't render "PASS" order?

Please assign me back if this assessment does not seem to be correct.
Cc: atotic@chromium.org
 Issue 860415  has been merged into this issue.
If we're strict about giving it the position it would have had if it was statically positioned, you'd definitely be right.

Still, the test renders as "PASS" in legacy-Blink, Edge, Firefox and Presto.

Spec: https://www.w3.org/TR/CSS22/visudet.html#abs-non-replaced-width

The spec uses the word "roughly" when it comes to setting the position to what the element would have had in the normal flow.

To paraphrase the relevant part of the spec in this case:
<q>The static position for 'right' is the distance from the right edge of the containing block to the right margin edge of a hypothetical box that would have been the first box of the element if its 'position' property had been 'static' and 'float' had been 'none'.</q>

I'm not sure. Is the spec clear enough? Are the other browsers doing it wrong?
I think this is about how to interpret "the position to what the element would have had in normal flow" in bidi context.

<div style="width:7em; direction:rtl; background: yellow;">
  SS<span>PA</span>
</div>

This renders "SSPA", so one could argue "SSPA" is correct. However, by making it an inline-block:

<div style="width:7em; direction:rtl; background: yellow;">
  SS<span style="display: inline-block">PA</span>
</div>

This renders "PASS", because bidi reordering moves the inline block before "SS".

Since OOF are objects for the line layout purpose, I think we should match to the inline-block case, and give the right-top static position at the left-top of "SS".

In LayoutNG, fragments are in visual order, so "SS" is at (0, 0).

The static position, as far as I debugged, is right-top (0, 0), so it looks correct to me. I haven't looked into why "PA" overlaps with "SS". If we can get consensus on the expected static position, I hope Aleks can point it out easily?
> The static position, as far as I debugged, is right-top (0, 0), so it looks 
> correct to me. I haven't looked into why "PA" overlaps with "SS". If we can get 
> consensus on the expected static position, I hope Aleks can point it 
> out easily?

I do not think static position is correct the way I understand it.
AddInlineOutOfFlowChildCandidate(...
    NGLogicalOffset& child_offset,
    TextDirection line_direction)

child_offset is the logical location of fragment corner wrt containing block.
line_direction determines which corner it refers to. 
kRtl means it is top/right corner, kLtr means top/left.

In our example, container is horizontal-tb, rtl. Container's logical origin is top/right corner.

child_offset(0,0), line_direction(kRtl) means that child's top/right corner should be at container's logical origin.

If we want "PA" to be to the left of "SS", its top/right should be left edge of "SS"

If we were using Ahem10px, child_offset should be (20, 0). 

Does this make sense?

Owner: kojii@chromium.org
Finally understood, thank you!

> If we want "PA" to be to the left of "SS", its top/right should be left edge of "SS"

This we seem to be in consensus.

> If we were using Ahem10px, child_offset should be (20, 0).

I see, I thought we're using line-relative (visual) offset, I think you're saying we need to use flow-relative offset based on |line_direction|, correct? Then it's my turn to look into code.

Thank you for pointing out what I misunderstood.
Summary: [LayoutNG] RTL abspos inline static position should be flow-relative to the given direction, not line-relative (was: [LayoutNG] RTL abspos inline static position is not correct)
Something looks not correct, but the WIP breaks many working tests. There might be conditions that are not doing correct, but needs more investigation what it is.
https://chromium-review.googlesource.com/c/chromium/src/+/1133106
You are right, your test does break.

Just looked at it again. I could not figure it out, but here is what I know, maybe you can. I never fully understood line-direction vs container direction, I mostly followed your direction.

NGContainerFragmentBuilder::AddInlineOutOfFlowChildCandidate 
    const NGLogicalOffset& child_offset,
    TextDirection line_direction,
creates NGOutOfFlowPositionedCandidate

NGContainerFragmentBuilder::GetAndClearOutOfFlowDescendantCandidates
converts NGOutOfFlowPositionedCandidate -> NGOutOfFlowPositionedDescendant

Simplified picture (ignoring line direction)

NGOutOfFlowPositionedCandidate location is defined by:
  NGLogicalOffset child_offset -- candidate's logical offset wrt Container being built
  NGOutOfFlowPositionedDescendant
    NGBlockNode node;
    NGStaticPosition static_position;
                     static_position.offset -- descendant physical offset wrt child_offset location.
                     static_position.type -- which corner does static position describe (tl/br, etc)
  
NGOutOfFlowPositionedDescendant location that is returned from GetAndClearOutOfFlowDescendantCandidates
  NGBlockNode node;
  NGStaticPosition static_position;
                   static_position.offset -- descendant physical offset wrt physical fragment
                   static_position.type -- which corner does static position describe (tl/br, etc)

The reason for this complexity are that we need NGOutOfFlowPositionedDescendant physical coordinates. 
When AddInlineOutOfFlowChildCandidate is called, we get logical coordinates, and cannot convert
them to physical until container size is known (which happens later).

##### HOW TO INTERPRET WHAT HAPPENS IN MY CODE

All your code has to do is to make sure that coordinates passed to AddInlineOutOfFlowChildCandidate(child_offset) are converted to physical coordinates correctly in
GetAndClearOutOfFlowDescendantCandidates at 

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/ng/ng_container_fragment_builder.cc?sq=package:chromium&g=0&l=177  

To convert logical to physical, the logical writing mode/direction passed to ConvertToPhysical here has to match writing mode that was assumed in AddInlineOutOfFlowChildCandidate

Initially, I expected AddInlineOutOfFlowChildCandidate to always use Container's writing mode/direction.
inline_layout_algorithm needed a writing mode that was different from Container's. That is why I added in the line_direction argument.

Hope this helps.

fast/multicol/out-of-flow/abspos-auto-position-on-line-rtl.html
Wait, I'm so confused...the test atotic@ wrote in #0 now works good. It was probably taken from fast/multicol/out-of-flow/abspos-auto-position-on-line-rtl.html , but currently it fails because of incorrect rebaseline. The actual result looks correct.

IIRC, it was incorrect when I tested before. Maybe I was dreaming, maybe Ian's recent CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1126807
https://chromium-review.googlesource.com/c/chromium/src/+/1126681
fixed this problem. Maybe the latter, because this is leading OOF case?

Anyway, we used to have separate code paths when OOF is at the beginning of block and when not. The latter CL changed it and we use single code path now. This is good.

I'll rebaseline fast/multicol/out-of-flow/abspos-auto-position-on-line-rtl.html
Oh, no, still confusing!

fast/multicol/out-of-flow/abspos-auto-position-on-line-rtl.html is a ref test, we fail because we fail to render the ref file. Sigh, it took so long to figure it out.
Found the case (not the cause yet), very weird.

http://jsbin.com/layatud/edit?html,output

This works:
  <div dir=rtl>SS<span class="abs">PA</span></div>
but this doesn't:
  <div dir=rtl>SS<span class="abs">PA</span>
  </div>

I think I found it. RTL abspos is working because of a bug in bidi reorder. When I fixed it, it broke 66 RTL abspos tests.
Great that you've found it. This is why RTL is tricky, sometimes two wrongs make it right.

Do you need help fixing up 
NGContainerFragmentBuilder::AddInlineOutOfFlowChildCandidate
NGContainerFragmentBuilder::GetAndClearOutOfFlowDescendantCandidates

to make it work with new bidi code? 

And how does this relate to this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1133106
Change static position of inline abpos to flow relative

Project Member

Comment 21 by bugdroid1@chromium.org, Jul 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/886aed05e9607480a00efb7d2eb17c2030616c9e

commit 886aed05e9607480a00efb7d2eb17c2030616c9e
Author: Koji Ishii <kojii@chromium.org>
Date: Fri Jul 20 06:33:29 2018

[LayoutNG] Change static position of inline abpos to flow relative

This patch:
1. Fixes out-of-flow static position to be flow relative,
   from line-relative.
2. Fixes bidi-reordering orders out-of-flow objects
   incorrectly.

The first fix has been a bug since the beginning, but most
RTL out-of-flow tests pass due to the second problem.

The second problem was introduced when we stopped inserting
an Object Replacement Character for out-of-flow, because it
creates a break opportunity but all 4 impls do not create
a break opportunity for out-of-flow. It wasn't a good idea
because bidi algorithm cannot resolve embedding levels
without a character that represents.

This fix will change break opportunities around out-of-flow
objects. We can fix it in NGLineBreaker if this turns out
to be a problem.

Bug:  848496 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I3f3c7b5a8ddf93cb4b9d6fce21ccc5e2b7623901
Reviewed-on: https://chromium-review.googlesource.com/1133106
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576799}
[modify] https://crrev.com/886aed05e9607480a00efb7d2eb17c2030616c9e/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/886aed05e9607480a00efb7d2eb17c2030616c9e/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
[modify] https://crrev.com/886aed05e9607480a00efb7d2eb17c2030616c9e/third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.cc
[modify] https://crrev.com/886aed05e9607480a00efb7d2eb17c2030616c9e/third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc

Status: Fixed (was: Assigned)
> Do you need help fixing up...

I think these functions are correct and don't need fix. Just that two bugs, one in bidi reorder and one in NGInlineLayoutAlgorithm, worked together to hide the root problem you reported in this issue.

I'll re-review other RTL failures but hopefully we're all good now ;) Thank you again for raising this and all the help to figure out what's happening!!

Sign in to add a comment