1098272 - Suppress line breaks inside ruby boxes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

The CSS Ruby spec proposed that forced line breaks inside ruby annotations shall be suppressed.

I feel that the line breaks should also be suppressed in ruby bases, at least currently, since my current implementation in bug 1052924 cannot properly handle line breaks inside ruby base. We may fix it later.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - Suppress line breaks inside ruby

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - Assert no line break inside ruby

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - Move initial line break check code

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - Remove an unused parameter

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 5 - Move final line break check code

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 6 - Remove break before check in ruby segment

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 7 - Check whether line break is allowed inside ruby

Comment 8

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

Comment on attachment 8539919 [details] [diff] [review]
patch 1 - Suppress line breaks inside ruby

Hmmm.  The existing IsDirectlyInsideRuby() /
NS_STYLE_IS_DIRECTLY_INSIDE_RUBY isn't a great name.  "directly" implies
that it's the child of the ruby, not any descendant.  I'd prefer
IsInlineDescendantOfRuby or maybe just IsInsideRuby.  Though fixing
that should probably happen elsewhere.

I'm a little worried about whether this interacts with any of our text
run caching.

Other than that, I think this looks good, but I'd like roc to have a
look, particularly for the caching issue.

Comment 9

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

Comment on attachment 8539930 [details] [diff] [review]
patch 2 - Assert no line break inside ruby

I guess we should find out through fuzzing if this is problematic.

In case this is wrong, does the text frame code look like it will safely handle having reported a break status but not having a continuation created?  (I'd be more worried about text frames than inline frames.)

Comment 10

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

Comment on attachment 8539935 [details] [diff] [review]
patch 6 - Remove break before check in ruby segment

Does this mean we'll change from knowing we need a break in the first pass, to waiting for the second pass?  Is that desirable f

Comment 11

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

Comment on attachment 8539936 [details] [diff] [review]
patch 7 - Check whether line break is allowed inside ruby

>+  // Allow line break between ruby bases when white-space allows,
>+  // we are not inside a nested ruby, and there is no span.
>+  bool allowLineBreak = !inNestedRuby && StyleText()->WhiteSpaceCanWrap(this);
>+  bool allowInitialLineBreak = allowLineBreak;
>+  if (!GetPrevInFlow()) {
>+    allowInitialLineBreak = !inNestedRuby &&
>+      parent->StyleText()->WhiteSpaceCanWrap(parent);
>+  }

I think I see the logic here, at least in terms of checking the element's own 'white-space' vs. the parent's 'white-space'.

That said, I'm not sure that checking 'white-space' really makes sense here -- 'white-space' should affect what happens when there's actually whitespace.  Ruby markup shouldn't introduce new break opportunities where the surroundings and bases weren't already breakable; it should just allow breaking in some (but not all) cases where there would be breaking of the surroundings/bases without the ruby markup.


I guess I might be ok with doing this for now, but I don't think this fixes bug 1089431.  (I'm actually not even sure it makes progress towards it.)

Was there a particular reason you wanted this patch now?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #11)
> Comment on attachment 8539936 [details] [diff] [review]
> patch 7 - Check whether line break is allowed inside ruby
> 
> >+  // Allow line break between ruby bases when white-space allows,
> >+  // we are not inside a nested ruby, and there is no span.
> >+  bool allowLineBreak = !inNestedRuby && StyleText()->WhiteSpaceCanWrap(this);
> >+  bool allowInitialLineBreak = allowLineBreak;
> >+  if (!GetPrevInFlow()) {
> >+    allowInitialLineBreak = !inNestedRuby &&
> >+      parent->StyleText()->WhiteSpaceCanWrap(parent);
> >+  }
> 
> I think I see the logic here, at least in terms of checking the element's
> own 'white-space' vs. the parent's 'white-space'.
> 
> That said, I'm not sure that checking 'white-space' really makes sense here
> -- 'white-space' should affect what happens when there's actually
> whitespace.  Ruby markup shouldn't introduce new break opportunities where
> the surroundings and bases weren't already breakable; it should just allow
> breaking in some (but not all) cases where there would be breaking of the
> surroundings/bases without the ruby markup.
> 
> 
> I guess I might be ok with doing this for now, but I don't think this fixes
> bug 1089431.  (I'm actually not even sure it makes progress towards it.)
> 
> Was there a particular reason you wanted this patch now?

The most important reason is that, without checking nested ruby, the nested ruby could break the assertion in patch 2. White-space is checked incidentally.

About the surrounding things, I feel it is not trivial to implement, but I don't think we need to care much about that for three reasons:
1. there's no use case that ruby should not be break inside or outside because of surroundings
2. annotations could make ruby base incontinuous, which makes it less sense to respect the surroundings
3. neither trident nor webkit do this

Given these reasons, I suggest we at least make bug 1089431 not block enable-css-ruby. Or do you have any advice about fixing that bug?

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #10)
> Comment on attachment 8539935 [details] [diff] [review]
> patch 6 - Remove break before check in ruby segment
> 
> Does this mean we'll change from knowing we need a break in the first pass,
> to waiting for the second pass?  Is that desirable f

I think so. After rethinking about this patch, I do not have a strong reason to do this. I think I did this mostly because it simplifies the logic when I was changing the code. If you don't like this, I can revert it and try another way.

It might be even worse that, in the current code, the second pass won't happen until the whole ruby is reflowed, because we only record optional break positions in the first pass. I guess it won't be a big problem, though.

Comment 14

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

Comment on attachment 8539936 [details] [diff] [review]
patch 7 - Check whether line break is allowed inside ruby

OK, but at least remove the comment about fixing bug 1089431.

Comment 15

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

Is it best to just skip patch 6, then?

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

OK, I'm fine with that.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #9)
> Comment on attachment 8539930 [details] [diff] [review]
> patch 2 - Assert no line break inside ruby
> 
> I guess we should find out through fuzzing if this is problematic.
> 
> In case this is wrong, does the text frame code look like it will safely
> handle having reported a break status but not having a continuation created?
> (I'd be more worried about text frames than inline frames.)

I'm not sure about that. But even if a continuation is created, it will live in the overflow frame list of rb/rt frame, because we won't create continuation for rb/rt. So I guess it would simply make them disappear without causing further problem (like infinite loop before this patch).

Comment 18

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

Comment on attachment 8539919 [details] [diff] [review]
patch 1 - Suppress line breaks inside ruby

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

::: gfx/thebes/gfxTextRun.h
@@ +307,5 @@
>                                   gfxFloat *aAdvanceWidthDelta,
>                                   gfxContext *aRefContext);
>  
> +    enum SuppressBreak {
> +      eSuppressNoBreak,

Call this eNoSuppressBreak

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a73f9db3885
https://hg.mozilla.org/integration/mozilla-inbound/rev/65952ac62ec9
https://hg.mozilla.org/integration/mozilla-inbound/rev/454264086d2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/abcc7ed1585b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b61276007
https://hg.mozilla.org/integration/mozilla-inbound/rev/aca891c01c88

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a73f9db3885
https://hg.mozilla.org/mozilla-central/rev/65952ac62ec9
https://hg.mozilla.org/mozilla-central/rev/454264086d2d
https://hg.mozilla.org/mozilla-central/rev/abcc7ed1585b
https://hg.mozilla.org/mozilla-central/rev/b00b61276007
https://hg.mozilla.org/mozilla-central/rev/aca891c01c88