David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 10 years ago

Right now all CSS ruby is placed above the bases.  It's not clear whether the offset used is the correct offset -- e.g., whether it should consider line-height, font-size, margin/border/padding, vertical-align on either the bases or the text.  We should:
 (a) figure out and implement the correct offset behavior, and
 (b) implement the ruby-position property to allow the position to be on either side, etc.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 4 • 10 years ago

The left parts of this bug depends on patches of bug 1052924.

	Daniel Holbert [:dholbert]
	Comment 5 • 10 years ago

Comment on attachment 8506453 [details] [diff] [review]
patch 1 - Create fundamental code for ruby styles

Sorry -- I should defer to a style-system peer on this patch.  I haven't written or reviewed a "create a new style-struct" patch before, so I'm unfamiliar with this boilerplate & am not sure what sorts of issues to watch out for. (It's pretty rare that we add a new style struct, really.)  Hopefully one of dbaron, bz, or heycam have cycles to look at this?

(Sorry for causing review back-and-forth churn; I should've noticed this when jet asked me about reviewing this yesterday. I skimmed the bug/patch too quickly at that point.)

Here are two nits that I noticed when skimming the patch, though:

># HG changeset patch
># Parent 5b50dae573076049dc1728a34a37ee6c84a5373b
># User Xidorn Quan <quanxunzhen@gmail.com>
>Bug 1055665 - Create fundamental code for ruby styles.

This commit message is a bit vague. It probably wants to say:
 Bug 1055665 part 1 - Create a new style struct for ruby-related properties.

>+++ b/layout/style/nsStyleStruct.cpp
[...]
>+
>+//-----------------------
>+// nsStyleVariables
>+//
>+
>+nsStyleRuby::nsStyleRuby()
>+{
>+  MOZ_COUNT_CTOR(nsStyleRuby);
>+}

Copypaste error in that header-comment there -- it wants a s/nsStyleVariables/nsStyleRuby/. :)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 6 • 10 years ago

Comment on attachment 8506453 [details] [diff] [review]
patch 1 - Create fundamental code for ruby styles

I don't think we want a new style struct for these.  They should go within an existing style struct, probably nsStyleText or maybe nsStyleVisibility.  I don't think there's a good memory-use reason to want a separate struct.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Comment 7 • 10 years ago

(Note that if you'd included a better patch description, I'd have been able to mark that within a day by reading the description of what you'd changed.)

	Daniel Holbert [:dholbert]
	Comment 8 • 10 years ago

Comment on attachment 8506458 [details] [diff] [review]
patch 2 - Parse ruby-position

(Going ahead with the review of part 2, regardless of comment 6, because this part will mostly look the same no matter what style struct we use.)

># HG changeset patch
># Parent adaa154f5df58bb4f90a7040d7081a65a299e3de
># User Xidorn Quan <quanxunzhen@gmail.com>
>Bug 1055665 - Parse ruby-position.

This could stand to be a little more verbose. :) Maybe something more like:
Bug 1055665: Add support for parsing & computing the CSS property "ruby-position"'

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>--- a/layout/style/nsCSSKeywordList.h
>+++ b/layout/style/nsCSSKeywordList.h
>@@ -305,16 +305,17 @@ CSS_KEY(initial, initial)
> CSS_KEY(inline, inline)
> CSS_KEY(inline-axis, inline_axis)
> CSS_KEY(inline-block, inline_block)
> CSS_KEY(inline-flex, inline_flex)
> CSS_KEY(inline-grid, inline_grid)
> CSS_KEY(inline-table, inline_table)
> CSS_KEY(inset, inset)
> CSS_KEY(inside, inside)
>+/*CSS_KEY(inter-character, inter_character)*/

This commented-out code, with no explanation alongside it, is a bit cryptic (& can turn into dead & abandoned code).

So: if we're not adding support for this value yet, we probably should either:
 (A) Not add it to this list yet.
...or
 (B) Add it commented-out, *with* a XXX / TODO comment that includes a bug number for a followup bug that will uncomment it)

(Same goes for the commented-out lines about NS_STYLE_RUBY_POSITION_INTER_CHARACTER.)

>+++ b/layout/style/nsCSSParser.cpp
> bool
>+CSSParserImpl::ParseRubyPosition()
>+{
>+  nsCSSValue position;
>+  if (!ParseVariant(position, VARIANT_INHERIT, nullptr)) {
>+    nsCSSValue vertical, horizontal;
[...]
>+    position.SetPairValue(vertical, horizontal);
>+  }
>+
>+  AppendValue(eCSSProperty_ruby_position, position);
>+  return true;
>+}

Two things:
 (1) This would be easier to follow (and save on indentation) if you make the "VARIANT_INHERIT" case an early-return. Then, the bulk of the function can be de-indented, and the final "AppendValue(...position)" call would be adjacent to the place where 'position' actually got its value.  (It requires one extra "return true" statement, but it's worth that extra line, IMO)

...but more substantially:
 (2) This function almost certainly wants to use the utility-function "ParseBitmaskValues" -- that's what we generally use to parse CSS properties that we represent as bitfields, and it simplifies the "values could come in either order, and these values are mutually-exclusive" logic quite a bit.

So, this function should basically look like CSSParserImpl::ParseFontVariantEastAsian and CSSParserImpl:::ParseFontVariantLigatures. For this to work, you'll need to make sure that the INTER_CHARACTER value (currently unsupported) has its own dedicated bit -- each enum value needs its own bit, for ParseBitmaskValues to work correctly, IIRC.  (You don't necessarily need to include it in the mask right now, since it's not yet supported, but you might want to reserve a bit for it.)


Also, on the other end of the parsing/serializing/displaying pipeline: you should be using nsStyleUtil::AppendBitmaskCSSValue() to re-encode your bitmask value into a string. In particular, that's what you should be using to implement DoGetRubyPosition in nsComputedDOMStyle.cpp.  You also should be using AppendBitmaskCSSValue in nsCSSValue::AppendToString(). (It doesn't look like your patch adds any code to nsCSSValue::AppendToString right now, which I think means you'll probably fail some mochitests, because you'll take the default switch-case here: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp?rev=21282be9ad95#1187 and I believe that case tries to treat your mask-encoded value as if it were a single enum in the property-table. And that won't work, so you'll end up with "unknown", I think. So -- you need to add a new case statement (with a AppendBitmaskCSSValue() call) to that section of nsCSSValue::AppendToString, under _Enumerated.)

>@@ -9166,17 +9162,37 @@ nsRuleNode::ComputeRubyData(void* aStart
>+  // ruby-position: pair, inherit, initial
>+  const nsCSSValue* positionValue = aRuleData->ValueForRubyPosition();
>+  switch (positionValue->GetUnit()) {
>+    case eCSSUnit_Unset:
>+    case eCSSUnit_Inherit:
>+      canStoreInRuleTree = false;
>+      ruby->mPosition = parentRuby->mPosition;
>+      break;
>+    case eCSSUnit_Initial:
>+      ruby->mPosition = (NS_STYLE_RUBY_POSITION_OVER |
>+                         NS_STYLE_RUBY_POSITION_RIGHT);
>+      break;
>+    case eCSSUnit_Pair: {
>+      const nsCSSValuePair& pair = positionValue->GetPairValue();
>+      ruby->mPosition = (pair.mXValue.GetIntValue() |
>+                         pair.mYValue.GetIntValue());
>+      break;
>+    }
>+    default:
>+      NS_NOTREACHED("Unexpected value unit");
>+  }

If you switch to ParseBitmaskValues() in your parsing function, then you can replace this whole chunk with a call to "SetDiscrete" (like we have for e.g. font-variant-ligatures)

>+++ b/layout/style/nsStyleConsts.h
>+// ruby-position
>+#define NS_STYLE_RUBY_POSITION__VERTICAL_MASK   (3 << 0)
>+#define NS_STYLE_RUBY_POSITION_OVER             (1 << 0)
>+#define NS_STYLE_RUBY_POSITION_UNDER            (2 << 0)
>+//#define NS_STYLE_RUBY_POSITION_INTER_CHARACTER  (3 << 0)
>+#define NS_STYLE_RUBY_POSITION__HORIZONTAL_MASK (3 << 2)
>+#define NS_STYLE_RUBY_POSITION_RIGHT            (1 << 2)
>+#define NS_STYLE_RUBY_POSITION_LEFT             (2 << 2)

Since each value will need its own bit, for us to use ParseBitmaskValues(), this should now just be:
   (1 << 0)
   (1 << 1)
   (1 << 2)
   (1 << 3)
   (1 << 4)
instead of 1 << 0, 2 << 0, 3 << 0, etc.

And you probably don't want to define the _MASK values at all anymore -- you'll only need them in nsCSSParser.cpp now. So it's probably best to just define the masks there (in a "masks" table that gets passed to ParseBitmaskValues -- see other callers of ParseBitmaskValues for examples.)

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
> nsStyleRuby::nsStyleRuby()
>+  : mPosition(NS_STYLE_RUBY_POSITION_OVER | NS_STYLE_RUBY_POSITION_RIGHT)
> {


Let's define this initial value in nsStyleConsts.h, e.g.:

#define NS_STYLE_RUBY_POSITION_INITIAL_VAL \
  (NS_STYLE_RUBY_POSITION_OVER | NS_STYLE_RUBY_POSITION_RIGHT)

...and then use that here & in nsRuleNode.cpp (in the SetDiscrete call requested above), instead of explicitly referencing these values.

	Daniel Holbert [:dholbert]
	Comment 10 • 10 years ago

Comment on attachment 8517243 [details] [diff] [review]
patch 1 - Parse & compute ruby-position

Thanks for the updated patch! Comments below, mostly pretty minor stuff.

>diff --git a/layout/style/nsCSSKeywordList.h b/layout/style/nsCSSKeywordList.h
>--- a/layout/style/nsCSSKeywordList.h
>+++ b/layout/style/nsCSSKeywordList.h
> CSS_KEY(inside, inside)
>+/* will support in bug 1055672 */
>+/* CSS_KEY(inter-character, inter_character) */

Optional nit: I think it'd be marginally better to have the comment *on the same line* as the commented-out value, to make them more inextricably tied.

(Right now, with them on separate lines, it's possible that someone could insert new lines between them, or uncomment the CSS_KEY while forgetting to drop the "will support" line above it.  One would hope that no one would be silly enough to do that, but I've found stale comments / code lying around from time to time that suggest otherwise. :))

So I'm imagining e.g.:
// CSS_KEY(inter-character, inter_character) // TODO (see bug 1055672)

Though, what you have is fine too.

>+bool
>+CSSParserImpl::ParseRubyPosition(nsCSSValue& aValue)
>+{
>+  return ParseVariant(aValue, VARIANT_INHERIT, nullptr) ||
>+         ParseBitmaskValues(
>+             aValue, nsCSSProps::kRubyPositionKTable, gRubyPositionMask);

Please rewrap the last 2 lines here like so:
           ParseBitmaskValues(aValue, nsCSSProps::kRubyPositionKTable,
                              gRubyPositionMask);

The style you're using here (with de-indented args) should be used as a last resort, when it's the only way to keep things under 80 characters, since it's not technically allowed in the mozilla coding style guide.

(Also, FWIW, if we *did* need this style here, I think you'd want to use a 2-space indent before "aValue", not a 4-space indent.)

>+++ b/layout/style/nsCSSProps.h
>@@ -659,16 +659,17 @@ public:
>   static const KTableValue kWordWrapKTable[];
>   static const KTableValue kWritingModeKTable[];
>   static const KTableValue kHyphensKTable[];
>   static const KTableValue kCounterSystemKTable[];
>   static const KTableValue kCounterSymbolsSystemKTable[];
>   static const KTableValue kCounterRangeKTable[];
>   static const KTableValue kCounterSpeakAsKTable[];
>+  static const KTableValue kRubyPositionKTable[];
> };

nit: this list is *mostly* alphabetically sorted (with some exceptions).  So, please add this new "kRubyPositionKTable" entry at the right alphabetical point -- after kResizeKTable.

Same goes for the definition of this array (kRubyPositionKTable) in the corresponding .cpp file.

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>+nsComputedDOMStyle::DoGetRubyPosition()
>+{
>+  nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
>+  int32_t intValue = StyleText()->mRubyPosition;
>+  nsAutoString valueStr;
>+  nsStyleUtil::AppendBitmaskCSSValue(
>+      eCSSProperty_ruby_position, intValue,
>+      NS_STYLE_RUBY_POSITION_OVER, NS_STYLE_RUBY_POSITION_LEFT, valueStr);

As above: don't use the deindented-args style here, since it's not necessary. Please rewrap to something like:
  nsStyleUtil::AppendBitmaskCSSValue(eCSSProperty_ruby_position,
                                     intValue,
                                     NS_STYLE_RUBY_POSITION_OVER,
                                     NS_STYLE_RUBY_POSITION_LEFT, valueStr);

>+++ b/layout/style/nsStyleConsts.h
>@@ -1068,16 +1068,25 @@ static inline mozilla::css::Side operato
> #define NS_STYLE_COUNTER_SPEAKAS_WORDS      2
> #define NS_STYLE_COUNTER_SPEAKAS_SPELL_OUT  3
> #define NS_STYLE_COUNTER_SPEAKAS_OTHER      255 // refer to another style
> 
> // See nsStyleDisplay::mScrollBehavior
> #define NS_STYLE_SCROLL_BEHAVIOR_AUTO       0
> #define NS_STYLE_SCROLL_BEHAVIOR_SMOOTH     1
> 
>+// ruby-position
>+#define NS_STYLE_RUBY_POSITION_OVER             (1 << 0)
>+#define NS_STYLE_RUBY_POSITION_UNDER            (1 << 1)
>+#define NS_STYLE_RUBY_POSITION_INTER_CHARACTER  (1 << 2)
>+#define NS_STYLE_RUBY_POSITION_RIGHT            (1 << 3)
>+#define NS_STYLE_RUBY_POSITION_LEFT             (1 << 4)
>+#define NS_STYLE_RUBY_POSITION_INITIAL \
>+  (NS_STYLE_RUBY_POSITION_OVER | NS_STYLE_RUBY_POSITION_RIGHT)

This chunk probably belongs up a bit higher, with the other constants for nsStyleText member-vars.  Maybe put it right below the #define for NS_STYLE_UNICODE_BIDI_PLAINTEXT?

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
>@@ -1603,16 +1603,18 @@ struct nsStyleText {
> 
>   nscoord mWordSpacing;                 // [inherited]
>   nsStyleCoord  mLetterSpacing;         // [inherited] coord, normal
>   nsStyleCoord  mLineHeight;            // [inherited] coord, factor, normal
>   nsStyleCoord  mTextIndent;            // [inherited] coord, percent, calc
> 
>   nsRefPtr<nsCSSShadowArray> mTextShadow; // [inherited] nullptr in case of a zero-length
> 
>+  uint8_t mRubyPosition;                // [inherited] see nsStyleConsts.h
>+

This needs to be listed up higher in the struct, so that it has a chance of packing with other uint8_t's. Maybe put it right after mControlCharacterVisibility, perhaps? (the last uint8_t) Or somewhere else in the pile of uint8_t's, if there's a more appropriate place.

After moving it, be sure to also reorder it in the nsStyleText() no-arg constructor & the copy-constructor.

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
>+  gCSSProperties["ruby-position"] = {
>+    domProp: "rubyPosition",
>+    inherited: true,
>+    type: CSS_TYPE_LONGHAND,
>+    initial_values: [ "over right", "right over" ],
>+    other_values: [
>+      "over left", "under right", "under left",
>+      "left over", "left under", "right under"

Nit: the ordering of these other_values seems a bit strange here.

Maybe reorder these to be more clearly-methodical? (to make it clearer via a quick skim that we're exercising the whole space of values)
 e.g.   "over left", "left over",
        "under left", "left under",
        "under right", "right under"

For me at least, it's easier to quickly skim that ^ list (combined with your initial_values) and convince myself that we're exercising the whole value-space.

>+    invalid_values: [
>+      "over", "under", "left", "right", "auto", "none", "not_a_position",
>+      "over over", "over under", "left left", "left right",
>+      "over left over", "right over left"
>+    ]

Let's throw in a number, or a px-valued length, or a percentage. (or all three)

r=me with the above. Thanks!

	Daniel Holbert [:dholbert]
	Comment 11 • 10 years ago

(Apologies; I just landed some commits on beta for unrelated bug 1093316, but I accidentally copypasted *this* bug's bug# into the commit message.

That push was: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=b4e9b4dab577

I'm going to backout & re-land with correct bug numbers; just mentioning it here in case anyone accidentally ends up here when rally looking for bug 1093316.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 12 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8517243 [details] [diff] [review]
> patch 1 - Parse & compute ruby-position
> 
> >+++ b/layout/style/nsCSSProps.h
> >@@ -659,16 +659,17 @@ public:
> >   static const KTableValue kWordWrapKTable[];
> >   static const KTableValue kWritingModeKTable[];
> >   static const KTableValue kHyphensKTable[];
> >   static const KTableValue kCounterSystemKTable[];
> >   static const KTableValue kCounterSymbolsSystemKTable[];
> >   static const KTableValue kCounterRangeKTable[];
> >   static const KTableValue kCounterSpeakAsKTable[];
> >+  static const KTableValue kRubyPositionKTable[];
> > };
> 
> nit: this list is *mostly* alphabetically sorted (with some exceptions). 
> So, please add this new "kRubyPositionKTable" entry at the right
> alphabetical point -- after kResizeKTable.
> 
> Same goes for the definition of this array (kRubyPositionKTable) in the
> corresponding .cpp file.

I guess we should have a bug for reordering them, so that newcomers won't do the same thing as I did.

	Daniel Holbert [:dholbert]
	Comment 13 • 10 years ago

That's a good idea. In particular, the tables at the end of the list (kHyphensKTable, kCounter*) should be moved up into the list.

(Other parts of the list might be slightly out-of-order; I'm less concerned about those, since (a) there might be a reason for grouping things a particular way, and (b) as long as the beginnng & end of the list are correct, it'll be clearer to newcomers that they should insert in sorted order.)

Would you mind filing that? (& taking it, if you like; otherwise, we can make it a mentored bug perhaps)

	Daniel Holbert [:dholbert]
	Comment 14 • 10 years ago

(For posterity's sake: kHyphensKTable was added at the end of the list back in 2011, here:
 http://hg.mozilla.org/mozilla-central/diff/852a668d69b9/layout/style/nsCSSProps.h
Before that, the list ended with w's and was likely pretty-correctly-sorted.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 16 • 10 years ago

I move the computation of ruby-position. Now bug 1094360 no longer affects this patch.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 20 • 10 years ago

Fix that it accepts single value.

	Daniel Holbert [:dholbert]
	Comment 21 • 10 years ago

Comment on attachment 8526608 [details] [diff] [review]
patch 1 - Parse & compute ruby-position

Ah, good catch @ not wanting to accept single-values.

Two nits:

>+++ b/layout/style/nsCSSParser.cpp
>+bool
>+CSSParserImpl::ParseRubyPosition(nsCSSValue& aValue)
>+{
>+  if (ParseVariant(aValue, VARIANT_INHERIT, nullptr)) {
>+    return true;
>+  }
>+  if (!ParseBitmaskValues(aValue, nsCSSProps::kRubyPositionKTable,
>+                          gRubyPositionMask)) {
>+    return false;
>+  }
>+  auto value = aValue.GetIntValue();
>+  return (value & gRubyPositionMask[0]) && (value & gRubyPositionMask[1]);
>+}

Add a comment to explain what's going on here. e.g. something like:
  // To be valid, the specified value must include *both* a vertical keyword
  // *and* a horizontal keyword. We reject it here if either is missing.

>+++ b/layout/style/nsStyleConsts.h
> #define NS_STYLE_UNICODE_BIDI_PLAINTEXT         0x8
> 
>+// ruby-position, see nsStyleText
>+#define NS_STYLE_RUBY_POSITION_OVER             0x01
[...]
>+
> #define NS_STYLE_TABLE_LAYOUT_AUTO              0
> #define NS_STYLE_TABLE_LAYOUT_FIXED             1

This new chunk probably belongs between the NS_STYLE_HYPHENS_* constants and the NS_STYLE_TEXT_SIZE_ADJUST_* constants, here...
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleConsts.h?rev=3c993904ea4f&mark=851-852#847
...to match the ordering of member-vars in nsStyleText. (Looks like this patch is consistent about that ordering everywhere but this one spot.)

r=me with that fixed. Thanks!

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 23 • 10 years ago

I'm going to use this function for nsRubyTextContainerFrame, where there is no line box available.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 10 years ago

Comment on attachment 8528146 [details] [diff] [review]
patch 2.1 - Make nsLineLayout::VerticalAlignLine not rely on line box

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

::: layout/generic/nsLineLayout.cpp
@@ +2108,5 @@
>  
>      // (1) and (2) above
>      bool applyMinLH = !psd->mZeroEffectiveSpanBox || mHasBullet;
> +    bool isLastLine = (mGotLineBox &&
> +                       !mLineBox->IsLineWrapped() && !mLineEndsInBR);

It doesn't really matter, but I think it's more logical to say that if there's no line box, then isLastLine should always be true.

	Daniel Holbert [:dholbert]
	Comment 27 • 10 years ago

Comment on attachment 8528771 [details] [diff] [review]
patch 2.2 - Position ruby annotations and set the line size of them correctly

Sorry for the delay here - work week, travel, etc. :) Hopefully you're not too blocked on me here. (You mentioned other bugs blocking these ones).

Anyway --- I'm doing some code-reading to make sure I understand what's going on in the surrounding code & what this changes, but I do have a suggested change on the header-comment:

>diff --git a/layout/generic/nsLineLayout.h b/layout/generic/nsLineLayout.h
>--- a/layout/generic/nsLineLayout.h
>+++ b/layout/generic/nsLineLayout.h
>+  // Get the final computed line-bSize value.
>+  // It should be called after calling VerticalAlignLine.
>+  nscoord GetFinalLineBSize() const { return mFinalLineBSize; }

Two nits on this comment:
 (1) The first line doesn't really clarify anything (it's just the function name, with spaces and "the" added, basically :)).
 (2) Second line could be interpreted to mean "This **should be called**, after XYZ" (when really you mean to say "Don't call this before XYZ").

Maybe fix to say:
  // Get the final size of the line, in the block direction.
  // Do not call this until after we've called VerticalAlignLine.

(Also, optional: maybe worth adding an assertion to check assert that we're honoring this disclaimer? e.g. Add an #ifdef DEBUG line to initialize mFinalLineBSize to nscoord_MIN, and extend this getter to have
  NS_ASSERTION(mFinalLineBSize != nscoord_MIN,
               "likely-uninitialized mFinalLineBSize; did you call VerticalAlignLine?);

More feedback coming soon...

	Daniel Holbert [:dholbert]
	Comment 28 • 10 years ago

Comment on attachment 8528771 [details] [diff] [review]
patch 2.2 - Position ruby annotations and set the line size of them correctly

>The test ruby-whitespace-1 is changed because unstyled intra-annotation
>white spaces lead to incorrect height of text containers.
[...]
>+++ b/layout/reftests/css-ruby/ruby-whitespace-1.html
>-rt   { display: ruby-text; white-space: nowrap; font-size: 50%; }
>+rt   { display: ruby-text; white-space: nowrap; }
> rbc  { display: ruby-base-container; }
> rtc  { display: ruby-text-container; }
> ruby, rb, rt, rbc, rtc { unicode-bidi: isolate; }
> </style>
> </head>
> <body>

Are you sure this test-change is needed? At least on my system, this change doesn't seem to be necessary -- the test still passes, with this bug's patches applied (but leaving the test un-changed). I tried a few other percent values (10%, 90%) in the testcase & reference case, too, and it looks like it passes with those values, too. Maybe there's a platform-specific font quirk involved here?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 29 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #28)
> Comment on attachment 8528771 [details] [diff] [review]
> patch 2.2 - Position ruby annotations and set the line size of them correctly
> 
> >The test ruby-whitespace-1 is changed because unstyled intra-annotation
> >white spaces lead to incorrect height of text containers.
> [...]
> >+++ b/layout/reftests/css-ruby/ruby-whitespace-1.html
> >-rt   { display: ruby-text; white-space: nowrap; font-size: 50%; }
> >+rt   { display: ruby-text; white-space: nowrap; }
> > rbc  { display: ruby-base-container; }
> > rtc  { display: ruby-text-container; }
> > ruby, rb, rt, rbc, rtc { unicode-bidi: isolate; }
> > </style>
> > </head>
> > <body>
> 
> Are you sure this test-change is needed? At least on my system, this change
> doesn't seem to be necessary -- the test still passes, with this bug's
> patches applied (but leaving the test un-changed). I tried a few other
> percent values (10%, 90%) in the testcase & reference case, too, and it
> looks like it passes with those values, too. Maybe there's a
> platform-specific font quirk involved here?

Ah... I guess I changed line-height before, but I removed it later. Without line-height: 1em; you're right, changing font-size is not necessary. I'll remove this change.

	Daniel Holbert [:dholbert]
	Comment 30 • 10 years ago

Comment on attachment 8528771 [details] [diff] [review]
patch 2.2 - Position ruby annotations and set the line size of them correctly

r=me on patch 2.2, with the following & comments 28 / 29 addressed:

>+++ b/layout/generic/nsRubyBaseContainerFrame.cpp
>-    textContainer->SetISize(isize);
>+    lineLayouts[i]->VerticalAlignLine();
>     lineLayouts[i]->EndLineReflow();
>+    textContainer->SetLineSize(LogicalSize(
>+            lineWM, isize, lineLayouts[i]->GetFinalLineBSize()));

Too much indentation here -- I think this should just be indented 2 spaces more than the line above it.  (Really, this formulation doesn't match the moz coding style guide; it might be better to split the LogicalSize out into a helper-var.  It is nice to save on lines/variables, though, so it's also probably OK as-is, with the indentation reduced to comply with what I'm pretty sure we do elsewhere.)

>+++ b/layout/generic/nsRubyTextContainerFrame.cpp
>@@ -66,25 +66,22 @@ nsRubyTextContainerFrame::Reflow(nsPresC
>                                  nsHTMLReflowMetrics& aDesiredSize,
>                                  const nsHTMLReflowState& aReflowState,
>                                  nsReflowStatus& aStatus)
> {
[...]
>   // All rt children have already been reflowed. All we need to do is
>-  // to report complete and return the desired size.
>+  // to report complete and return the desired size provided by the
>+  // ruby base container.
[...]
>+  // ISize and BSize are provided by the ruby base container
>+  // during reflow of that container.
>   WritingMode lineWM = aReflowState.mLineLayout->GetWritingMode();
>-  WritingMode frameWM = aReflowState.GetWritingMode();
>-  LogicalMargin borderPadding = aReflowState.ComputedLogicalBorderPadding();
>-
>-  // ISize is provided by the ruby base container
>-  // during reflow of that container.
>-  aDesiredSize.ISize(lineWM) = mISize;
>-  nsLayoutUtils::SetBSizeFromFontMetrics(this, aDesiredSize, aReflowState,
>-                                         borderPadding, lineWM, frameWM);
>+  aDesiredSize.SetSize(lineWM, mLineSize);

The second comment there (about "ISize and BSize") is a bit confusing, now that the code doesn't actually mention "ISize" or "BSize" anywhere. (whereas previously, it just mentioned "ISize" and was followed by a line that set ISize().)

We could reword to just mention "size", but let's just remove that comment, I think. At this point, it's just rehashing something that your patch is already clarifying up above, about desired size being provided by the ruby base container.

	Daniel Holbert [:dholbert]
	Comment 31 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #30)
> r=me on patch 2.2, with the following & comments 28 / 29 addressed

er, "comment 27 & comment 28" :)

	Daniel Holbert [:dholbert]
	Comment 32 • 10 years ago

We need some tests here -- can you add reftests for "ruby-position", and also add reftests for how the things mentioned in comment 0 (line-height, font-size, margin/border/padding, vertical-align on either the bases or the text) do/don't affect the offset?

	Daniel Holbert [:dholbert]
	Comment 33 • 10 years ago

Comment on attachment 8528772 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position

>+++ b/layout/generic/nsRubyFrame.cpp
>@@ -373,24 +380,50 @@ nsRubyFrame::ReflowSegment(nsPresContext
>+      DebugOnly<uint8_t> horizontalPosition = rubyPosition &
>+        (NS_STYLE_RUBY_POSITION_LEFT | NS_STYLE_RUBY_POSITION_RIGHT);
>+      MOZ_ASSERT(horizontalPosition == NS_STYLE_RUBY_POSITION_LEFT ||
>+                 horizontalPosition == NS_STYLE_RUBY_POSITION_RIGHT);
[...]
>+      DebugOnly<uint8_t> verticalPosition = rubyPosition &
>+        (NS_STYLE_RUBY_POSITION_OVER | NS_STYLE_RUBY_POSITION_UNDER |
>+         NS_STYLE_RUBY_POSITION_INTER_CHARACTER);
>+      MOZ_ASSERT(verticalPosition == NS_STYLE_RUBY_POSITION_OVER ||
>+                 verticalPosition == NS_STYLE_RUBY_POSITION_UNDER ||
>+                 verticalPosition == NS_STYLE_RUBY_POSITION_INTER_CHARACTER);

This DebugOnly / MOZ_ASSERT stuff is pretty bulky & doesn't impact the logic here at all; it might be better to separate it into a helper-function, e.g. SanityCheckRubyPosition(uint8_t aRubyPosition).

>+      if (rubyPosition & NS_STYLE_RUBY_POSITION_LEFT) {
>+        x = offsetRect.X() - bsize;
>+        offsetRect.SetLeftEdge(x);
>+      } else {
>+        x = offsetRect.XMost();
>+        offsetRect.SetRightEdge(x + bsize);
>+      }

Add a comment here saying e.g.
  // writing-mode is vertical, so bsize is the annotation's *width*.
...and similar for the horizontal case. (where bsize is the annotation's height)

That helps clarify what we're doing with the "bsize" arithmetic here, & that it's in the correct axis (y with widths, x with heights)

>+      x = offsetRect.X();
>+      if (rubyPosition & NS_STYLE_RUBY_POSITION_OVER) {
>+        y = offsetRect.Y() - bsize;
>+        offsetRect.SetTopEdge(y);
>+      } else if (rubyPosition & NS_STYLE_RUBY_POSITION_UNDER) {
>+        y = offsetRect.YMost();
>+        offsetRect.SetBottomEdge(y + bsize);
>+      } else {
>+        MOZ_ASSERT_UNREACHABLE("Unsupported ruby-position");

Add a comment to this "else" clause, mentioning "inter-character" & bug 1055672.

Also: it looks like "offsetRect" ends up being unused -- is that intentional?  (It's sort of used to establish x and y, but e.g. the "offsetRect.SetTopEdge(y);" line quoted above has no effect, because we've already established "y" before that.)

If the goal here is just to establish x and y (and it looks like it is), it seems like this could perhaps be simplified to focus less on offsetRect. The old code had e.g.:
>     if (lineWM.IsVertical()) {
>-      x = lineWM.IsVerticalLR() ? -bsize : baseRect.XMost();
>-      y = baseRect.Y();
and I think we could still basically have that same two-line formulation for this clause, except that the "lineWM.IsVerticalLR()" would be replaced with "(rubyPosition & NS_STYLE_RUBY_POSITION_LEFT)", correct?

(Might be better to do it with if/else than with ternary conditions -- like you do -- but my point is that I don't see a benefit from building up offsetRect here.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 34 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #33)
> Comment on attachment 8528772 [details] [diff] [review]
> patch 3 - Position text containers according to ruby-position
> 
> Also: it looks like "offsetRect" ends up being unused -- is that
> intentional?  (It's sort of used to establish x and y, but e.g. the
> "offsetRect.SetTopEdge(y);" line quoted above has no effect, because we've
> already established "y" before that.)

It makes effect when there are multiple rtc. The spec says:

If multiple ruby annotation containers have the same ruby-position, they stack along the block axis, with lower levels of annotation closer to the base text.

	Daniel Holbert [:dholbert]
	Comment 35 • 10 years ago

Comment on attachment 8528772 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position

Ah, right -- I forgot we're in a loop there. That makes sense, thanks. Definitely include some tests that exercise the multiple-rtc-on-different-sides scenario :)

I'm just unclear about this chunk now:
>   nsRect baseRect = aBaseContainer->GetRect();
>+  // The position of the rect of the base container is relative to the
>+  // block frame containing the lines

Per IRC, I think this comment isn't really true. It's sort-of true during reflow, and then (as you pointed out) the linelayout comes along and fixes it, so our base container ends up being relative to its ruby frame, and having a x/y value that's not really connected to the block container at all.

Anyway -- really, I think we just care about the *size* of the rect here, right?  (You're already zeroing either the x or y coordinate, and the coordinate that you're *not* zeroing is already 0 in my local testcase at least).

Given that, can we just do...
  nsRect offsetRect(nsPoint(0,0), aBaseContainer->GetSize());
?

("baseMetrics" might even be a better thing to use here (instead of aBaseContainer->GetSize()), though that's got the size stored in a logical coords instead of physical coords, so it's a bit clumsier to deal with)

	Daniel Holbert [:dholbert]
	Comment 36 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #35)
> Given that, can we just do...
>   nsRect offsetRect(nsPoint(0,0), aBaseContainer->GetSize());

er, as you say on IRC, we can't do that because the inline direction might be nonzero if we have multiple rbc frames.

So, looks like we do need to use aBaseContainer->GetRect() after all, to get that inline position.

Can you adjust this comment:
>+  // The position of the rect of the base container is relative to the
>+  // block frame containing the lines, but those of text containers are
>+  // relative to the ruby container frame, hence we have to reset the
>+  // original point in block direction to have the correct offset.

to say something like:
  // We need to position our rtc frames on one side or the other of the base
  // container's rect, using a coordinate space that's relative to the ruby
  // frame. Right now, the base container's rect's block-axis position is
  // relative to the block container frame, though this will be adjusted later
  // in reflow to make it relative to the ruby frame. For now, though, we can't
  // trust its block-axis position, so we use 0 instead. (i.e. we assume that
  // the base container is adjacent to the <ruby> frame's block-start edge.)
  // XXX Do we need to add the ruby frame's border/padding to the 0 here?

That's the understanding I'm coming away with, at least; correct/adjust as-appropriate :)

	Daniel Holbert [:dholbert]
	Comment 37 • 10 years ago

(Looks like the first patch here was just bitrotted by bug 1110181, but it should still apply with fuzz.)

	Daniel Holbert [:dholbert]
	Comment 38 • 10 years ago

Comment on attachment 8528772 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position

r=me on part 3, with the two "add a comment" notes from comment 33 addressed, and with the larger comment clarified per comment 36.

(And with reftests [maybe in their own patch] per comment 32.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 39 • 10 years ago

Address the comments above, and add a test for ruby-position.

The part numbers are flattened, so it is 4 instead 3 now.

	Daniel Holbert [:dholbert]
	Comment 40 • 10 years ago

Comment on attachment 8537592 [details] [diff] [review]
patch 4 - Position text containers according to ruby-position

I think this is missing one nit from comment 33 -- add a comment noting that bsize is the annotation's *width*, inside the vertical-writing-mode clause (and that it's the annotations' *height*, inside the horizontal-writing-mode clause).

Also, please add a second "over" annotation to the test (maybe called "over2"), to show that we can do nested over, under, and then over again , and get that final "over" in the right place. (on top of the first one)

And if possible (I think it's possible?), please add a test for "left"/"right" annotations, as well (in a vertical writing-mode).  You'll probably need to add
 pref(layout.css.vertical-text.enabled,true)
before that test's line in the reftest.list file, to make sure vertical text is enabled.

r=me with the above addressed. Thanks!

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 42 • 10 years ago

I separate the tests into this independent patch.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 43 • 10 years ago

The bug number in the tests should be bug 1112474. I've changed them locally.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 44 • 10 years ago

Given the try result here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1f38d27eba69 I'm going to mark both tests in vertical mode expected failed.

	Daniel Holbert [:dholbert]
	Comment 45 • 10 years ago

Comment on attachment 8537672 [details] [diff] [review]
patch 5 - compute overflow areas of ruby frame

># HG changeset patch
># User Xidorn Quan <quanxunzhen@gmail.com>
># Date 1418805808 -39600
>#      Wed Dec 17 19:43:28 2014 +1100
># Node ID 4fee14c52a5bc353b357a19f313074b34d265249
># Parent  1e4c2fa4bcb05019bf2e5cf2d7f417b31c1c53a8
>Bug 1055665 part 5 - Compute overflow areas for ruby frame.
>
>diff --git a/layout/generic/nsRubyFrame.cpp b/layout/generic/nsRubyFrame.cpp
>--- a/layout/generic/nsRubyFrame.cpp
>+++ b/layout/generic/nsRubyFrame.cpp
>@@ -238,35 +238,38 @@ nsRubyFrame::Reflow(nsPresContext* aPres
>   LogicalMargin borderPadding = aReflowState.ComputedLogicalBorderPadding();
>   nscoord startEdge = borderPadding.IStart(frameWM);
>   nscoord endEdge = aReflowState.AvailableISize() - borderPadding.IEnd(frameWM);
>   NS_ASSERTION(aReflowState.AvailableISize() != NS_UNCONSTRAINEDSIZE,
>                "should no longer use available widths");
>   aReflowState.mLineLayout->BeginSpan(this, &aReflowState,
>                                       startEdge, endEdge, &mBaseline);
> 
>+  aDesiredSize.mOverflowAreas.Clear();

I don't think we need this explicit Clear() -- mOverflowAreas is intentionally initialized to be empty already:
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowMetrics.h?rev=e4310d8289b9&mark=57-57#56

It looks like a few other frame classes do have an explicit Clear() at the start of their reflow -- I'm guessing you might've added this from looking at one of them.  But it's unnecessary, and I'd rather we not have it, to reduce boilerplate and redundant work.

>@@ -432,16 +436,18 @@ nsRubyFrame::ReflowSegment(nsPresContext
>       } else {
>         // XXX inter-character support in bug 1055672
>         MOZ_ASSERT_UNREACHABLE("Unsupported ruby-position");
>       }
>     }
>     FinishReflowChild(textContainer, aPresContext, textMetrics,
>                       &textReflowState, x, y, 0);
>   }
>+
>+  aOverflowAreas.UnionAllWith(offsetRect);
> }

I think we really want to be unioning in each child's overflow area here, rather than unioning in offsetRect (which we just use for positioning).  In particular -- if the children themselves have overflow, that won't be reflected in offsetRect -- but it will be reflected in their overflow area, and we need to capture that.

I think we want to use nsFrame::ConsiderChildOverflow, documented here:
  https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.h#429

In nsFlexContainerFrame, we have:
   for (nsFrameList::Enumerator e(mFrames); !e.AtEnd(); e.Next()) {
     ConsiderChildOverflow(aDesiredSize.mOverflowAreas, e.get());
   }
I think we want something like that here.

	Daniel Holbert [:dholbert]
	Comment 46 • 10 years ago

Comment on attachment 8537673 [details] [diff] [review]
patch 6 - tests

>+++ b/layout/reftests/css-ruby/reftest.list
>@@ -14,10 +14,13 @@ fuzzy-if(winWidget,28,1) == dynamic-remo
> == ruby-whitespace-2.html ruby-whitespace-2-ref.html
>+== ruby-position-horizontal.html ruby-position-horizontal-ref.html
>+pref(layout.css.vertical-text.enabled,true) == ruby-position-vertical-lr.html ruby-position-vertical-lr-ref.html # bug 1112427
>+pref(layout.css.vertical-text.enabled,true) fails == ruby-position-vertical-rl.html ruby-position-vertical-rl-ref.html # bug 1112427

No need for a "# bug 1112427" annotation on the vertical-lr line, since you've got that test marked as passing.

>+++ b/layout/reftests/css-ruby/ruby-position-horizontal-ref.html
>+    div, p {
>+      padding: 0; margin: 0;
>+    }

This style rule seems unnecessary -- neither <div> nor <p> have padding by default, and <div> doesn't have any margin by default - so no need to set those things to 0.

If you were to keep this, I think it can be reduced to:
    p {
      margin: 0;
    }

BUT, you might just want to avoid using "p" here entirely, since you explicitly *don't* want the default <p> styling.  Something like <div class="fakeRubyText"> would probably be better -- and then you can do away with this rule altogether. (and replace "p" with "div.fakeRubyText" in the abspos style rule that comes later)

>+<body>
>+  <div style="height: 8em; line-height: 8em;">
>+    <div style="display: inline-block; line-height: normal;">
>+      <div class="anon"><p style="top: -100%;">over##</p>&nbsp;</div>
>+      <div class="anon"><p style="top: 100%;">under#</p>&nbsp;</div>
>+      <div class="anon"><p style="top: -200%;">over2#</p>&nbsp;</div>
>+      <div class="anon"><p style="top: 200%;">under2</p>&nbsp;</div>

Two nits:
 (1) Why is the class called "anon"? Is there anything anonymous here? (I don't think so...) Maybe call it "wrapper" or "container"? (since it exists solely to act as a container for its abspos child)

 (2) All of the "anon" and &nbsp; characters seem excessive / hacky.  I think you just need a *single* abspos containing block, which all of the fake annotations can be positioned inside of.  (And you should include a comment explaining why we need the &nbsp; character(s), so they're not just magic.)

I think this is equivalent and structurally simpler (just one wrapper, and just one &nbsp, with an explanation for the &nbsp; (please correct the explanation if it's wrong though) :))
     <div class="anon">
        <p style="top: -100%;">over##</p>
        <p style="top: 100%;">under#</p>
        <p style="top: -200%;">over2#</p>
        <p style="top: 200%;">under2</p>
        &nbsp;<!-- to give container a nonzero size for
                   percent values to resolve against -->
     </div>
     base##

Could you apply that sort of simplification?

(These nits -- (1) and (2) here -- applies to all of the reference cases in this patch.)

>+++ b/layout/reftests/css-ruby/ruby-position-vertical-lr-ref.html
>+      <div class="anon"><p style="right: 200%;">left2#</p>&nbsp;</div>
>+      <!--<div class="anon"><p style="right: -200%;">right2</p>&nbsp;</div>-->
>+      base##
>+    </div>

Right now, this is just mysteriously-commented-out HTML. Best not to have that, without an explanation/note, in test files, or else it's just cruft.

I think it's worth keeping, for symmetry w/ the horizontal test, but please file a bug on investigating what causes the off-by-1px issue with this annotation, and mention that bug number in the HTML comment here, to make this less mysterious.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 47 • 10 years ago

Remove the meaningless SetLineSize of nsRubyTextContainerFrame (since nsRubyBaseContainerFrame is its friend, it can change mLineSize directly), also mark another reftest fail.

	Daniel Holbert [:dholbert]
	Comment 50 • 10 years ago

Comment on attachment 8538129 [details] [diff] [review]
patch 3 - Position ruby annotations and set the line size of them correctly

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #47)
> Created attachment 8538129 [details] [diff] [review]
> patch 3 - Position ruby annotations and set the line size of them correctly
> 
> Remove the meaningless SetLineSize of nsRubyTextContainerFrame (since
> nsRubyBaseContainerFrame is its friend, it can change mLineSize directly)

Actually, I think I'd prefer that we keep the setter, to better-preserve at least a veneer of encapsulation.

(Even though we have a "friend" class declaration, I don't think we should drop all pretense of encapsulation & directly manipulate member-data on the other frame class, IMO.  In fact, it looks like the friend class declaration was only added here in the first place so that we'd have access to the setter that you're removing.  If anything is going to change here, I'd rather we go the other direction & make the setter public, so that we can drop the "friend" declaration. (instead of dropping the setter & taking full advantage of the "friend" declaration)  But I'm OK with a friend decl and a protected setter, too, as we had in the previous version of this patch.)

>+++ b/layout/generic/nsLineLayout.h
>+  // Get the final size of the line, in the block direction
>+  // Do not call this until after we've called VerticalAlignLine.
>+  nscoord GetFinalLineBSize() const

Needs a period at the end of the first sentence (after "direction").

>+  {
>+    MOZ_ASSERT(mFinalLineBSize != nscoord_MIN,
>+               "VerticalAlignLine should have been called before");

It's conceivable that this assertion can be tripped by content that uses absurdly large values.  (i.e. fuzzers)  We don't want to abort debug builds in that case (we'll produce bad layout, but no big deal) -- so, this should just be NS_ASSERTION. (non-fatal)

(In cases like this where it seems conceivable that a bogus testcase could trip an assertion, it's better to make the assert non-fatal so that fuzzers can proceed & discover actually-interesting/scary crashes (which might involve using bogusly-huge content) instead of forcing them to abort early on.)

>+++ b/layout/generic/nsRubyBaseContainerFrame.cpp
>@@ -376,18 +376,21 @@ nsRubyBaseContainerFrame::Reflow(nsPresC
>+    nsLineLayout* lineLayout = lineLayouts[i].get();
>+    lineLayout->VerticalAlignLine();
>+    textContainer->mLineSize =
>+      LogicalSize(lineWM, isize, lineLayout->GetFinalLineBSize());
>+    lineLayout->EndLineReflow();

(As noted above, I'd rather we use a setter here instead of directly tweaking textContainer->mLineSize -- let's bring that back from the previous version of this patch.)

	Daniel Holbert [:dholbert]
	Comment 51 • 10 years ago

Comment on attachment 8538130 [details] [diff] [review]
patch 5 - Compute relative position and overflow areas for ruby frames

>+++ b/layout/generic/nsRubyBaseContainerFrame.cpp
>@@ -380,16 +380,17 @@ nsRubyBaseContainerFrame::Reflow(nsPresC
>     // It happens before the ruby text container is reflowed, and that
>     // when it is reflowed, it will just use this size.
>     nsRubyTextContainerFrame* textContainer = i < rtcCount ?
>       mTextContainers[i] : mSpanContainers[i - rtcCount];
>     nsLineLayout* lineLayout = lineLayouts[i].get();
>     lineLayout->VerticalAlignLine();
>     textContainer->mLineSize =
>       LogicalSize(lineWM, isize, lineLayout->GetFinalLineBSize());
>+    lineLayout->RelativePositionFrames(textContainer->mOverflowAreas);
>     lineLayout->EndLineReflow();
>   }

Does this mean that relative positioning inside of ruby annotations didn't work, prior to this change? (does it work after this change?)

This really might deserve its own bug & tests... (And see below about textContainer->mOverflowAreas.)

>+++ b/layout/generic/nsRubyTextContainerFrame.h
>+  // The intended dimensions and the overflow areas of this ruby text
>+  // container. It is set by the corresponding ruby base container when
>+  // the segment is reflowed, and used when the ruby text container is
>+  // reflowed by its parent.
>   mozilla::LogicalSize mLineSize;
>+  nsOverflowAreas mOverflowAreas;

I'm uncomfortable with this (and hence r-minusing) because:
 (1) nsIFrame already has "mOverflow" and getter/setter "GetOverflowAreas()/SetOverflowAreas()".  It's too confusing to have those and also a completely different thing called "mOverflowAreas".

 (2) It seems wasteful to make a frame class bigger, only to store some data that's only used briefly during reflow.  (This applies to mLineSize as well, but I'm pushing back a bit now that we're adding another non-trivially-sized thing.) It feels like we should be storing this information in a temporary variable on the stack somehow.  In table layout, we put stuff like this on the "table reflow state"; I wonder if there's something like that we should use here.  (Though I also recall dbaron recommending against using custom state member-variables.)

(Maybe we can just have the ruby base container call the existing SetOverflowAreas method to *actually* set our overflow areas here?  I'm not sure if the overflow-variables that it would be passing [populated by nsLineLayout::RelativePositionFrames] would be in the right coordinate space & have the right amount of processing to directly pass to SetOverflowAreas, offhand.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 52 • 10 years ago

OK, so let's drop patch 5 for this bug. It might be better to be in bug 1055658. I don't think it would affect our tests in the last patch. But I am indeed interested in what's the best way to store the information during reflow, especially given that the information need to be passed between siblings.

FYI SetOverflowAreas() is private in nsIFrame, so we cannot call it in any place.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 53 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #50)
> Comment on attachment 8538129 [details] [diff] [review]
> patch 3 - Position ruby annotations and set the line size of them correctly
> 
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #47)
> > Created attachment 8538129 [details] [diff] [review]
> > patch 3 - Position ruby annotations and set the line size of them correctly
> > 
> > Remove the meaningless SetLineSize of nsRubyTextContainerFrame (since
> > nsRubyBaseContainerFrame is its friend, it can change mLineSize directly)
> 
> Actually, I think I'd prefer that we keep the setter, to better-preserve at
> least a veneer of encapsulation.
> 
> (Even though we have a "friend" class declaration, I don't think we should
> drop all pretense of encapsulation & directly manipulate member-data on the
> other frame class, IMO.  In fact, it looks like the friend class declaration
> was only added here in the first place so that we'd have access to the
> setter that you're removing.  If anything is going to change here, I'd
> rather we go the other direction & make the setter public, so that we can
> drop the "friend" declaration. (instead of dropping the setter & taking full
> advantage of the "friend" declaration)  But I'm OK with a friend decl and a
> protected setter, too, as we had in the previous version of this patch.)

Well, actually we cannot drop the friend even if we make that setter public. The current code invokes some other protected methods in nsContainerFrame.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 54 • 10 years ago

Change some code to make it work properly when there are autohidden annotations.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 55 • 10 years ago

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

	Daniel Holbert [:dholbert]
	Comment 56 • 10 years ago

Comment on attachment 8538131 [details] [diff] [review]
patch 6 - Tests

r=me. Thanks!

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 57 • 10 years ago

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c8dc0181bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/146f3133b135
https://hg.mozilla.org/integration/mozilla-inbound/rev/b55ca3e17876
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82e1fe5cfd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e097ce354d5

	Phil Ringnalda (:philor)
	Comment 58 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d4c8dc0181bb
https://hg.mozilla.org/mozilla-central/rev/146f3133b135
https://hg.mozilla.org/mozilla-central/rev/b55ca3e17876
https://hg.mozilla.org/mozilla-central/rev/d82e1fe5cfd1
https://hg.mozilla.org/mozilla-central/rev/4e097ce354d5

	Jean-Yves Perrier [:teoli]
	Comment 59 • 9 years ago

If somebody wants to help with the documentation, the following action has to be done:
* create https://developer.mozilla.org/en-US/docs/Web/CSS/ruby-position
(How to do it: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Howto/Document_a_CSS_property )

I'm happy to help. (Staff writers plan to do it as part of the whole ruby documentation, but this is not planned for Q1, so any help welcome.)

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 60 • 9 years ago

You probably should remove introduction of ruby-position from the "Firefox 37 for developers" page, because the syntax of ruby-position was changed in Firefox 38 in bug 1123917.

	Jean-Yves Perrier [:teoli]
	Comment 61 • 9 years ago

Xidorn: thanks. I did it. As I also implemented the new syntax, and this old one never shipped, I don't think this need any documentation.