1267515 - Implement better support of nested ruby

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

I'm not sure whether I fully understand the text in the spec about this, but it seems our current behavior that annotations overlap each other for nested ruby is clearly undesired and we may want to fix it.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1267515 part 1 - Refactor ruby leadings into a helper RubyBlockLeadings struct.

Review commit: https://reviewboard.mozilla.org/r/91814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91814/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1267515 part 2 - Place ruby annotations outside annotations from ruby descendants.

Review commit: https://reviewboard.mozilla.org/r/91816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91816/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1267515 part 3 - Add reftest for nested ruby.

Review commit: https://reviewboard.mozilla.org/r/91818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91818/

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8809218 [details]
Bug 1267515 part 3 - Add reftest for nested ruby.

https://reviewboard.mozilla.org/r/91818/#review92474

The test looks fine; r=me, with one thing to consider:

::: layout/reftests/w3c-css/submitted/ruby/reftest.list:16
(Diff revision 1)
>  == ruby-text-combine-upright-001b.html ruby-text-combine-upright-001-ref.html
>  == ruby-text-combine-upright-002a.html ruby-text-combine-upright-002-ref.html
>  == ruby-text-combine-upright-002b.html ruby-text-combine-upright-002-ref.html
> +
> +# Tests for nested ruby
> +== nested-ruby-pairing-001.html nested-ruby-pairing-001-ref.html

You might consider naming this test like "ruby-nested-pairing" instead of "nested-ruby-pairing", for consistency with other tests here (which all start with the word "ruby").

(If you make that change, do remember to update the reference file's name inside of the <link rel="match"> attribute as well.)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8809216 [details]
Bug 1267515 part 1 - Refactor ruby leadings into a helper RubyBlockLeadings struct.

https://reviewboard.mozilla.org/r/91814/#review92490

Commit message nit:
>  Bug 1267515 part 1 - Add RubyBlockLeadings type for leadings from ruby. r?dholbert

Perhaps rewrite as:
  "Refactor ruby leadings into a helper RubyBlockLeadings struct"
...or something like that, to make it clearer that this commit is purely a refactoring (encapsulating already-existing data/code), and isn't intended to change behavior at all.

::: layout/generic/nsRubyFrame.h:63
(Diff revision 1)
> -  // The leading required to put the annotations.
> +  // The leadings required to put the annotations.
>    // They are not initialized until the first reflow.
> -  nscoord mBStartLeading;
> +  mozilla::RubyBlockLeadings mLeadings;

The "not initialized until the first reflow" comment is now less correct than it was previously.

(When these were plain nscoord values, they were truly uninitialized. But now, they'll be immediately initialized to 0, and then given a meaningful value later on.)

Maybe rewrite that line as: "These are dummy-initialized to 0, and get meaningful values at first reflow." (And maybe rewrap the first few words up to the first line, so that everything still fits on 2 lines total here?)

 ...or something like that?

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8809216 [details]
Bug 1267515 part 1 - Refactor ruby leadings into a helper RubyBlockLeadings struct.

https://reviewboard.mozilla.org/r/91814/#review92492

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8809217 [details]
Bug 1267515 part 2 - Place ruby annotations outside annotations from ruby descendants.

https://reviewboard.mozilla.org/r/91816/#review92494

Several typos in commit message:
> Bug 1267515 part 2 - Place ruby anontations outside anontations from ruby descendents. r?dholbert 

1. anontations --> annotations
(This misspelling happens twice in the commit message -- please fix both)

2. descendents --> descendants
(It seems both are technically acceptable, but the "ants" spelling is much more common accepted, and by far the more common usage in our codebase (~4000 vs ~80). And firefox's built-in spell-checker only accepts the "ants" spelling, FWIW.)

::: layout/generic/nsRubyBaseContainerFrame.h:58
(Diff revision 1)
> +  void UpdateDescendentLeadings(const mozilla::RubyBlockLeadings& aLeadings) {
> +    mDescendentLeadings.Update(aLeadings);
> +  }
> +  mozilla::RubyBlockLeadings GetDescendentLeadings() const {

As noted for the commit message, s/Descendent/Descendant/ (changing to "ant") throughout this patch, please. (both functions here, plus the member-variable name)

::: layout/generic/nsRubyBaseContainerFrame.h:91
(Diff revision 1)
>                       mozilla::RubyColumn& aColumn,
>                       bool& aIsComplete);
>  
>    nscoord mBaseline;
> +
> +  // Leading produced by descendent ruby annotations.

(This comment needs s/ent/ant/, as well)

::: layout/generic/nsRubyFrame.cpp:96
(Diff revision 1)
> +  for (nsIFrame* frame = aFrame->GetParent();
> +       frame && frame->IsFrameOfType(nsIFrame::eLineParticipant);
> +       frame = frame->GetParent()) {
> +    if (frame->GetType() == nsGkAtoms::rubyBaseContainerFrame) {
> +      return static_cast<nsRubyBaseContainerFrame*>(frame);

Nit: Maybe s/frame/ancestor/ here?

(It's nice to avoid having variables with near-identical names ("aFrame" and "frame") in scope at the same time, when it's possible to use a more precise name for one of them.)

::: layout/generic/nsRubyFrame.cpp:178
(Diff revision 1)
>    }
>    if (boxDecorationBreakClone || NS_FRAME_IS_COMPLETE(aStatus)) {
>      aDesiredSize.ISize(lineWM) += borderPadding.IEnd(frameWM);
>    }
>  
> +  // Update descendent leadings of ancestor ruby base container.

s/ent/ant/

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8809218 [details]
Bug 1267515 part 3 - Add reftest for nested ruby.

https://reviewboard.mozilla.org/r/91818/#review92502

::: layout/reftests/w3c-css/submitted/ruby/nested-ruby-pairing-001.html:19
(Diff revision 1)
> +  <div id="test">
> +    <ruby><ruby>東<rt>とう</rt>南<rt>なん</rt></ruby><rt lang=en>Southeast</rt></ruby>の方角
> +  </div>
> +  <div id="ref">
> +    <ruby><rb>東<rb>南<rt>とう<rt>なん<rtc lang=en>Southeast</ruby>の方角
> +  </div>

One concern I'm just noticing about the reftest: 

Chrome actually renders the "test" part (the left half of the testcase, with actual nesting) *correctly* -- BUT -- they render the "ref" part (the right half, without nesting) *incorrectly*!  Which means they get the entire reference-case wrong. And so if you compare testcase vs. reference case in Chrome, the mismatching part of the testcase is actually the *correct* part. :)

Could you investigate that a bit, and
(a) file a Chrome bug
(b) Only if it's easy to do so: adjust the test so that Chrome renders the reference part correctly (Maybe they're confused by an omitted <rbc> or <rtc> wrapper or something?)

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8809218 [details]
Bug 1267515 part 3 - Add reftest for nested ruby.

https://reviewboard.mozilla.org/r/91818/#review92502

> One concern I'm just noticing about the reftest: 
> 
> Chrome actually renders the "test" part (the left half of the testcase, with actual nesting) *correctly* -- BUT -- they render the "ref" part (the right half, without nesting) *incorrectly*!  Which means they get the entire reference-case wrong. And so if you compare testcase vs. reference case in Chrome, the mismatching part of the testcase is actually the *correct* part. :)
> 
> Could you investigate that a bit, and
> (a) file a Chrome bug
> (b) Only if it's easy to do so: adjust the test so that Chrome renders the reference part correctly (Maybe they're confused by an omitted <rbc> or <rtc> wrapper or something?)

