1108429 - Implement default ruby alignment (ruby-align: space-around)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

I plan to implement the space-around first, and ruby-align later.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - Add ruby utils for reserving isize of ruby boxes

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - Reserve isize for ruby boxes

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - Add flag to indicate the existance of ruby

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - Handle isize reservation in sync annotation rect

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 5 - Enable justification computation when there is ruby

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 6 - Record inner justification opportunities for each frame

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 - Modify jusitication computation to take ruby into account

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 - Implement default ruby alignment

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8538363 [details] [diff] [review]
patch 1 - Add ruby utils for reserving isize of ruby boxes

Review of attachment 8538363 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/RubyUtils.h
@@ +24,5 @@
> +  }
> +
> +  static void SetReservedISize(nsIFrame* aFrame, nscoord aISize);
> +  static void ClearReservedISize(nsIFrame* aFrame);
> +  static nscoord GetReservedISize(nsIFrame* aFrame);

You need comments explaining what the "Reserved ISize" is.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8538371 [details] [diff] [review]
patch 7 - Modify jusitication computation to take ruby into account

Review of attachment 8538371 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsLineLayout.cpp
@@ +2490,2 @@
>    PerFrameData* mLastParticipant;
> +  bool mAcrossRubyBaseBound;

Please document what this means.

@@ +2569,5 @@
>          }
>        }
>  
>        aState.mLastParticipant = pfd;
> +      aState.mAcrossRubyBaseBound = isRubyBase;

Why are we setting this in two places?

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 8538363 [details] [diff] [review]
> patch 1 - Add ruby utils for reserving isize of ruby boxes
> 
> Review of attachment 8538363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/RubyUtils.h
> @@ +24,5 @@
> > +  }
> > +
> > +  static void SetReservedISize(nsIFrame* aFrame, nscoord aISize);
> > +  static void ClearReservedISize(nsIFrame* aFrame);
> > +  static nscoord GetReservedISize(nsIFrame* aFrame);
> 
> You need comments explaining what the "Reserved ISize" is.

OK, so "Reserved ISize" is the difference between the isize a frame should be and what it is currently. The difference is reserved so that we can expand the frames to the expected isize later. The isize has been reserved in line layout via AdvanceICoord.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 8538371 [details] [diff] [review]
> patch 7 - Modify jusitication computation to take ruby into account
> 
> Review of attachment 8538371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsLineLayout.cpp
> @@ +2490,2 @@
> >    PerFrameData* mLastParticipant;
> > +  bool mAcrossRubyBaseBound;
> 
> Please document what this means.

It means, we are going across a boundary of a ruby base, that is, we are entering one, leaving one, or both.

> @@ +2569,5 @@
> >          }
> >        }
> >  
> >        aState.mLastParticipant = pfd;
> > +      aState.mAcrossRubyBaseBound = isRubyBase;
> 
> Why are we setting this in two places?

It is set here because we are leaving the ruby base.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Do you think my comments above have been clear enough for understanding the patches? If so, I'll update the patches to include them.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #11)
> OK, so "Reserved ISize" is the difference between the isize a frame should
> be and what it is currently. The difference is reserved so that we can
> expand the frames to the expected isize later. The isize has been reserved
> in line layout via AdvanceICoord.

This isn't detailed enough. You need to say what causes the difference and what exactly you mean by "now" and "later".

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8538371 [details] [diff] [review]
patch 7 - Modify jusitication computation to take ruby into account

Review of attachment 8538371 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsLineLayout.cpp
@@ +2490,2 @@
>    PerFrameData* mLastParticipant;
> +  bool mAcrossRubyBaseBound;

OK, call it "mCrossingRubyBaseBoundary".

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Ruby boxes usually need to synchronize their isize among different levels, which means they might have a larger isize than what themselves require. When these ruby boxes are reflowed, they are sized like a normal inline container, but the difference between this size and the isize it should be, is reserved in line layout, and saved as "Reserved ISize" in the property of the frame.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Is comment 16 clear enough?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you give an example?

