1089431 - Meet the specification for line breaking between ruby bases

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

the spec says:
 
   Whether ruby can break between two adjacent ruby bases is controlled
   by normal line-breaking rules for the base text, exactly as if the
   ruby bases were adjacent inline boxes. (The annotations are ignored
   when determining soft wrap opportunities for the base level.)

but the current patch in bug 1052924 allows break between every two ruby bases, which should be fixed later.

Comment 1

[Jet Villegas (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #0)
> the current patch in bug 1052924 allows break between every two ruby
> bases, which should be fixed later.

Agreed that we could fix this later. Our core use cases for Asian text allow for line breaking between non-whitespace characters, so we should be OK with the behavior in the current patch.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Reblock enable-css-ruby, given the cases mentioned in www-style list by Koji Ishii are indeed important.

https://lists.w3.org/Archives/Public/www-style/2015Jan/0259.html

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - Skip ruby text containers when building text runs

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - Break between ruby bases according to line-breaking rules

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - Make line breaking inside ruby be triggered at correct time

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - Reftest

Comment 7

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

Comment on attachment 8552774 [details] [diff] [review]
patch 1 - Skip ruby text containers when building text runs

I'm going to redirect this review to roc because this code is a bit hairy and he actually knows about it.

I have one comment, though, which is that I think the comments should be clearer that we want ruby text frames to be skipped, but we want to continue across them.  (I think this is different from everything else that causes us to not continue text runs, since those things cause the text run to end.)

Comment 8

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

Comment on attachment 8552774 [details] [diff] [review]
patch 1 - Skip ruby text containers when building text runs

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

What David said.

Comment 9

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

Comment on attachment 8552775 [details] [diff] [review]
patch 2 - Break between ruby bases according to line-breaking rules

>+static gfxBreakPriority
>+LineBreakBefore(const nsHTMLReflowState& aReflowState, nsRubyBaseFrame* aFrame)

Could you explain what led to the logic in this function?  It's a
reasonably large chunk of code, and it looks like it ought to be something
that we already have somewhere.


In other words, why can't this patch reuse more existing code than it does?
(I'm also a little concerned about additional use of GetLastOptionalBreakPosition.)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #9)
> Comment on attachment 8552775 [details] [diff] [review]
> patch 2 - Break between ruby bases according to line-breaking rules
> 
> >+static gfxBreakPriority
> >+LineBreakBefore(const nsHTMLReflowState& aReflowState, nsRubyBaseFrame* aFrame)
> 
> Could you explain what led to the logic in this function?  It's a
> reasonably large chunk of code, and it looks like it ought to be something
> that we already have somewhere.
> 
> In other words, why can't this patch reuse more existing code than it does?
> (I'm also a little concerned about additional use of
> GetLastOptionalBreakPosition.)

It is the logic simplified from gfxTextRun::BreakAndMeasureText. I thought about reusing code in that part, but didn't figure out a good way to do so. Actually, that function handles one case which is not handled here, which is the hyphen break. For this case, that function unconditionally requires a PropertyProvider. And initializing a PropertyProvider outside the text frame seems to make it unnecessarily more complicated.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In addition, gfxTextRun::BreakAndMeasureText seems to have only one caller. I guess we can remove its default values and make many of its statement unconditional.)

Comment 12

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

What about, instead of reusing the code by calling it directly, reusing it by detecting what the child of the ruby base container does (i.e., whether it does a break-before) and breaking to match?

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

I thought about doing so as well. But it seems that we cannot let the child break itself, because before we return to the rbc reflow code, it is unknown whether we can break. There might be cases like spans, nested ruby, for example, which is hard to tell the descendents.

Comment 14

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

Comment on attachment 8552775 [details] [diff] [review]
patch 2 - Break between ruby bases according to line-breaking rules

Sorry, I probably should have transferred these reviews to Jonathan last week...

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Jonathan, could you review my patches here?

Comment 16

[Jonathan Kew [:jfkthame]]

Sorry, I've been buried in other stuff, but I'll try to review these in the next day or two. Leaving the ni? in place for now as a reminder to myself...

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8552775 [details] [diff] [review]
patch 2 - Break between ruby bases according to line-breaking rules

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

Sorry for the delay here. I'm happy enough with this, given the discussion above. If we could clean up and refactor BreakAndMeasureText some day, such that it's possible to share more code that's currently in LineBreakBefore here, that'd be great, but I don't think it needs to block this.

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8552776 [details] [diff] [review]
patch 3 - Make line breaking inside ruby be triggered at correct time

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

::: layout/generic/nsLineLayout.h
@@ +305,5 @@
>      *aOffset = mLastOptionalBreakFrameOffset;
>      *aPriority = mLastOptionalBreakPriority;
>      return mLastOptionalBreakFrame;
>    }
> +  // Whether there is any optional break position has been recorded.

s/there is//

@@ +306,5 @@
>      *aPriority = mLastOptionalBreakPriority;
>      return mLastOptionalBreakFrame;
>    }
> +  // Whether there is any optional break position has been recorded.
> +  bool HasOptionalBreakPosition() const { return mLastOptionalBreakFrame; }

I think this would be clearer if you explicitly write mLastOptionalBreakFrame != nullptr, instead of relying on the implicit conversion.

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8552777 [details] [diff] [review]
patch 4 - Reftest

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

::: layout/reftests/css-ruby/line-breaking-1.html
@@ +11,5 @@
> +<body>
> +  <div style="width: .5em; border: 1px solid silver;">
> +    <ruby>
> +      <rb>「<rb>な<rb>に<rb>、<rb>誰<rb>？<rb>」<rb>「<rb>私<rb>です<rb>」</rb>
> +      <rtc><rt>

Why do we have a trailing <rtc> and <rt> here? These look redundant, afaics.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #19)
> Comment on attachment 8552777 [details] [diff] [review]
> patch 4 - Reftest
> 
> Review of attachment 8552777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/css-ruby/line-breaking-1.html
> @@ +11,5 @@
> > +<body>
> > +  <div style="width: .5em; border: 1px solid silver;">
> > +    <ruby>
> > +      <rb>「<rb>な<rb>に<rb>、<rb>誰<rb>？<rb>」<rb>「<rb>私<rb>です<rb>」</rb>
> > +      <rtc><rt>
> 
> Why do we have a trailing <rtc> and <rt> here? These look redundant, afaics.

It is meant to test whether RTCs are correctly excluded from the text run of the base. Maybe I should write it out clearly.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27ea871e6e5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/eada1a2a1b28
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72b4bc2eddb
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e692be90a5

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27ea871e6e5c
https://hg.mozilla.org/mozilla-central/rev/eada1a2a1b28
https://hg.mozilla.org/mozilla-central/rev/e72b4bc2eddb
https://hg.mozilla.org/mozilla-central/rev/07e692be90a5