1150614 - Position floats correctly in vertical writing modes with dir=rtl

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[Simon Montagu :smontagu]

I made vertical-writing-mode versions of the float-in-rtl* tests from bug 1114329. The positioning of the floats fails in all the tests.

Comment 1

[Simon Montagu :smontagu]

Attachment: The tests

I only intend to fix the issues with floats here, so I marked the tests with dir="rtl" on the body as failing: they have issues with placing the text outside the floats, which is part of bug 1131451.

The tests with -moz-fit-content also depend on bug 1122253, so perhaps I should mark them as failing too for now.

Comment 2

[Simon Montagu :smontagu]

Attachment: The patch

Similar to bug 1131013, this fixes up the positioning ad hoc for floats pending a general solution to bug 1131451; it adds support for dir=rtl on the assumption that line-left in vertical writing modes is always the top and line-right is always the bottom, ignoring sideways-left for now.

Comment 3

[Simon Montagu :smontagu]

FTR, the tests all assert with attachment 8581545 [details] [diff] [review] from bug 1145218; with attachment 8580753 [details] [diff] [review] they assert and fail less drastically than with neither patch.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #1)
> The tests with -moz-fit-content also depend on bug 1122253, so perhaps I
> should mark them as failing too for now.

Yes, with a comment pointing to the relevant bug.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #3)
> FTR, the tests all assert with attachment 8581545 [details] [diff] [review]
> from bug 1145218; with attachment 8580753 [details] [diff] [review] they
> assert and fail less drastically than with neither patch.

I'm not seeing any assertions when I run these tests locally (with a post-bug 1145218 tree).

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8587540 [details] [diff] [review]
The patch

Review of attachment 8587540 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK for now, until we have more complete bidi/vertical support.

::: layout/generic/nsBlockReflowState.cpp
@@ +831,5 @@
>    // convert the result back into this block's writing mode.
>    LogicalPoint floatPos(wm);
> +  bool leftFloat = NS_STYLE_FLOAT_LEFT == floatDisplay->mFloats;
> +  bool ltr = wm.IsBidiLTR();
> +  bool vertical = wm.IsVertical();

No need for the separate |ltr| and |vertical| bools, just write wm.Is...() directly in the conditionals below.

Comment 7

[Jonathan Kew [:jfkthame]]

BTW, on a try push that included these patches, I got "unexpected passes" on Android for a bunch of the new tests; see https://treeherder.mozilla.org/#/jobs?repo=try&revision=140c47a6dc0c. However, I suspect this may be because the reference files are failing to render correctly there.

Comment 8

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Simon Montagu :smontagu from comment #3)
> > FTR, the tests all assert with attachment 8581545 [details] [diff] [review]
> > from bug 1145218; with attachment 8580753 [details] [diff] [review] they
> > assert and fail less drastically than with neither patch.
> 
> I'm not seeing any assertions when I run these tests locally (with a
> post-bug 1145218 tree).

Sorry, that was a typo: comment 3 should say "with attachment 8580753 [details] [diff] [review] they no longer assert, and fail less..."

Comment 9

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> BTW, on a try push that included these patches, I got "unexpected passes" on
> Android for a bunch of the new tests; see
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=140c47a6dc0c.
> However, I suspect this may be because the reference files are failing to
> render correctly there.

Thanks for the heads-up, I'll look into this

Comment 10

[Simon Montagu :smontagu]

Attachment: UNEXPECTED PASS rendering on Android

I'm not sure what is causing this, nor why it only happens on certain tests. The bottom of the image seems to be cut off, causing the unexpected pass because the failure is hidden.

Comment 11

[Simon Montagu :smontagu]

Hmm, from a small sample it looks as if the bottom of *all* reftests is cut off on this version of Android, which means that this would happen with any reftest where the action is at the bottom. This is likely to happen more often with tests for vertical writing modes with dir="rtl". In the meanwhile I can put these tests in a containing div with restricted height.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6bd5a0a047d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bbe38dcb529

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the very-likely cause of B2G reftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d14970cb2b

https://treeherder.mozilla.org/logviewer.html#?job_id=9439906&repo=mozilla-inbound

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Also confirmed to be the cause of frequent linux64 e10s mochitest-bc failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=9449429&repo=mozilla-inbound

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14)
> Also confirmed to be the cause of frequent linux64 e10s mochitest-bc
> failures.
> https://treeherder.mozilla.org/logviewer.html#?job_id=9449429&repo=mozilla-
> inbound

Argh, or not. Literally the last result to come in on the retriggers post-backout hit this failure. So you're off the hook for this one :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Backed out for being the very-likely cause of B2G reftest failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d14970cb2b

Definitely green post-backout.

Comment 17

[Simon Montagu :smontagu]

Attachment: Add fuzz to tests from bug 383883

I'm unsure why these reftests started failing, since the patch shouldn't have any effect on floats in non-vertical writing modes. I thought that perhaps since the tests don't use reftest-wait they might be a little unstable, but adding reftest-wait handling didn't help. So let's just fuzz.

Comment 18

[Jonathan Kew [:jfkthame]]

Although the patch has no effect on the meaning of the code for horizontal mode, it must have altered timing in some way, such that the b2g test usually (though not invariably: there was a green run on cset e9735c142acc during the time this was landed) catches some residue from the testcase as rendered before the onload() handler's changes have taken effect. Maybe it's as subtle as causing an additional cache miss during FlowAndPlaceFloat, such that it takes a fraction longer.

The failure also indicates that we don't quite have an adequate visual-overflow area on b2g textframes to account for possible antialiasing, but that's a separate (and very minor) issue.

We could probably work around the problem by altering the text so that it doesn't end with a problematic glyph like "f", or by adding some padding, but all this is just hackery. Fuzzing it will do just as well for now.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2fbd0459bab
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee3bca3fc2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/599a8abf54a3

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2fbd0459bab
https://hg.mozilla.org/mozilla-central/rev/3ee3bca3fc2d
https://hg.mozilla.org/mozilla-central/rev/599a8abf54a3