Closed Bug 1395777 Opened 7 years ago Closed 7 years ago

Treat rtc with orthogonal writing-mode as inter-character

Categories

(Core :: Layout: Block and Inline, enhancement, P3)

Product:
Core
Component:
Layout: Block and Inline
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: kevin.hsieh, Assigned: kevin.hsieh)

References

(
URL
)

Details

Attachments

(2 files)

Bug 1395777 (part 1) - Allow rtc to use its own writing mode.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1395777 (part 2) - Make orthogonal ruby annotations inter-character.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
This bug is for an experimental implementation of inter-character rubies (Bug 1055672). See the CSSWG issue for more details.
Comment on attachment 8904855 [details]
Bug 1395777 (part 1) - Allow rtc to use its own writing mode.

https://reviewboard.mozilla.org/r/176628/#review182116

::: layout/generic/nsLineLayout.cpp:1217
(Diff revision 3)
> +      if (lineWM.IsOrthogonalTo(rtc->mFrame->GetWritingMode())) {
> +        // Inter-character case: don't attempt to sync annotation bounds.
> +        continue;
> +      }

As far as you don't have inter-character rtc attached to the base line layout, this shouldn't be necessary.

::: layout/generic/nsLineLayout.cpp:3024
(Diff revision 3)
> +    if (lineWM.IsOrthogonalTo(annotation->mFrame->GetWritingMode())) {
> +      // Inter-character case: don't attempt to expand ruby annotations.
> +      continue;
> +    }

Ditto.

::: layout/generic/nsRubyBaseContainerFrame.cpp:358
(Diff revision 3)
>      reflowInputs.AppendElement(reflowInput);
>      nsLineLayout* lineLayout = new nsLineLayout(aPresContext,
>                                                  reflowInput->mFloatManager,
>                                                  reflowInput, nullptr,
> -                                                aReflowInput.mLineLayout);
> +                                                orthogonalWM ? nullptr
> +                                                : aReflowInput.mLineLayout);

Please align `:` with `?` above.

::: layout/generic/nsRubyTextContainerFrame.cpp:131
(Diff revision 3)
>    // Although a ruby text container may have continuations, returning
>    // complete reflow status is still safe, since its parent, ruby frame,
>    // ignores the status, and continuations of the ruby base container
>    // will take care of our continuations.
>    aStatus.Reset();
> -  WritingMode lineWM = aReflowInput.mLineLayout->GetWritingMode();
> +  WritingMode rtcWM = GetWritingMode();

Hmmm, I guess this is fine.

Given that you don't use line layout anymore, you can probably remove the code which sets line layout for it in https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/generic/nsRubyFrame.cpp#318-322
Attachment #8904855 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8904855 [details]
Bug 1395777 (part 1) - Allow rtc to use its own writing mode.

https://reviewboard.mozilla.org/r/176628/#review182116

> Please align `:` with `?` above.

I'll move the ternary onto its own line.
Priority: -- → P3
Comment on attachment 8904856 [details]
Bug 1395777 (part 2) - Make orthogonal ruby annotations inter-character.

https://reviewboard.mozilla.org/r/176630/#review182144

I'm still concerned about the potential forward compatibility issue of this approach... I actually wonders whether inter-character ruby is a good idea at all now :/

::: layout/generic/nsRubyFrame.cpp:210
(Diff revision 5)
> +  // Cache the boolean preference for inter-character ruby annotation support.
> +  static bool sAllowInterCharacterRuby = false;
> +  static bool sAllowInterCharacterRubyInited = false;
> +  if (!sAllowInterCharacterRubyInited) {
> +    Preferences::AddBoolVarCache(&sAllowInterCharacterRuby,
> +                                 "layout.css.ruby.intercharacter.enabled");
> +    sAllowInterCharacterRubyInited = true;
> +  }

I would prefer you put this code into nsLayoutUtils just like how we handle many other layout prefs, e.g. https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/base/nsLayoutUtils.cpp#745-759

