Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Vertically oriented boxes in RTL display elements mirrored vertically

RESOLVED FIXED in mozilla15

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaws, Assigned: smontagu)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Created attachment 619759 [details] [diff] [review]
reftests

Updated the filenames and updated reftest.list.
Attachment #619757 - Attachment is obsolete: true
(Assignee)

Comment 2

5 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

5 years ago
Created attachment 619852 [details] [diff] [review]
Patch
Assignee: nobody → smontagu
Attachment #619852 - Flags: review?(dholbert)
(Assignee)

Comment 4

5 years ago
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.
Attachment #619759 - Attachment is obsolete: true
Attachment #619853 - Flags: review?(dholbert)

Comment 5

5 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;
}
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox13: ? → +
tracking-firefox14: ? → +

Comment 6

5 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 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+
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
Ah, the reftest files are in /reftests/bidi but the tweaked reftest.list is in /reftests/bugs/
(Assignee)

Comment 13

5 years ago
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!
(Assignee)

Comment 15

5 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
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.
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

5 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.
status-firefox13: affected → ---
status-firefox14: affected → ---
tracking-firefox13: + → ?
tracking-firefox14: + → ?
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.
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Comment 23

5 years ago
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
Attachment #621928 - Flags: review?(dholbert)
Attachment #621928 - Flags: review?(dholbert) → review+
(Assignee)

Comment 24

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/81535cbb76bc
https://hg.mozilla.org/mozilla-central/rev/81535cbb76bc

Comment 26

5 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.