Closed Bug 750551 Opened 12 years ago Closed 12 years ago

Vertically oriented boxes in RTL display elements mirrored vertically

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jaws, Assigned: smontagu)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch reftests (obsolete) — 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.
Attached patch reftests (obsolete) — Splinter Review
Updated the filenames and updated reftest.list.
Attachment #619757 - Attachment is obsolete: true
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.
Attached patch PatchSplinter Review
Assignee: nobody → smontagu
Attachment #619852 - Flags: review?(dholbert)
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 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;
}
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 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+
(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.
(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 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+
Ah, the reftest files are in /reftests/bidi but the tweaked reftest.list is in /reftests/bugs/
arggh, I did that when renaming the files per comment 10
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!
This test appears to be basically perma-orange.
(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
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.
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.
Target Milestone: mozilla15 → ---
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.
This was all green on tryserver https://tbpl.mozilla.org/?tree=Try&rev=b815b884f704
Attachment #621928 - Flags: review?(dholbert)
Attachment #621928 - Flags: review?(dholbert) → review+
(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! :)
You need to log in before you can comment on or make changes to this bug.