::: layout/generic/nsRubyFrame.cpp:360
(Diff revision 5)
> -      if (side.value() == eLogicalSideBStart) {
> +      if (sAllowInterCharacterRuby && rtcWM.IsVerticalRL() &&
> +          lineWM.GetInlineDir() == WritingMode::eInlineLTR) {
> +        // Inter-character case: vertical-rl in ltr horizontal writing
> +        LogicalPoint offset(lineWM, offsetRect.ISize(lineWM),
> +          offsetRect.BSize(lineWM) > size.BSize(lineWM) ?
> +          (offsetRect.BSize(lineWM) - size.BSize(lineWM)) / 2 : 0);
> +        position = offsetRect.Origin(lineWM) + offset;
> +        aReflowInput.mLineLayout->AdvanceICoord(size.ISize(lineWM));
> +      } else if (sAllowInterCharacterRuby && lineWM.IsVerticalRL() &&
> +                 rtcWM.GetInlineDir() == WritingMode::eInlineLTR) {
> +        // Inter-character case: ltr horizontal writing in vertical-rl
> +        LogicalPoint offset(lineWM, offsetRect.ISize(lineWM),
> +          (offsetRect.BSize(lineWM) > size.BSize(lineWM) ?
> +           (offsetRect.BSize(lineWM) - size.BSize(lineWM)) / 2 : 0)
> +          - offsetRect.BSize(lineWM));
> +        position = offsetRect.Origin(lineWM) + offset;
> +        aReflowInput.mLineLayout->AdvanceICoord(size.ISize(lineWM));

Why couldn't we just do `sAllowInterCharacterRuby && rtcWM.IsOrthogonalTo(lineWM)`?

::: layout/reftests/css-ruby/reftest.list:48
(Diff revision 5)
> +pref(layout.css.ruby.intercharacter.enabled,false) == ruby-intercharacter-rl.htm ruby-intercharacter-lr.htm
> +pref(layout.css.ruby.intercharacter.enabled,true) != ruby-intercharacter-rl.htm ruby-intercharacter-lr.htm

Please add a `==` reftest for the enabled case to ensure it works in the expected way.
Attachment #8904856 - Flags: review?(xidorn+moz)
Comment on attachment 8904856 [details]
Bug 1395777 (part 2) - Make orthogonal ruby annotations inter-character.

https://reviewboard.mozilla.org/r/176630/#review182144

> Why couldn't we just do `sAllowInterCharacterRuby && rtcWM.IsOrthogonalTo(lineWM)`?

This is because for the purposes of this implementation, I've only accounted for the cases of vertical-rl in ltr horizontal writing and ltr horizontal writing in vertical-rl, falling back to non-inter-character behavior otherwise. Other cases are more difficult and sometimes it's not entirely clear what should be done.
Comment on attachment 8904856 [details]
Bug 1395777 (part 2) - Make orthogonal ruby annotations inter-character.

https://reviewboard.mozilla.org/r/176630/#review184788

::: layout/generic/nsRubyFrame.cpp:360
(Diff revision 6)
> +      } else if (allowInterCharacter && lineWM.IsVerticalRL() &&
> +                 rtcWM.GetInlineDir() == WritingMode::eInlineLTR) {

If we can only support these specific cases rather than a general concept that orthogonal writing-mode leads to inter-character, I'd prefer we just support vertical-in-horizontal above, since horizontal-in-vertical doesn't really have usecase backed, just like all other orthogonal writing-mode combinations. It isn't worth handling that specifically.

::: layout/reftests/css-ruby/ruby-intercharacter.htm:20
(Diff revision 6)
> +}
> +</style>
> +</head>
> +<body>
> +  <ruby>
> +    <rbc>BASE</rbc><rtc>ANNOTATION</rtc>

So there are two cases:
1. the annotation is longer than the height of base text, and it is top aligned;
2. the annotation is shorter, and it is centered.

I think we should have test for both behaviors.

If you find it hard to do precious reference, consider using the Ahem testing font, which generates 1em square for most characters, so that you can produce reliable test and reference.

Also, if you want to support horizontal-in-vertical, there should be a test for that as well. (Although I don't think it's worth, and we probably should just remove that code.)
Attachment #8904856 - Flags: review?(xidorn+moz)
Comment on attachment 8904856 [details]
Bug 1395777 (part 2) - Make orthogonal ruby annotations inter-character.

https://reviewboard.mozilla.org/r/176630/#review185642
Attachment #8904856 - Flags: review?(xidorn+moz) → review+
There's some kind of platform-specific issue with one of the reftests. I'm looking into it.
Using ni? to request re-review of modifications to first reftest.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab560d9b081108217208dd4863eb2e353719fa0
Flags: needinfo?(xidorn+moz)
r+
Flags: needinfo?(xidorn+moz)
Keywords: checkin-needed
Note: the functionality implemented here is disabled by default but can be enabled by flipping the "layout.css.ruby.intercharacter.enabled" pref to true.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/044422a93db9
(part 1) - Allow rtc to use its own writing mode. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ad20c4bf0d57
(part 2) - Make orthogonal ruby annotations inter-character. r=xidorn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/044422a93db9
https://hg.mozilla.org/mozilla-central/rev/ad20c4bf0d57
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1401709
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: