Closed Bug 1181890 Opened 9 years ago Closed 9 years ago

ruby-align: center behaves like ruby-align: start when bidi handling is triggered

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + fixed
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

The rubies in the table have "ruby-align: center" specified, but the behavior is like "ruby-align: left".

I cannot reproduce this on the release version, hence likely a regression from Firefox 40.
Thanks! That means Firefox 40 is also affected.
Summary: ruby-align is sometimes ineffective and handled like "start" → ruby-align: center behaves like ruby-align: start when bidi handling is triggered
Attached patch patchSplinter Review
Assignee: nobody → quanxunzhen
Attachment #8632078 - Flags: review?(jfkthame)
[Tracking Requested - why for this release]:
It is a regression since this release, and it causes problem on some real content on Wikipedia.
I mean... firefox40, not firefox41.
Comment on attachment 8632078 [details] [diff] [review]
patch

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

Looks good. Some minor adjustments suggested below (which I don't think change behavior, they just clean up the code a bit).

Also, please add a != version of the reftest to check that ruby-align:center renders differently from ruby-align:start.

r=me with that.

::: layout/base/nsBidiPresUtils.cpp
@@ +1273,5 @@
>    // If this line consists of a line frame, reorder the line frame's children.
>    if (aFirstFrameOnLine->GetType() == nsGkAtoms::lineFrame) {
>      aFirstFrameOnLine = aFirstFrameOnLine->GetFirstPrincipalChild();
>      if (!aFirstFrameOnLine)
> +      return 0;

nit: while you're here, please add the missing braces around this (gecko style).

@@ +1445,5 @@
> +  if (childList.IsEmpty()) {
> +    return;
> +  }
> +
> +  WritingMode frameWM = aFrame->GetWritingMode();

The caller already knows the frame's writing mode, so pass it in as a parameter instead of performing a (virtual) GetWritingMode() call here.

@@ +1456,5 @@
> +                                aBorderPadding.IStart(frameWM));
> +  isize += aBorderPadding.IEnd(frameWM);
> +
> +  auto rubyAlign = aFrame->StyleText()->mRubyAlign;
> +  if (rubyAlign == NS_STYLE_RUBY_ALIGN_START) {

No need for the rubyAlign variable here, IMO (it's not reused at all): just use the aFrame->... expression directly in the condition.

@@ +1459,5 @@
> +  auto rubyAlign = aFrame->StyleText()->mRubyAlign;
> +  if (rubyAlign == NS_STYLE_RUBY_ALIGN_START) {
> +    return;
> +  }
> +  nscoord leftISize = aFrame->ISize(frameWM) - isize;

Please name this something different, as "left" sounds like it's referring to a physical direction/side, which may be confusing in vertical writing mode. Maybe "residualISize"?

@@ +1467,5 @@
> +
> +  // When ruby-align is not "start", if the content does not fill this
> +  // frame, we need to center the children.
> +  for (nsIFrame* child : childList) {
> +    LogicalRect rect = child->GetLogicalRect(0);

I think you should call child->GetLogicalRect(frameWM, 0) here, both because it's cheaper (it avoids a virtual GetWritingMode() call within GetLogicalRect), and because some day we might support bidi within ruby, but IIUC the adjustment here should be applied in the parent frame's WM for all the children regardless of their own WM.

::: layout/reftests/css-ruby/bug1181890-ref.html
@@ +1,1 @@
> +<p style="ruby-align: center">

Except when we're specifically testing quirks-mode behavior, I'd prefer to include a <!DOCTYPE html> declaration in the test and reference files, so that they run in standards mode.
Attachment #8632078 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/mozilla-central/rev/3e19c8e5c60c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8632078 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1141931
[User impact if declined]: have alignment issue with Chinese ruby in some pages
[Describe test coverage new/current, TreeHerder]: added test
[Risks and why]: low, as it only affects very limited cases
[String/UUID change made/needed]: n/a
Attachment #8632078 - Flags: approval-mozilla-beta?
Attachment #8632078 - Flags: approval-mozilla-aurora?
Comment on attachment 8632078 [details] [diff] [review]
patch

Low risk, has tests, taking it.
Attachment #8632078 - Flags: approval-mozilla-beta?
Attachment #8632078 - Flags: approval-mozilla-beta+
Attachment #8632078 - Flags: approval-mozilla-aurora?
Attachment #8632078 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
Although this has already been uplifted to 40, tracking this user visible regression in case the bug is reopened.
You need to log in before you can comment on or make changes to this bug.