implement vertical positioning of CSS ruby, including ruby-position property (other than the 'inter-character' value)

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dbaron, Assigned: xidorn)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Trunk
mozilla37
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 15 obsolete attachments)

20.33 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
3.15 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.43 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
7.75 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
7.95 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
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.
Summary: implement vertical positioning of CSS ruby, including ruby-position property → implement vertical positioning of CSS ruby, including ruby-position property (other than the 'inter-character' value)
Assignee: nobody → quanxunzhen
(Assignee)

Comment 1

4 years ago
Created attachment 8505835 [details] [diff] [review]
patch 1 - Determine BSize of rtc based on rt frames
(Assignee)

Comment 2

4 years ago
Created attachment 8506453 [details] [diff] [review]
patch 1 - Create fundamental code for ruby styles
Attachment #8505835 - Attachment is obsolete: true
Attachment #8506453 - Flags: review?(dbaron)
(Assignee)

Comment 3

4 years ago
Created attachment 8506458 [details] [diff] [review]
patch 2 - Parse ruby-position
Attachment #8506458 - Flags: review?(dbaron)
(Assignee)

Comment 4

4 years ago
The left parts of this bug depends on patches of bug 1052924.
(Assignee)

Updated

4 years ago
Blocks: 1055672
(Assignee)

Updated

4 years ago
Blocks: 1055676
(Assignee)

Updated

4 years ago
Blocks: 1069519
Attachment #8506453 - Flags: review?(dbaron) → review?(dholbert)
Attachment #8506458 - Flags: review?(dbaron) → review?(dholbert)
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/. :)
Attachment #8506453 - Flags: review?(dholbert)
Attachment #8506453 - Flags: review?(dbaron)
Attachment #8506453 - Flags: feedback+
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.
Attachment #8506453 - Flags: review?(dbaron) → review-
(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.)
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.
Attachment #8506458 - Flags: review?(dholbert) → feedback+
(Assignee)

Comment 9

4 years ago
Created attachment 8517243 [details] [diff] [review]
patch 1 - Parse & compute ruby-position
Attachment #8506453 - Attachment is obsolete: true
Attachment #8506458 - Attachment is obsolete: true
Attachment #8517243 - Flags: review?(dholbert)
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!
Attachment #8517243 - Flags: review?(dholbert) → review+
Blocks: 1094360
(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.)
(Assignee)

Comment 12

4 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.
Flags: needinfo?(dholbert)
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)
Flags: needinfo?(dholbert)
(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.)
No longer blocks: 1094360
(Assignee)

Comment 15

4 years ago
Created attachment 8517889 [details] [diff] [review]
patch 1 - Parse & compute ruby-position, r=dholbert
Attachment #8517243 - Attachment is obsolete: true
Attachment #8517889 - Flags: review+
(Assignee)

Comment 16

4 years ago
I move the computation of ruby-position. Now bug 1094360 no longer affects this patch.
(Assignee)

Updated

4 years ago
Depends on: 1052924
(Assignee)

Comment 17

4 years ago
Created attachment 8524982 [details] [diff] [review]
patch 2 - Determine BSize of RTC based on RT frames
(Assignee)

Comment 18

4 years ago
Created attachment 8524983 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position
(Assignee)

Updated

4 years ago
Attachment #8517889 - Attachment description: patch, r=dholbert → patch 1 - Parse & compute ruby-position, r=dholbert
(Assignee)

Comment 19

4 years ago
Created attachment 8525100 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position
Attachment #8524983 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 8526608 [details] [diff] [review]
patch 1 - Parse & compute ruby-position

Fix that it accepts single value.
Attachment #8517889 - Attachment is obsolete: true
Attachment #8526608 - Flags: review?(dholbert)
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!
Attachment #8526608 - Flags: review?(dholbert) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 8527949 [details] [diff] [review]
patch 1 - Parse & compute ruby-position, r=dholbert
Attachment #8526608 - Attachment is obsolete: true
Attachment #8527949 - Flags: review+
(Assignee)

Comment 23

4 years ago
Created attachment 8528146 [details] [diff] [review]
patch 2.1 - Make nsLineLayout::VerticalAlignLine not rely on line box

