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.

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.

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.

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.

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

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

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.

(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..."

(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

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.

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.

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

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

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

(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 :)

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

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.

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.

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

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