Closed
Bug 750551
Opened 12 years ago
Closed 12 years ago
Vertically oriented boxes in RTL display elements mirrored vertically
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: jaws, Assigned: smontagu)
References
Details
Attachments
(3 files, 2 obsolete files)
1.82 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
If a page is viewed in RTL and it has <body dir=rtl> along with the body having: body { -moz-box-orient: vertical; display: -moz-box; } then the children of the body are shown in reverse vertical order, which is probably not what should happen for RTL mode. I attached a reftest to demonstrate this. Setting tracking-firefox13 and tracking-firefox14 since this bug is blocking the new about:home on RTL.
Reporter | ||
Comment 1•12 years ago
|
||
Updated the filenames and updated reftest.list.
Attachment #619757 -
Attachment is obsolete: true
Assignee | ||
Comment 2•12 years ago
|
||
nsBoxFrame::GetInitialDirection uses dir="rtl" to reverse layout. It seems to make some effort to have this only happen in the horizontal orientation case, but not for all code paths.
Assignee | ||
Comment 3•12 years ago
|
||
Assignee: nobody → smontagu
Attachment #619852 -
Flags: review?(dholbert)
Assignee | ||
Comment 4•12 years ago
|
||
Attachment 619759 [details] [diff] fails even with the patch because there is more vertical space between the <p> elements with display: -moz-box. This uses display: -moz-box for both test and reference.
Attachment #619759 -
Attachment is obsolete: true
Attachment #619853 -
Flags: review?(dholbert)
Comment 5•12 years ago
|
||
Comment on attachment 619852 [details] [diff] [review] Patch >+ GetContent()->GetAttr(kNameSpaceID_None, nsGkAtoms::dir, dirValue); >+ if (dirValue.EqualsLiteral("reverse")) { GetContent()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir, nsGkAtoms::reverse, eCaseMatters) Or it might be possible to rewrite the code as follows: if (GetContent()->AttrValueIs(...)) aIsNormal = !aIsNormal; else if (IsVertical()) { static nsIContent::AttrValuesArray strings[] = {&nsGkAtoms::rtl, &nsGkAtoms::ltr, nsnull}; PRInt32 index = GetContent()->FindAttrValueIn(...); if (index >= 0) aIsNormal = index != 0; }
Updated•12 years ago
|
status-firefox13:
--- → affected
status-firefox14:
--- → affected
Comment 6•12 years ago
|
||
Simon and/or Daniel, we need this on beta, since the CSS on the new about:home revision hit this bug. Do you think the patches here is safe to take for beta?
Comment 7•12 years ago
|
||
Comment on attachment 619852 [details] [diff] [review] Patch r=me, with neil's first suggested change in comment 5 -- that is, I think we want to call AttrValueIs w/ the same arguments that we pass to FindAttrValueIn, rather than calling GetAttr & EqualsLiteral. (I don't think it's worth following the rest of comment 5 -- I agree that Neil's alternate logic might be slightly better (with s/IsVertical/IsHorizontal/), but it introduces an extra virtual function call in the horizontal case, whereas Simon's existing patch just has one virtual function call regardless of whether we're horizontal or vertical. Given that, the alternate logic isn't clearly-better enough to sway me.) (Side note: I'm not sure I've reviewed changes to layout/xul before, but I think this is a simple-enough patch that it's sufficient that Simon & Neil & I all think it's sane. :))
Attachment #619852 -
Flags: review?(dholbert) → review+
Comment 8•12 years ago
|
||
(In reply to Frank Yan (:fryn) from comment #6) > Simon and/or Daniel, we need this on beta, since the CSS on the new > about:home revision hit this bug. Do you think the patches here is safe to > take for beta? Well... It affects web layout, so that's a little scary. In particular, it'll reverse the flow for... (a) vertical -moz-boxes with dir="rtl" (the dir=rtl will now be ignored, so we'll now be top-to-bottom instead of bottom-to-top) (b) vertical -moz-boxes with dir="ltr" and style="box-direction: reverse" (the dir="ltr" will now be ignored, so "box-direction:reverse" will now take effect, making us bottom-to-top instead of top-to-bottom) Since this affects layout & isn't fixing a layout regression (AFAIK), this seems like the type of thing we'd normally avoid taking on a beta (but could potentially take on Aurora). However, given that we have a new feature that doesn't work in some circumstances because of this bug, maybe that's more important...? (I don't know how to make that judgement-call.) If it's possible to hack around this in about:home, it seems like that'd be better for the beta branch.
Comment 9•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #8) > In particular, it'll reverse the flow for... (to be clear: the new reversed flow is arguably correct, and the old flow was broken, in the cases that I've called out. I just don't know if there's content (on the web or in an addon) that depends on the existing behavior, whether intentionally or unintentionally. If there is, it'd be pretty bad to break that midway through a beta cycle -- that's why we tend to avoid taking layout fixes for non-regressions during beta.)
Comment 10•12 years ago
|
||
Comment on attachment 619853 [details] [diff] [review] Modified reftests >diff --git a/layout/reftests/bugs/reftest.list b/layout/reftests/bugs/reftest.list >--- a/layout/reftests/bugs/reftest.list >+++ b/layout/reftests/bugs/reftest.list >@@ -1695,8 +1695,9 @@ fuzzy-if(d2d,1,19) fuzzy-if(cocoaWidget, > == 722923-1.html 722923-1-ref.html > == 723484-1.html 723484-1-ref.html > == 728983-1.html 728983-1-ref.html > == 729143-1.html 729143-1-ref.html > == 731521-1.html 731521-1-ref.html > needs-focus == 731726-1.html 731726-1-ref.html > == 735481-1.html 735481-1-ref.html > == 745934-1.html 745934-1-ref.html >+== 750551.html 750551-ref.html Name these reftest files 750551-1.html / 750551-1-ref.html (inserting "-1") to match the convention for test-file-naming in that directory. r=me with that.
Attachment #619853 -
Flags: review?(dholbert) → review+
Comment 11•12 years ago
|
||
I backed this out because of the reftest failure: https://hg.mozilla.org/integration/mozilla-inbound/rev/97bef33bf606 Example log: https://tbpl.mozilla.org/php/getParsedLog.php?id=11535301&tree=Mozilla-Inbound&full=1
Comment 12•12 years ago
|
||
Ah, the reftest files are in /reftests/bidi but the tweaked reftest.list is in /reftests/bugs/
Assignee | ||
Comment 13•12 years ago
|
||
arggh, I did that when renaming the files per comment 10
Comment 14•12 years ago
|
||
Also, one related nit: it looks like this originally landed with the reftest.list file out-of-order, probably due to a merge conflict (resulting in a 74* test being listed after this test's 75* test): > == 745934-1.html 745934-1-ref.html >+== 750551-1.html 750551-1-ref.html > == 748803-1.html 748803-1-ref.html https://hg.mozilla.org/integration/mozilla-inbound/rev/8c93370061f8#l3.1 When you re-land, do make sure it ends up preserving the ascending test-order (whichever folder you stick the tests in). Thanks!
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/14c53b0ff2e7 https://hg.mozilla.org/integration/mozilla-inbound/rev/cab8af0ca040
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Comment 16•12 years ago
|
||
This test appears to be basically perma-orange.
Comment 17•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #16) > This test appears to be basically perma-orange. ....on Linux, that is. Looks like the 1 & 3 are slightly misaligned. Marked as failing on Linux for now. https://hg.mozilla.org/integration/mozilla-inbound/rev/15b8fd11fc95
Comment 18•12 years ago
|
||
Simon, mind investigating/fixing Comment 17? I'd rather not have that fails-if annotation stick for too long. I'm guessing it's from whitespace somewhere in the test... removing a bunch of whitespace might fix it up.
Comment 19•12 years ago
|
||
Original landing and backout: http://hg.mozilla.org/mozilla-central/rev/8c93370061f8 http://hg.mozilla.org/mozilla-central/rev/eb679c399a30 http://hg.mozilla.org/mozilla-central/rev/97bef33bf606
Assignee | ||
Comment 20•12 years ago
|
||
I don't see the failure on Linux locally. It looks like an issue with text-align, though I don't understand why that wouldn't affect all platforms. Pushed https://hg.mozilla.org/try/rev/b815b884f704 as an experimental fix.
Reporter | ||
Comment 21•12 years ago
|
||
Bug 750056 has now been fixed using a workaround in Fx13 and Fx14, so the priority of these fixes is lower now since we don't know of web content that is hitting this bug.
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/14c53b0ff2e7 https://hg.mozilla.org/mozilla-central/rev/cab8af0ca040 https://hg.mozilla.org/mozilla-central/rev/15b8fd11fc95
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Assignee | ||
Comment 23•12 years ago
|
||
This was all green on tryserver https://tbpl.mozilla.org/?tree=Try&rev=b815b884f704
Attachment #621928 -
Flags: review?(dholbert)
Updated•12 years ago
|
Attachment #621928 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81535cbb76bc
Comment 26•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #21) > Bug 750056 has now been fixed using a workaround in Fx13 and Fx14. We don't need to track this for 13 and 14 anymore. Thanks for fixing this bug quickly anyways! :)
tracking-firefox13:
? → ---
tracking-firefox14:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•