I'm going to use this function for nsRubyTextContainerFrame, where there is no line box available.
Attachment #8528146 - Flags: review?(roc)
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.
Attachment #8528146 - Flags: review?(roc) → review+
(Assignee)

Comment 25

4 years ago
Created attachment 8528771 [details] [diff] [review]
patch 2.2 - Position ruby annotations and set the line size of them correctly
Attachment #8524982 - Attachment is obsolete: true
Attachment #8528771 - Flags: review?(dholbert)
(Assignee)

Comment 26

4 years ago
Created attachment 8528772 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position
Attachment #8525100 - Attachment is obsolete: true
Attachment #8528772 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Blocks: 1107701
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...
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?
(Assignee)

Comment 29

4 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.
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.
Attachment #8528771 - Flags: review?(dholbert) → review+
(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" :)
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?
Flags: in-testsuite?
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.)
(Assignee)

Comment 34

4 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.
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)
(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 :)
(Looks like the first patch here was just bitrotted by bug 1110181, but it should still apply with fuzz.)
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.)
Attachment #8528772 - Flags: review?(dholbert) → review+
(Assignee)

Comment 39

4 years ago
Created attachment 8537592 [details] [diff] [review]
patch 4 - Position text containers according to ruby-position

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

The part numbers are flattened, so it is 4 instead 3 now.
Attachment #8528772 - Attachment is obsolete: true
Attachment #8537592 - Flags: review?(dholbert)
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!
Attachment #8537592 - Flags: review?(dholbert) → review+
(Assignee)

Updated

4 years ago
Blocks: 1112474
(Assignee)

Comment 41

4 years ago
Created attachment 8537672 [details] [diff] [review]
patch 5 - compute overflow areas of ruby frame
Attachment #8537672 - Flags: review?(dholbert)
(Assignee)

Comment 42

4 years ago
Created attachment 8537673 [details] [diff] [review]
patch 6 - tests

I separate the tests into this independent patch.
Attachment #8537673 - Flags: review?(dholbert)
(Assignee)

Comment 43

4 years ago
The bug number in the tests should be bug 1112474. I've changed them locally.
(Assignee)

Comment 44

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

Comment 47

4 years ago
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), also mark another reftest fail.
Attachment #8528771 - Attachment is obsolete: true
Attachment #8538129 - Flags: feedback?(dholbert)
(Assignee)

Comment 48

4 years ago
Created attachment 8538130 [details] [diff] [review]
patch 5 - Compute relative position and overflow areas for ruby frames
Attachment #8537672 - Attachment is obsolete: true
Attachment #8537672 - Flags: review?(dholbert)
Attachment #8538130 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #8538130 - Attachment description: patch 4 - Compute relative position and overflow areas for ruby frames → patch 5 - Compute relative position and overflow areas for ruby frames
(Assignee)

Comment 49

4 years ago
Created attachment 8538131 [details] [diff] [review]
patch 6 - Tests
Attachment #8537673 - Attachment is obsolete: true
Attachment #8537673 - Flags: review?(dholbert)
Attachment #8538131 - Flags: review?(dholbert)
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.)
Attachment #8538129 - Flags: feedback?(dholbert) → feedback+
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.)
Attachment #8538130 - Flags: review?(dholbert) → review-
(Assignee)

Comment 52

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

Comment 53

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

Comment 54

4 years ago
Created attachment 8538294 [details] [diff] [review]
patch 3 - Position text containers according to ruby-position

Change some code to make it work properly when there are autohidden annotations.
Attachment #8538129 - Attachment is obsolete: true
Attachment #8538294 - Flags: review?(dholbert)
(Assignee)

Updated

4 years ago
Attachment #8538130 - Attachment is obsolete: true
Attachment #8538294 - Flags: review?(dholbert) → review+
Comment on attachment 8538131 [details] [diff] [review]
patch 6 - Tests

r=me. Thanks!
Attachment #8538131 - Flags: review?(dholbert) → review+
Depends on: 1115264
Keywords: dev-doc-needed
Flags: in-testsuite? → in-testsuite+
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.)
(Assignee)

Updated

4 years ago
Blocks: 1123917
(Assignee)

Comment 60

4 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.
Flags: needinfo?(jypenator)
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.
Flags: needinfo?(jypenator)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.