They just don't support the new ruby design in CSS Ruby 1, including <rb> and <rtc>. Blink, which inherits its ruby support from WebKit, only supports single level ruby annotation per <ruby> tag, while the spec describes the effect using the multi-level concept. So I think this test itself is correct and it is expected that it doesn't render the reference correctly.

One of the editors of the CSS Ruby spec is an layout implementor of Blink, so I believe the lack of support of that is well acknowledged there, thus I don't think we need to file a Chrome bug for that.

Comment 10

[Daniel Holbert [:dholbert]]

OK, fair enough.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8809216 [details]
Bug 1267515 part 1 - Refactor ruby leadings into a helper RubyBlockLeadings struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91814/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8809217 [details]
Bug 1267515 part 2 - Place ruby annotations outside annotations from ruby descendants.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91816/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8809218 [details]
Bug 1267515 part 3 - Add reftest for nested ruby.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91818/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10b8471d7d45
part 1 - Refactor ruby leadings into a helper RubyBlockLeadings struct. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/ca6ff07941fc
part 2 - Place ruby annotations outside annotations from ruby descendants. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a8e375f76713
part 3 - Add reftest for nested ruby. r=dholbert

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10b8471d7d45
https://hg.mozilla.org/mozilla-central/rev/ca6ff07941fc
https://hg.mozilla.org/mozilla-central/rev/a8e375f76713

Comment 16

[Chris Mills [:cmills]]

I'm not really sure what needs to be modified in the docs as a result of this bug fix. Is there any change for web developers? Or is it just an implementation improvement?

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

Probably just an implementation improvement. It makes nested ruby rendered in the same way as other browsers.

Comment 18

[Chris Mills [:cmills]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Probably just an implementation improvement. It makes nested ruby rendered
> in the same way as other browsers.

Cool, thanks.