When you say "the isize it should be", how exactly is the "isize it should be" determined?

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

With some exceptions, each ruby internal box has two isizes, which are intrinsic isize and intended isize. The intrinsic isize is what a box itself needs. It is determined when the box gets reflowed.

The intended isize is what it should be as the final result. For an rb/rt box, the intended isize is the size of its ruby column. For an rbc/rtc box, the intended isize is the size of its ruby segment. The intended isize is never smaller than the intrinsic isize for any ruby box. It is initially determined when a ruby column/segment gets fully reflowed, and may be advanced when a box is expanded, e.g. when applying justification.

The difference between the intended isize and the intrinsic isize is reserved in the line layout after reflowing the box, hence it is called reserved isize here. The reserved isize is used to expand the ruby boxes from their intrinsic isize to the intended isize during aligning the line.

There are three exceptions for the intrinsic isize:
1. An rbc box does not have intended isize unless there is any span or collapsed annotation in its segment;
2. An rtc box have intended isize only if it is for a span or collapsed annotations;
3. If an rtc box have intended isize, its children must not have.

Is it clear enough now? I feel that the word "intrinsic isize" here might refer to another concept in our code, but I have no idea what term should be used instead.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's a lot clearer, thanks. That explanation should definitely be in a comment somewhere.

We already have intrinsic preferred and max isizes, which seem different since they don't take line breaks into account. How is your "intrinsic isize" affected by line breaking? I can't tell from your explanation.

"intended" is also too vague. Maybe we should call it "final ruby isize".

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Thanks, I'll add them. I'm going to add them to RubyUtils.h.

rb/rt boxes currently never have line break inside (bug 1105051), so the "intrinsic isize" I mentioned here should effectively be equal to the intrinsic preferred isize for them. (I assume the other one should be intrinsic min isize, not max). However, right, the "intrinsic isize" for rbc/rtc boxes is affected by line breaking. The "intrinsic isize" for them only takes the left parts into account, i.e. any pushed frames are not included.

Comment 22

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

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #21)
> intrinsic min isize, not max). However, right, the "intrinsic isize" for
> rbc/rtc boxes is affected by line breaking. The "intrinsic isize" for them
> only takes the left parts into account, i.e. any pushed frames are not
> included.

I'd prefer not to use the term "intrinsic" for things that are affected by line breaking.  Line breaking is a function of the environment (available width) outside of the element, so it's not intrinsic to the element.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - Add ruby utils for reserving isize of ruby boxes

Add the comment.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 8 - Implement default ruby alignment

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8539991 [details] [diff] [review]
patch 1 - Add ruby utils for reserving isize of ruby boxes

Review of attachment 8539991 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/RubyUtils.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +
> +/**
> + * Reserved ISize

fix this line

@@ +28,5 @@
> + *
> + * The difference between the reflowed isize and the final isize is
> + * reserved in the line layout after reflowing a box, hence it is called
> + * "Reserved ISize" here. It is used to expand the ruby boxes from their
> + * reflowed isize to the final isize during aligning the line.

"during alignment of the line"

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a339d7695b60
https://hg.mozilla.org/integration/mozilla-inbound/rev/f437602d958d
https://hg.mozilla.org/integration/mozilla-inbound/rev/daae8202f566
https://hg.mozilla.org/integration/mozilla-inbound/rev/de8839e79001
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9213948acb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/28819098bfcf
https://hg.mozilla.org/integration/mozilla-inbound/rev/e018b5e1f399
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d2322fb050

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a339d7695b60
https://hg.mozilla.org/mozilla-central/rev/f437602d958d
https://hg.mozilla.org/mozilla-central/rev/daae8202f566
https://hg.mozilla.org/mozilla-central/rev/de8839e79001
https://hg.mozilla.org/mozilla-central/rev/d9213948acb6
https://hg.mozilla.org/mozilla-central/rev/28819098bfcf
https://hg.mozilla.org/mozilla-central/rev/e018b5e1f399
https://hg.mozilla.org/mozilla-central/rev/44d2322fb050