[LayoutNG] RTL abspos inline static position should be flow-relative to the given direction, not line-relative |
||||||||
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.
,
May 31 2018
,
Jun 1 2018
,
Jun 1 2018
Agree with your assessment. There were some refactoring in bidi reorder, so maybe I broke?
,
Jun 1 2018
Reassigning to kojii, since I do not know how to fix inline offsets.
,
Jul 10
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.
,
Jul 10
,
Jul 10
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?
,
Jul 10
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?
,
Jul 11
> 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?
,
Jul 11
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.
,
Jul 11
,
Jul 12
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
,
Jul 12
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.
,
Jul 19
fast/multicol/out-of-flow/abspos-auto-position-on-line-rtl.html
,
Jul 19
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
,
Jul 19
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.
,
Jul 19
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>
,
Jul 19
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.
,
Jul 20
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
,
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
,
Jul 20
> 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 |
||||||||
Comment 1 by atotic@chromium.org
, May 31 2018@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?