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)
Core
Layout
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)
241 bytes,
text/html
|
Details | |
11.36 KB,
patch
|
jfkthame
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•9 years ago
|
||
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=24bb85e9dced&tochange=26118238484f Triggered by: Bug 1141931
Updated•9 years ago
|
Blocks: 1141931
Keywords: regressionwindow-wanted
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
Thanks! That means Firefox 40 is also affected.
Comment 3•9 years ago
|
||
oops pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a7dbe1bc7878&tochange=588c7ee3cf4f
Assignee | ||
Updated•9 years ago
|
Summary: ruby-align is sometimes ineffective and handled like "start" → ruby-align: center behaves like ruby-align: start when bidi handling is triggered
Assignee | ||
Comment 4•9 years ago
|
||
Assignee: nobody → quanxunzhen
Attachment #8632078 -
Flags: review?(jfkthame)
Assignee | ||
Comment 5•9 years ago
|
||
[Tracking Requested - why for this release]: It is a regression since this release, and it causes problem on some real content on Wikipedia.
tracking-firefox41:
--- → ?
Assignee | ||
Comment 6•9 years ago
|
||
I mean... firefox40, not firefox41.
tracking-firefox40:
--- → ?
tracking-firefox41:
? → ---
Comment 7•9 years ago
|
||
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+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3e19c8e5c60c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab15a5c156c7
Flags: in-testsuite+
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Comment 14•9 years ago
|
||
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.
Description
•