Bug 1055665 will land without tests for ruby-position: left/right since there are still problems with interaction between ruby and vertical text.

Could you explain what you're changing here? It looks like this is doing quite a bit more than just enabling a test (per this bug's original summary). That's not a problem; I just want to better-understand what's changing & why. Also, FWIW: with this patch applied and with the ruby & vertical-text prefs enabled, the "ruby-position-vertical-lr.html" test still doesn't quite pass, on my Linux Desktop. In the testcase, it looks like base, left, and left2 are all 1px to the right of where they're placed in the reference case.

(though maybe there's just another patch somewhere that needs to be applied to fix that failure? I was testing with this patch applied directly on top of a mozilla-central build from yesterday.)

Comment on attachment 8544387 [details] [diff] [review] patch Well, actually I just wanted to find a way to make those tests work even after bug 1115249 lands. (the current tests rely on the "line-height: normal", but according to the spec, RTCs always have "line-height: 1".) I found this change fix the test on Windows, and I guess it also fixes it on other platforms, so I enabled it and put the patch here. Now my guess is proven to be false, so let's put it off. jfkthame told me that he has pushed some patches about vertical text to m-i today, which might have influence on this test. I'll check it tomorrow to see if it works then.

This patchset may not work without bug 1132008 landed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cfbb58e27eb Well, they still cannot pass in Mac and... Windows (why? I tested the patch on my Windows, and they pass here...)

Comment on attachment 8565157 [details] [diff] [review] patch 1 - Make css-ruby reftest utils writing-mode aware >+++ b/layout/reftests/css-ruby/utils.js >@@ -1,9 +1,17 @@ >-function getHeight(elem) { >- return elem.getBoundingClientRect().height + 'px'; >+function getBlockAxisName(elem) { >+ var wm = getComputedStyle(elem).writingMode; >+ return !wm || wm == 'horizontal-tb' ? 'height' : 'width'; IMO, ternary conditions are easier to read if you put parens around the boolean condition, e.g. return (!wm || wm == 'horizontal-tb') ? 'height' : 'width'; Otherwise, human readers may visually parse this return statement as something like... return !wm || [some-other-expression]; ...which is not what you're going for. (and then they have to re-read it once they notice the "?" and realize what's going on). r=me with those parens

Comment on attachment 8565158 [details] [diff] [review] patch 2 - Fix the reftests >+++ b/layout/reftests/css-ruby/ruby-position-horizontal-ref.html >@@ -1,32 +1,34 @@ >- &nbsp; <!-- to give container a nonzero size for >- percent values to resolve against --> [...] >+ <span>&nbsp;</span> I think the <!-- --> comment is worth keeping -- otherwise it's unclear what the point of the &nbsp; is here. >+ <script type="text/javascript"> >+ makeBSizeOfParentMatch(document.getElementsByTagName('span')); >+ </script> I don't understand why you're adding this / what's going on here. Could you explain? >+++ b/layout/reftests/css-ruby/utils.js >+function makeBSizeOfParentMatch(elems) { >+ for (var elem of elems) >+ elem.dataset.bsize = getBSize(elem); >+ for (var elem of elems) >+ setBSize(elem.parentNode, elem.dataset.bsize); >+} (Why the two loops & "dataset" usage here? Why not just a single loop with setBSize(elem.parentNode, getBSize(elem)); ?)

(In reply to Daniel Holbert [:dholbert] from comment #9) > Comment on attachment 8565158 [details] [diff] [review] > patch 2 - Fix the reftests > > >+++ b/layout/reftests/css-ruby/utils.js > >+function makeBSizeOfParentMatch(elems) { > >+ for (var elem of elems) > >+ elem.dataset.bsize = getBSize(elem); > >+ for (var elem of elems) > >+ setBSize(elem.parentNode, elem.dataset.bsize); > >+} > > (Why the two loops & "dataset" usage here? Why not just a single loop with > setBSize(elem.parentNode, getBSize(elem)); > ?) Because setting size and reading bounding rect will trigger reflow in every pass.

Ahh, of course; nice. Could you add a comment to briefly explain that that's why we're doing this? (Still not clear on why we need the makeBSizeOfParentMatch() call in the first place, too. It's probably obvious with some context, but I'm just missing that context, not having looked at these tests in a while.)

Yeah, I'll add more detailed comments to the tests soon :)

Comment on attachment 8565158 [details] [diff] [review] patch 2 - Fix the reftests [canceling review, since there's more coming here, so that my review queue reflect reality]

It seems that there is a bug in the impl which cause the failures on the reftests.

With bug 1136557 fixed, it seems OS X and Windows are also happy now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b2689e3edb

https://hg.mozilla.org/integration/mozilla-inbound/rev/67bd29e2759c https://hg.mozilla.org/integration/mozilla-inbound/rev/3e826cb4e941

https://hg.mozilla.org/mozilla-central/rev/67bd29e2759c https://hg.mozilla.org/mozilla-central/rev/3e826cb4e941