1069519 - ruby should sometimes influence line height calculation

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

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

As described in the css-ruby spec, ruby should sometimes (although rarely, given reasonable line-height styles) influence line height calculation.  We need to implement this.

http://dev.w3.org/csswg/css-ruby/#line-height

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8529968 [details] [diff] [review]
patch

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

::: layout/generic/nsLineLayout.cpp
@@ +1740,5 @@
>      psd->mBStartLeading = leading / 2;
>      psd->mBEndLeading = leading - psd->mBStartLeading;
>      psd->mLogicalBSize = logicalBSize;
> +    if (spanFrame->GetType() == nsGkAtoms::rubyFrame) {
> +      // We may need to extend leadings here for ruby annotations.

Please add the URL for this chunk of spec to this comment or another comment around here, to ground this in the spec. (The relevant URL would be http://dev.w3.org/csswg/css-ruby/#line-height , I think.)

@@ +1748,5 @@
> +      nscoord deltaLeading = startLeading + endLeading - leading;
> +      if (deltaLeading > 0) {
> +        // If the total leading is not wide enough for ruby annotations,
> +        // extend the side which is not enough. If both sides are not
> +        // wide enough, replace the leadings with the requested values.

Looks like the spec doesn't consider the "both sides are not wide enough" scenario that you mention here. The spec just says "additional leading is added on the appropriate side" -- and notably, "side" is singular.  Could you email www-style about this, if it needs fixing?

Also, I think there's a case that's not handled by the code here -- it looks like deltaLeading could be <= 0 here (making us skip this whole clause), but if startLeading and endLeading are lopsided (one being much bigger than the other), then either psd->mBEndLeading or psd->mBStartLeading might still need adjusting.  (since those psd-> variables are each initialized to each be 1/2 the leading)  The way the patch has things right now, there's a weird discontinuity when deltaLeading crosses 0 in this kind of situation.

::: layout/generic/nsRubyFrame.cpp
@@ +428,5 @@
>    }
> +
> +  // Set block leadings of the base container
> +  // Because we only need the differences here, it is not necessary to
> +  // have a correct container width here. Providing a zero is enough.

s/here/in the LogicalRects here/

(to clarify where exactly we technically need -- but are ignoring -- the container-width)

@@ +430,5 @@
> +  // Set block leadings of the base container
> +  // Because we only need the differences here, it is not necessary to
> +  // have a correct container width here. Providing a zero is enough.
> +  LogicalRect contentRect(lineWM, baseRect, 0);
> +  LogicalRect logicalRect(lineWM, offsetRect, 0);

Could these be named something clearer?  "LogicalRect logicalRect" is a bit vague.

If these are really just logical versions of baseRect and offsetRect, maybe just name them e.g. "lBaseRect" and "lOffsetRect"?

@@ +434,5 @@
> +  LogicalRect logicalRect(lineWM, offsetRect, 0);
> +  nscoord startLeading =
> +    contentRect.BStart(lineWM) - logicalRect.BStart(lineWM);
> +  nscoord endLeading = logicalRect.BEnd(lineWM) - contentRect.BEnd(lineWM);
> +  MOZ_ASSERT(startLeading >= 0 && endLeading >= 0);

Seems like this assertion could be made to fail using a sufficiently-humongous font size -- specifically, a humongous ruby annotation -- larger than nscoord_MAX -- could make these "leading" values huge enough to wrap into negative territory.

Given that, this assertion definitely shouldn't be fatal (so, not MOZ_ASSERT).  For now, probably best to make this NS_ASSERTION, so that we'll catch failures in test-suites, and we'll have a chance of noticing it in local debug builds, without crashing. Eventually, this should use the flag that's been proposed in bug 765861.

::: layout/generic/nsRubyFrame.h
@@ +76,5 @@
>  
>    nscoord mBaseline;
> +
> +  nscoord mBStartLeading;
> +  nscoord mBEndLeading;

Add a line of documentation to explain what these are for.

(Also: It looks like they and mBaseline aren't initialized at all until our first reflow; maybe worth explicitly mentioning that.)

::: layout/reftests/css-ruby/reftest.list
@@ +3,5 @@
>  == ruby-whitespace-1.html ruby-whitespace-1-ref.html
>  == ruby-whitespace-2.html ruby-whitespace-2-ref.html
>  != ruby-reflow-1-opaqueruby.html ruby-reflow-1-noruby.html
>  == ruby-reflow-1-transparentruby.html ruby-reflow-1-noruby.html
> +!= line-height-transparentannotation.html line-height-noannotation.html

!= reftests aren't very robust. Can you add a "==" reftest, as well?

(Also: ideally, your added test files should have a "-1" suffix, so that you can add new variants of them without having to rename the existing files.)

::: layout/reftests/css-ruby/ruby-reflow-1-transparentruby.html
@@ +6,5 @@
>  <link rel="help" href="http://www.w3.org/TR/2014/WD-css-ruby-1-20140805/">
>  <meta name="assert" content="Test checks that ruby bases are reflowed.">
>  <meta charset="UTF-8">
>  <style>
> +body { line-height: 2em; }

[comments after this point apply to all of the "ruby-reflow-1" reftest files tweaked in this patch]

This "2em" here seems a bit magic. I'm guessing this is just to make sure there's enough room for the ruby annotation, without relying on the auto-line-height-increasing code from this patch, right?

If so, a few nits:
 - We should probably be setting 'line-height' on ruby, not on body.  At least, the spec text about this line-height stuff is about "the line-height specified on the ruby container". 
 - We should probably be setting it to "2", not "2em". (The preferred way of setting line-height is a unitless number, which is interpreted as a multiple of the font-size. See https://developer.mozilla.org/en-US/docs/Web/CSS/line-height )

@@ +15,2 @@
>  rbc  { display: ruby-base-container; }
> +rtc  { display: ruby-text-container; line-height: 0; color: transparent; }

Why the "line-height:0" here? (when we have "line-height:2em" on the parent, and "line-height:normal" on the child.)

This seems strange & magical.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8529968 [details] [diff] [review]
> patch
> 
> Review of attachment 8529968 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1748,5 @@
> > +      nscoord deltaLeading = startLeading + endLeading - leading;
> > +      if (deltaLeading > 0) {
> > +        // If the total leading is not wide enough for ruby annotations,
> > +        // extend the side which is not enough. If both sides are not
> > +        // wide enough, replace the leadings with the requested values.
> 
> Looks like the spec doesn't consider the "both sides are not wide enough"
> scenario that you mention here. The spec just says "additional leading is
> added on the appropriate side" -- and notably, "side" is singular.  Could
> you email www-style about this, if it needs fixing?

OK.

> Also, I think there's a case that's not handled by the code here -- it looks
> like deltaLeading could be <= 0 here (making us skip this whole clause), but
> if startLeading and endLeading are lopsided (one being much bigger than the
> other), then either psd->mBEndLeading or psd->mBStartLeading might still
> need adjusting.  (since those psd-> variables are each initialized to each
> be 1/2 the leading)  The way the patch has things right now, there's a weird
> discontinuity when deltaLeading crosses 0 in this kind of situation.

I think this is intended. Normally, the annotations share both start leading and end leading (as shown by Figure 6). So if the total leading is enough for all annotations, we do not need to adjust any leading.

> ::: layout/reftests/css-ruby/ruby-reflow-1-transparentruby.html
> @@ +6,5 @@
> >  <link rel="help" href="http://www.w3.org/TR/2014/WD-css-ruby-1-20140805/">
> >  <meta name="assert" content="Test checks that ruby bases are reflowed.">
> >  <meta charset="UTF-8">
> >  <style>
> > +body { line-height: 2em; }
> 
> [comments after this point apply to all of the "ruby-reflow-1" reftest files
> tweaked in this patch]
> 
> This "2em" here seems a bit magic. I'm guessing this is just to make sure
> there's enough room for the ruby annotation, without relying on the
> auto-line-height-increasing code from this patch, right?

Right.

> If so, a few nits:
>  - We should probably be setting 'line-height' on ruby, not on body.  At
> least, the spec text about this line-height stuff is about "the line-height
> specified on the ruby container". 

I don't think so. The spec says:

    In order to ensure *consistent spacing* of lines, documents with ruby typically ensure that the line-height is large enough ..."

which means the whole document should have the same line-height which takes ruby into account.

>  - We should probably be setting it to "2", not "2em". (The preferred way of
> setting line-height is a unitless number, which is interpreted as a multiple
> of the font-size. See
> https://developer.mozilla.org/en-US/docs/Web/CSS/line-height )

OK.

> 
> @@ +15,2 @@
> >  rbc  { display: ruby-base-container; }
> > +rtc  { display: ruby-text-container; line-height: 0; color: transparent; }
> 
> Why the "line-height:0" here? (when we have "line-height:2em" on the parent,
> and "line-height:normal" on the child.)
> 
> This seems strange & magical.

This would be removed when either bug 1111463 or bug 1115249 is landed.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #4)
> > The way the patch has things right now, there's a weird
> > discontinuity when deltaLeading crosses 0 in this kind of situation.
> 
> I think this is intended. Normally, the annotations share both start leading
> and end leading (as shown by Figure 6). So if the total leading is enough
> for all annotations, we do not need to adjust any leading.

You're right, though that's not quite what I was getting at -- but I think I was misunderstanding, so I think this is OK after all.

I was imagining a scenario where a small decrease to the "line-height" property could make us go from skipping your "deltaLeading > 0" clause (and accepting the default half-on-top, half-on-bottom distribution), to entering that clause (and adopting the requested start & end leadings). And I was visualizing this causing a noticable discontinuity for cases where there are lopsided ruby annotations (e.g. small on top, large on bottom).  But I'm now realizing that in such a lopsided case, there wouldn't really be any discontinuity -- when deltaLeading crosses 0, we'd end up assigning all of the deltaLeading to the larger edge (e.g. the bottom), and we'd go from making no adjustment to making a miniscule adjustment (only deltaLeading).

So, never mind about this discontinuity -- sorry for my confusion on that.

> > This "2em" here seems a bit magic. I'm guessing this is just to make sure
> > there's enough room for the ruby annotation, without relying on the
> > auto-line-height-increasing code from this patch, right?
> 
> Right.

OK. So, it sounds like we need some reftests that lack an explicit "line-height" (or have a small line-height) so that we *do* exercise your new auto-line-height-increasing code.

(The current tests won't visit the "deltaLeading > 0" clause in VerticalAlignLines.  We need tests to exercise each of the chunks of that clause -- the increase-startLeading case, the increase-endLeading case, and the increase-both case.)

(We also need to test the no-increase-needed-because-line-height-is-large-enough case; the patch's existing tests sort of cover that with the large "line-height", though they're != instead of == so they don't really ensure that we render correctly.)

> I don't think so. The spec says:
>     In order to ensure *consistent spacing* of lines, documents with ruby
> typically ensure that the line-height is large enough ..."
>
> which means the whole document should have the same line-height which takes
> ruby into account.

Ah, thanks for clarifying that. I hadn't realized ruby documents tended to use a consistent line-height throughout, even in non-annotated sections. Given that, I agree that setting line-height on <body> makes sense.

> > Why the "line-height:0" here? (when we have "line-height:2em" on the parent,
> > and "line-height:normal" on the child.)
> > 
> > This seems strange & magical.
> 
> This would be removed when either bug 1111463 or bug 1115249 is landed.

OK, thanks. If you haven't already, might be worth filing a bug (depending on one or both of those bugs), to make sure we eventually clean this up in reftests. e.g. "Remove line-height: 0 from rtc style rules in ruby reftests, once we don't need it anymore")  Otherwise, this seems likely to stick around & needlessly propagate via copypaste into newer reftests.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8544994 [details] [diff] [review]
patch

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

::: layout/generic/nsRubyFrame.cpp
@@ +383,5 @@
> +
> +  // Set block leadings of the base container
> +  LogicalMargin leadings(lineWM, offsetRect - baseRect);
> +  NS_ASSERTION(leadings.BStart(lineWM) >= 0 && leadings.BEnd(lineWM) >= 0,
> +               "unexpected extra leading size");

I don't understand this assertion message.  I think perhaps it should really say something like:
  "leadings should be non-negative (because adding ruby annotations can only increase our footprint)"
or something like that.

::: layout/reftests/css-ruby/reftest.list
@@ +19,5 @@
>  == inlinize-blocks-2.html inlinize-blocks-2-ref.html
>  == inlinize-blocks-3.html inlinize-blocks-3-ref.html
>  == inlinize-blocks-4.html inlinize-blocks-4-ref.html
>  == inlinize-blocks-5.html inlinize-blocks-5-ref.html
> +== line-height-1.html line-height-1-ref.html

It looks like this added test covers an annotation that triggers extra top-leading.

As noted in the second chunk of comment 5, we really should be testing the other possibilities as well. (requiring extra bottom-leading; and requiring Npx extra on top & Mpx on bottom). (I suspect the "line-height-is-tall-enough, no extra leading needed" case is covered by existing tests.)

I imagine we should be able to detect whether we've added bottom-leading by having a line of ruby text, followed by a <br> and a line of normal text, right?

::: layout/reftests/css-ruby/ruby-reflow-1-noruby.html
@@ +5,5 @@
>  <link rel="author" title="Susanna Bowen" href="mailto:[e-mail]">
>  <link rel="help" href="http://www.w3.org/TR/2014/WD-css-ruby-1-20140805/">
>  <meta name="assert" content="Test checks that ruby bases are reflowed.">
>  <meta charset="UTF-8">
> +<link rel="stylesheet" href="common.css">

(This is replacing "ruby { display: ruby; } rb   { display: ruby-base; white-space: nowrap; }" etc.

Most of the changes to existing test-files in this patch here seem to be just replacing ruby boilerplate CSS with a link to common.css.  Is that actually relevant to the rest of this patch?  Seems like it'd be better to split that cleanup into its own patch.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - use common.css in previous ruby reftests

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - implement line height calculation

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - reftests

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8545064 [details] [diff] [review]
patch 1 - use common.css in previous ruby reftests

Thanks for splitting this part out! r=me

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8545068 [details] [diff] [review]
patch 3 - reftests

The first part of comment 7 still needs to be addressed.

r=me with that

Comment 13

[Daniel Holbert [:dholbert]]

(er, sorry -- meant to paste that into the review window for patch 2, not patch 3. Anyway, r=me on patch 2 with the first part of comment 7 addressed. Looking at patch 3 now.)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8545068 [details] [diff] [review]
patch 3 - reftests

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

r=me on part 3, with a comment explaining what each reference case are doing (see below)

::: layout/reftests/css-ruby/line-height-1-ref.html
@@ +9,5 @@
> +  <div id="base" style="padding-top: .5em"><span id="inline">base</span></div>
> +  <script>
> +    var height = inline.getBoundingClientRect().height + 'px';
> +    base.style.height = height;
> +    base.style.lineHeight = height;

Please add a comment to this script explaining what we're trying to do here.

e.g.
    // Our testcase has a (transparent) ruby annotation, which we expect to
    // increase the line's top leading to 0.5em. We create a reference
    // rendering by manually applying 0.5em of padding-top, and we get rid
    // of any *actual* leading by setting 'height' and 'line-height' to
    // the exact height of the text.

And similar for line-height-2-ref.html and line-height-3-ref.html (with s/top/bottom/, etc.)

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8545068 [details] [diff] [review]
> patch 3 - reftests
> 
> Review of attachment 8545068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on part 3, with a comment explaining what each reference case are doing
> (see below)
> 
> ::: layout/reftests/css-ruby/line-height-1-ref.html
> @@ +9,5 @@
> > +  <div id="base" style="padding-top: .5em"><span id="inline">base</span></div>
> > +  <script>
> > +    var height = inline.getBoundingClientRect().height + 'px';
> > +    base.style.height = height;
> > +    base.style.lineHeight = height;
> 
> Please add a comment to this script explaining what we're trying to do here.
> 
> e.g.
>     // Our testcase has a (transparent) ruby annotation, which we expect to
>     // increase the line's top leading to 0.5em. We create a reference
>     // rendering by manually applying 0.5em of padding-top, and we get rid
>     // of any *actual* leading by setting 'height' and 'line-height' to
>     // the exact height of the text.
> 
> And similar for line-height-2-ref.html and line-height-3-ref.html (with
> s/top/bottom/, etc.)

Actually they are not for getting rid of leading, but for simulate the height calculation of inline boxes. Inline boxes seem to have a height match the max height in font. However this height does not affect any other boxes during layout, and I don't see any method to get this information from CSS. But ruby layout depends on it to place the annotations. Hence I have to manually get the bounding rect of the text to make it work properly.

Comment 16

[Daniel Holbert [:dholbert]]

Ah, OK, thanks for clarifying. (Please include a brief explanation of that in the this -ref file, in lieu of my suggested text.)

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - reftests

Add utils script for the reference pages.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8545535 [details] [diff] [review]
patch 3 - reftests

Thanks -- this is clearer.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3292d545bea
https://hg.mozilla.org/integration/mozilla-inbound/rev/049151052034
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ca345a3767

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3292d545bea
https://hg.mozilla.org/mozilla-central/rev/049151052034
https://hg.mozilla.org/mozilla-central/rev/87ca345a3767