Last Comment Bug 750551 - Vertically oriented boxes in RTL display elements mirrored vertically
: Vertically oriented boxes in RTL display elements mirrored vertically
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks: 750056
  Show dependency treegraph
 
Reported: 2012-04-30 16:43 PDT by (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
Modified: 2013-11-13 02:19 PST (History)
5 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reftests (802 bytes, patch)
2012-04-30 16:43 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
reftests (1.28 KB, patch)
2012-04-30 16:46 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch (1.82 KB, patch)
2012-05-01 00:41 PDT, Simon Montagu :smontagu
dholbert: review+
Details | Diff | Review
Modified reftests (1.41 KB, patch)
2012-05-01 00:45 PDT, Simon Montagu :smontagu
dholbert: review+
Details | Diff | Review
Add text-align: left to the reftest (1.21 KB, patch)
2012-05-08 03:34 PDT, Simon Montagu :smontagu
dholbert: review+
Details | Diff | Review

Description (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-04-30 16:43:25 PDT
Created attachment 619757 [details] [diff] [review]
reftests

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.
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-04-30 16:46:01 PDT
Created attachment 619759 [details] [diff] [review]
reftests

Updated the filenames and updated reftest.list.
Comment 2 Simon Montagu :smontagu 2012-05-01 00:04:13 PDT
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.
Comment 3 Simon Montagu :smontagu 2012-05-01 00:41:06 PDT
Created attachment 619852 [details] [diff] [review]
Patch
Comment 4 Simon Montagu :smontagu 2012-05-01 00:45:09 PDT
Created attachment 619853 [details] [diff] [review]
Modified reftests

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.
Comment 5 neil@parkwaycc.co.uk 2012-05-01 01:37:10 PDT
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;
}
Comment 6 Frank Yan (:fryn) 2012-05-03 15:29:57 PDT
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 Daniel Holbert [:dholbert] 2012-05-03 17:34:33 PDT
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. :))
Comment 8 Daniel Holbert [:dholbert] 2012-05-03 17:51:41 PDT
(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 Daniel Holbert [:dholbert] 2012-05-03 17:53:55 PDT
(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 Daniel Holbert [:dholbert] 2012-05-03 17:56:51 PDT
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.
Comment 11 :Ehsan Akhgari (out sick) 2012-05-07 09:56:08 PDT
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 Daniel Holbert [:dholbert] 2012-05-07 10:31:43 PDT
Ah, the reftest files are in /reftests/bidi but the tweaked reftest.list is in /reftests/bugs/
Comment 13 Simon Montagu :smontagu 2012-05-07 11:00:15 PDT
arggh, I did that when renaming the files per comment 10
Comment 14 Daniel Holbert [:dholbert] 2012-05-07 11:37:47 PDT
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!
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-05-07 14:00:36 PDT
This test appears to be basically perma-orange.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-05-07 14:21:43 PDT
(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 Daniel Holbert [:dholbert] 2012-05-07 14:27:51 PDT
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 20 Simon Montagu :smontagu 2012-05-07 23:19:09 PDT
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.
Comment 21 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-05-07 23:27:10 PDT
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 23 Simon Montagu :smontagu 2012-05-08 03:34:02 PDT
Created attachment 621928 [details] [diff] [review]
Add text-align: left to the reftest

This was all green on tryserver https://tbpl.mozilla.org/?tree=Try&rev=b815b884f704
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-05-08 18:52:03 PDT
https://hg.mozilla.org/mozilla-central/rev/81535cbb76bc
Comment 26 Frank Yan (:fryn) 2012-05-08 18:57:52 PDT
(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! :)

Note You need to log in before you can comment on or make changes to this bug.