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.

(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.

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 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 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 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.)

(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.

(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.)

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?

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 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...

Jonathan, could you review my patches here?

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 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 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 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.

(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.

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

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