1132008 - Implement correct block-axis sizing of ruby text container

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

As it has not been completely clear what should happen, and/or how should block-axis sizing be implemented for ruby base container, and the current behavior of rbcs are fine in most cases, it is decided to implement the sizing for ruby text containers first.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - Calculate bsize of rtc

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - Sync bounds of <rt>s in line layout

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - Remove unused code

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - Update reftests

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39a7945c7b69

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563238 [details] [diff] [review]
patch 1 - Calculate bsize of rtc

>+    nsCSSOffsetState offsetState(child, aReflowState.rendContext, 0);
>+    LogicalMargin margin = offsetState.ComputedLogicalMargin()
>+      .ConvertTo(lineWM, offsetState.GetWritingMode());

Is there a reason you can't use child->GetUsedMargin() instead of
constructing a nsCSSOffsetState (which is somewhat heavyweight)?

Or maybe even add and use nsIFrame::GetMarginRect() (but note in
a comment that it doesn't make sense for elements whose margins collapse)?

>+    nscoord bstart = rect.BStart(lineWM) - margin.BStart(lineWM);
...
>+    nscoord bend = rect.BEnd(lineWM) + margin.BEnd(lineWM);

Please capitalize as bStart and bEnd.

>+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
>+        pos.B(lineWM) += deltaBCoord;
>+        child->SetPosition(lineWM, pos, containerWidth);

This doesn't do the right thing for sticky positioning.  You should
use nsIFrame::MovePositionBy() instead.

r=dbaron with that

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #6)
> Comment on attachment 8563238 [details] [diff] [review]
> patch 1 - Calculate bsize of rtc
> 
> >+    nsCSSOffsetState offsetState(child, aReflowState.rendContext, 0);
> >+    LogicalMargin margin = offsetState.ComputedLogicalMargin()
> >+      .ConvertTo(lineWM, offsetState.GetWritingMode());
> 
> Is there a reason you can't use child->GetUsedMargin() instead of
> constructing a nsCSSOffsetState (which is somewhat heavyweight)?

I guess I can use this.

> Or maybe even add and use nsIFrame::GetMarginRect() (but note in
> a comment that it doesn't make sense for elements whose margins collapse)?
> 
> >+    nscoord bstart = rect.BStart(lineWM) - margin.BStart(lineWM);
> ...
> >+    nscoord bend = rect.BEnd(lineWM) + margin.BEnd(lineWM);
> 
> Please capitalize as bStart and bEnd.

I'd prefer blockStart and blockEnd if we have to use this style.

> >+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
> >+        pos.B(lineWM) += deltaBCoord;
> >+        child->SetPosition(lineWM, pos, containerWidth);
> 
> This doesn't do the right thing for sticky positioning.  You should
> use nsIFrame::MovePositionBy() instead.

No, I can't. I tried using nsIFrame::MovePositionBy, but it produces the incorrect result for relative positioning. The comment above that method also says it should be used after relative positioning.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (UTC+13) (needinfo? for questions) from comment #6)
> >+        LogicalPoint pos = child->GetLogicalPosition(lineWM, containerWidth);
> >+        pos.B(lineWM) += deltaBCoord;
> >+        child->SetPosition(lineWM, pos, containerWidth);
> 
> This doesn't do the right thing for sticky positioning.  You should
> use nsIFrame::MovePositionBy() instead.

Actually, I guess relative positioning hasn't happened yet.  So, instead, you should keep it this way, but add a comment explaining that relative position has not happened yet, so you don't want MovePositionBy.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8563239 [details] [diff] [review]
patch 2 - Sync bounds of <rt>s in line layout

>+ * For containers, their are not part of the line in their levels,
>+ * which means their bounds are not set properly before.

"For containers, their" -> "Containers"


It's confusing to call the rect of the rtc "bounds" and the rect of
the rt "rect".  Could you call them rtcRect and rtRect, or
rtcBounds and rtBounds?

r=dbaron with that

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

try push in comment 5

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ad44003f7be
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d9cfa37df70
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b496503037
https://hg.mozilla.org/integration/mozilla-inbound/rev/426d022857ff

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ad44003f7be
https://hg.mozilla.org/mozilla-central/rev/2d9cfa37df70
https://hg.mozilla.org/mozilla-central/rev/a6b496503037
https://hg.mozilla.org/mozilla-central/rev/426d022857ff