Closed Bug 1123917 Opened 5 years ago Closed 5 years ago

Collapse ruby-position values

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: xidorn, Assigned: xidorn)

References

()

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

The current spec defines ruby-position to have pair value. But I and some other people think this is unnecessary complicated, and have requested to collapse it to accept only enumerated values instead of pair. [1][2]

The editor seems to agree with this change. [3]

Also, currently WebKit's prefixed ruby-position uses enumerated values.

I think we should change the implementation to this new value syntax.

[1] https://lists.w3.org/Archives/Public/www-style/2014Sep/0228.html
[2] https://lists.w3.org/Archives/Public/www-style/2015Jan/0300.html
[3] https://lists.w3.org/Archives/Public/www-style/2015Jan/0331.html
Assignee: nobody → quanxunzhen
Depends on: 1055665
Both editors have agreed with this change.
https://lists.w3.org/Archives/Public/www-style/2015Jan/0372.html
Attachment #8552105 - Flags: review?(dholbert)
Comment on attachment 8552105 [details] [diff] [review]
patch 2 - Collapse ruby-position values

r=me with the following addressed:

>Bug 1123917 part 2 - Collapse ruby-position values.

This is too vague.  Expand to something like:
  "Make 'ruby-position' CSS property only accept 'over' & 'under' instead of compound values."

>+++ b/layout/generic/nsRubyFrame.cpp
[...]
>+    nscoord x = baseRect.X();
>+    nscoord y = baseRect.Y();
>+    if (side.isSome()) {
>+      switch (side.value()) {
>+      case eSideLeft:
>         x = offsetRect.X() - bsize;
>         offsetRect.SetLeftEdge(x);
>+        break;
[...]
>+      case eSideBottom:
>         y = offsetRect.YMost();
>         offsetRect.SetBottomEdge(y + bsize);
>+        break;

A few things:
 - Can we use offsetRect here instead of baseRect, to initialize x/y?  Note that the current code (m-c) uses offsetRect consistently here, and doesn't use baseRect at all inside this loop -- and this makes the code easier to reason about, because you only have to be thinking about one rect & how it changes.  (And I think offsetRect.x should be equal to baseRect.x, if our side is top/bottom, and similar for y if our side is left/right -- so I think this change shouldn't affect behavior.)

 - The patch initializes both 'x' and 'y' here, and then the switch statement stomps on one of those values and leaves the other untouched. That unnecessary-initialization (of either x or y) feels worth avoiding -- maybe by declaring x & y without values, and adding an explicit "y = offsetRect.Y();" to each of the left & right cases, and similar for x in the top/bottom cases. (so that each case initializes both coordinates -- slightly more verbose, but also slightly clearer [since we initialize the coordinate-pair as a whole] & less variable-value-churn).   If you'd rather keep this as it is, though, then please at least add a comment above the x/y decls saying something like "Note: We'll clobber one of these in the switch statement below."

>+++ b/layout/style/nsCSSPropList.h
>     eStyleAnimType_Sides_Right)
> CSS_PROP_TEXT(
>     ruby-position,
>     ruby_position,
[...]
>-    CSS_PROP_NO_OFFSET,
>+    offsetof(nsStyleText, mRubyPosition),
>     eStyleAnimType_None)

Optional: Might as well upgrade the animtype to eStyleAnimType_EnumU8, if you're providing an offsetof() here now.  (This should get us basic "animation support" for free, which IIRC means authors will be able to use this in CSS animations, just snapping between values.) Alternately, if you want to leave this AnimType_None for now, then it's probably best to keep the offset field untouched, as CSS_PROP_NO_OFFSET, because that field is documented as being "Ignored (and generally CSS_PROP_NO_OFFSET ...) for properties whose animtype_ is eStyleAnimType_None" at the top of this file:
https://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h#64

(We're somewhat, though not entirely, consistent about using _NO_OFFSET with AnimType_None, IIRC.)

>+++ b/layout/style/nsStyleConsts.h
>@@ -836,23 +836,19 @@ static inline mozilla::css::Side operato
> #define NS_STYLE_WORDWRAP_BREAK_WORD            1
> 
> // See nsStyleText
> #define NS_STYLE_HYPHENS_NONE                   0
> #define NS_STYLE_HYPHENS_MANUAL                 1
> #define NS_STYLE_HYPHENS_AUTO                   2
> 
> // ruby-position, see nsStyleText
[...]
>+#define NS_STYLE_RUBY_POSITION_OVER             0x00
>+#define NS_STYLE_RUBY_POSITION_UNDER            0x01
>+#define NS_STYLE_RUBY_POSITION_INTER_CHARACTER  0x02 // placeholder, not yet parsed
> 
> // See nsStyleText
> #define NS_STYLE_TEXT_SIZE_ADJUST_NONE          0
> #define NS_STYLE_TEXT_SIZE_ADJUST_AUTO          1

Since this is just an enum value now (not a mask), let's just use 0, 1, 2 instead of 0x00, 0x01, 0x02, for better readability & consistency w/ code around it.
Attachment #8552105 - Flags: review?(dholbert) → review+
Comment on attachment 8552104 [details] [diff] [review]
patch 1 - Add line-relative direction to writing modes

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

For clarity, please make the commit message something like "Add line-relative direction mapping methods to writing modes".
r=me, with minor suggestion below.

::: layout/generic/WritingModes.h
@@ +68,5 @@
>  }
>  
> +inline LogicalEdge GetOppositeEdge(LogicalEdge aEdge)
> +{
> +  return aEdge != eLogicalEdgeStart ? eLogicalEdgeStart : eLogicalEdgeEnd;

It's probably more efficient to write this as

  return LogicalEdge(1 - aEdge);

(perhaps with a comment that this relies on the only two LogicalEdge enum values being 0 and 1).
Attachment #8552104 - Flags: review?(jfkthame) → review+
Comment on attachment 8552695 [details] [diff] [review]
patch 2 - Make ruby-position only accept [over|under] instead of compound values

Nice -- this is much clearer with nsPoint, TopLeft, etc!

Eventually, though, this nsPoint *really* wants to be a LogicalPoint, since the reason it exists is to provide args for FinishReflowChild() -- and FinishReflowChild() is being converted to take LogicalPoint instead of "x,y". See XXXtemporary comment above the old x,y version here:
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.h?rev=c16d59bb79d1#281
(The new LogicalPoint version of the function is up a bit higher in that file)

I think it probably still make sense to land this patch as-is, and do the FinishReflowChild-version/LogicalPoint conversion separately -- so r=me on this patch, and please file a followup on using the modernized logical-coord version of FinishReflowChild.

(Alternately if you'd like to do the LogicalPoint/FinishReflowChild conversion as part of this patch, that works too -- but it'd probably be good for me or someone to sanity-check that change w/ one more round of review.)
Attachment #8552695 - Flags: review?(dholbert) → review+
Blocks: 1124434
https://hg.mozilla.org/mozilla-central/rev/99aa38f78dae
https://hg.mozilla.org/mozilla-central/rev/74d5b7a1a448
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/ruby-align
and
https://developer.mozilla.org/en-US/Firefox/Releases/38#CSS (no mention of change as the previous version was behind a flag)
You need to log in before you can comment on or make changes to this bug.