Closed Bug 1112474 Opened 5 years ago Closed 5 years ago

Fix ruby-position reftests and enable them

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1055665 will land without tests for ruby-position: left/right since there are still problems with interaction between ruby and vertical text.
Blocks: writing-mode
Summary: Test ruby-position: left/right with vertical text → Enable test for css-ruby with vertical text
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → quanxunzhen
Attachment #8544387 - Flags: review?(dholbert)
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.
Attachment #8544387 - Flags: review?(dholbert)
Depends on: 1127162
Summary: Enable test for css-ruby with vertical text → Fix ruby-position reftests and enable them
Depends on: 1133573
Depends on: 1132008
Attached patch patch 2 - Fix the reftests (obsolete) — Splinter Review
This patchset may not work without bug 1132008 landed.
Attachment #8544387 - Attachment is obsolete: true
Attachment #8565158 - Flags: review?(dholbert)
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
Attachment #8565157 - Flags: review?(dholbert) → review+
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]
Attachment #8565158 - Flags: review?(dholbert) → feedback+
It seems that there is a bug in the impl which cause the failures on the reftests.
Depends on: 1136557
Attachment #8565158 - Attachment is obsolete: true
Attachment #8579097 - Flags: review?(dholbert)
With bug 1136557 fixed, it seems OS X and Windows are also happy now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7b2689e3edb
You need to log in before you can comment on or make changes to this bug.