918258 - Convert reftests 448987.html and 449653-1.html to mochitests

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/448987-ref.html
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/448987.html
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/449653-1.html
contain enablePrivilege code.

This doesn't work in reftest, so those tests are disabled:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/reftest.list#1170

Those tests should be rewritten to use the reftest feature in mochitest, like this one, for instance:
http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug332655-1.html?force=1

I guess both tests should be moved under /layout/generic/test/.

Comment 1

[Martijn Wargers (dead)]

448987.html should be made like:
http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_bug332655-1.html?force=1
and 449653-1.html should be made like: 
http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug579767.html?force=1#10

Comment 2

[Martijn Wargers (dead)]

Attachment: 918258.diff

I've done it so that the test and reference files are loaded into iframes and then things are compared.

After applying patch (why doesn't patch handle this for me?):
hg add layout/generic/test/test_bug448987.html layout/generic/test/test_bug449653.html
hg rename layout/reftests/bugs/448987-ref.html layout/generic/test/file_bug448987_ref.html
hg rename layout/reftests/bugs/448987.html layout/generic/test/file_bug448987.html
hg rename layout/reftests/bugs/449653-1-ref.html layout/generic/test/file_bug449653_1_ref.html
hg rename layout/reftests/bugs/449653-1.html layout/generic/test/file_bug449653_1.html

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #2)
> After applying patch (why doesn't patch handle this for me?):

("hg qimport" is what you want, I think? From skimming the patch, it looks like it captured the file renames in a way that hg qimport should understand (although "patch" won't), e.g. your attached patch has:
> rename from layout/reftests/bugs/448987.html
> rename to layout/generic/test/file_bug448987.html
which hg qimport should understand.)

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 807174 [details] [diff] [review]
918258.diff

>diff --git a/layout/reftests/bugs/448987.html b/layout/generic/test/file_bug448987.html
[...]
>+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
[...]
>-function synthesizeKey(aKey, aEvent, aWindow)
>-{
[...]
> function focus_area() {
>   document.getElementById("pre").focus();
>-  synthesizeKey("VK_TAB",{},window)
>+  synthesizeKey("VK_TAB", { }, window);

Invoke this as "EventUtils.synthesizeKey", so it's clear where it's defined. (An MXR search shows that a good chunk of our other tests do this.)

>diff --git a/layout/reftests/bugs/448987-ref.html b/layout/generic/test/file_bug448987_ref.html
> function focus_area() {
>   document.getElementById("pre").focus();
>-  synthesizeKey("VK_TAB",{},window)
>+  synthesizeKey("VK_TAB", { }, window);
>+  parent.endTest();

[here, too, in the reference case]

Also: so it looks like the "tab" is sent in *both* the reference case and the testcase, to trigger a focus outline to be painted.  But there's no real verification that this actually works. (i.e. if the focus outline fails to be drawn for some reason, or it's drawn in the parent outside of the range of our snapshot, then the testcase and reference will still match and we'll still pass even though we're not testing the thing we're intending to test.)

To make sure the outline is actually being drawn as-expected, please make one more copy of the reference case, but with *no* synthesizeKey (so, no focus-outline), and load that in a third iframe and assert that it does *not* match the testcase's snapshot.  (Call it e.g. file_bug448987_notref.html)

(Sorry to put this burden on you - I know you didn't write the test, but as long as we're enabling it, I want to re-enable it with a high enough degree of confidence that it's actually testing what it's supposed to be test).

>diff --git a/layout/reftests/bugs/449653-1.html b/layout/generic/test/file_bug449653_1.html
>rename from layout/reftests/bugs/449653-1.html
>rename to layout/generic/test/file_bug449653_1.html
>--- a/layout/reftests/bugs/449653-1.html
>+++ b/layout/generic/test/file_bug449653_1.html
> </script>
> </body></html>
>\ No newline at end of file

Might as well add a newline at the end of this file, while you're here.

>diff --git a/layout/generic/test/test_bug448987.html b/layout/generic/test/test_bug448987.html
[...]
>+function endTest() {
>+  ok(compareSnapshots(snapshotf1,
>+                      snapshotWindow(f2.contentWindow), true)[0],
>+     "area shape=default> should render focus outline");

You're missing the opening "<" in "<area" in the message there.

>diff --git a/layout/generic/test/test_bug449653.html b/layout/generic/test/test_bug449653.html
>+<iframe src="file_bug449653_1.html" id="f1"></iframe>
>+<iframe id="f2" src="file_bug449653_1_ref.html"></iframe>

In the first iframe there, put the "id" attribute first, to match the second one and make this more readable.  (Right now, the two iframes have those attributes in opposite orders, which is odd.)

r=me with that, assuming the tests pass. :)

Comment 5

[Daniel Holbert [:dholbert]]

ALSO: The first mochitest needs to start its actions inside of SimpleTest.waitForFocus(), as described here:
  https://developer.mozilla.org/en-US/docs/Mozilla/QA/Avoiding_intermittent_oranges#Tests_which_require_focus

With that additional level of indirection, I think you probably want to start that mochitest out with all of its iframes having an empty "src" attribute, and then set the "src" attribute of the first one in your waitForFocus callback. (and then let it load & fire "startTest" as it currently does, though at that point you probably want to rename "startTest" to "firstIframeLoaded" since now it'll no longer really be the start of the test)

Comment 6

[Martijn Wargers (dead)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (Sorry to put this burden on you - I know you didn't write the test, but as
> long as we're enabling it, I want to re-enable it with a high enough degree
> of confidence that it's actually testing what it's supposed to be test).

Np, thanks for your help!

Comment 7

[Martijn Wargers (dead)]

Attachment: 918258.diff

Ok, I think I've followed all your suggestions.
The only problem is now that test_bug448987 is failing.

It seems that tabbing into image maps is impossible.

Comment 8

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> It seems that tabbing into image maps is impossible.

And that seems like a bug (regression from bug 448987) to me, or not?

Comment 9

[Mats Palmgren (inactive)]

Tabbing in layout/reftests/bugs/448987.html seems to work fine for me in Linux Nightly.
Perhaps the mochitest needs SimpleTest.waitForFocus?

Comment 10

[Martijn Wargers (dead)]

Ok, it indeed seems to work fine under Linux (in a VM).
However, on my Macbook Pro, it doesn't seem to work.
I tested this url this time: http://www.december.com/html/demo/imagemap.html

Comment 11

[Mats Palmgren (inactive)]

Try with "Full Keyboard Access" enabled in your OSX system preferences.
(need to restart Firefox after changing that I think)

Comment 12

[Mats Palmgren (inactive)]

We should probably just #ifdef out this test on OSX in the Makefile for that reason.
I don't know whether our test machines runs with that pref or not.

Comment 13

[Martijn Wargers (dead)]

Yeah, "Full Keyboard Access" makes it work on the Mac. Unfortunately, I tried to update my build and am now stuck trying to update Python, so can't try the patch, currently.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 807733 [details] [diff] [review]
918258.diff

>+++ b/layout/generic/test/test_bug448987.html
[...]
>+function firstIframeLoaded() {
>+  snapshotf1 = snapshotWindow(f1.contentWindow);
>+  f2.src="file_bug448987_ref.html";
>+}
>+
>+function secondIframeLoaded() {
>+  ok(compareSnapshots(snapshotf1,
>+                      snapshotWindow(f2.contentWindow), true)[0],
>+     "<area shape=default> should render focus outline");
>+  f3.src="file_bug448987_notref.html";
>+}
>+
>+function endTest() {
>+  ok(compareSnapshots(snapshotf1,
>+                      snapshotWindow(f3.contentWindow), false)[0],
>+     "file_bug448987.html should render focus outline, file_bug448987_notref.html should not");
>+  SimpleTest.finish();

nit: seems like it mike make sense to name this "thirdIframeLoaded" rather than "endTest()", for consistency, but I guess it doesn't really matter.

r=me, with the mac failure sorted out (e.g. disabling the test on mac if necessary)

Note that you can't simply add "#ifdef" wrappers around the added lines in the Makefile, though -- I believe that will prematurely terminate the list. You need a separate section, along the lines of this:
http://mxr.mozilla.org/mozilla-central/source/content/canvas/test/Makefile.in?rev=45097bc3a578&mark=137-142#137

Comment 15

[Martijn Wargers (dead)]

Attachment: 918258.diff

I had to add timers in the iframes, right after the pre.focus call, like this:
+  document.getElementById("pre").focus();
+  setTimeout(next, 0);
 }

Without it, area in the image map wouldn't get the focus outline, but the image map itself. It's luck that I found this, because the reference iframe file suffered from the same problem. Perhaps an extra check should be added for this?

(In reply to Daniel Holbert [:dholbert] from comment #4)
> [...]
> > function focus_area() {
> >   document.getElementById("pre").focus();
> >-  synthesizeKey("VK_TAB",{},window)
> >+  synthesizeKey("VK_TAB", { }, window);
> 
> Invoke this as "EventUtils.synthesizeKey", so it's clear where it's defined.
> (An MXR search shows that a good chunk of our other tests do this.)

I forgot to comment on this, I tried to use EventUtils.synthesizeKey, but that didn't work. I think EventUtils is only defined in browser-chrome tests or something. But perhaps a good idea to have it in plain mochitests too?

Comment 16

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4d43dfa2164d

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #15)
> > Invoke this as "EventUtils.synthesizeKey", so it's clear where it's defined.
> > (An MXR search shows that a good chunk of our other tests do this.)
> 
> I forgot to comment on this, I tried to use EventUtils.synthesizeKey, but
> that didn't work. I think EventUtils is only defined in browser-chrome tests
> or something. But perhaps a good idea to have it in plain mochitests too?

(Ah, ok - never mind then.)

Comment 18

[Martijn Wargers (dead)]

Ok, tryserver gives this error on various platforms (but it's also passing on the various platforms, so it seems the test could potentially work everyweher):
982 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | <area shape=default> should render focus outline

I investigated this on my Ubuntu VM, something like this makes it work:
function focus_area() {
  document.getElementById("pre").onfocus = function() {
    document.getElementById("pre").onfocus = null;
    document.getElementById("area").onfocus = function() { setTimeout(parent.firstIframeLoaded, 0); }
    setTimeout(function() { synthesizeKey("VK_TAB", { }, window); }, 100); 
  };
  document.getElementById("pre").focus();
}

As you can see, I had to use all kinds of timers. Especially the timer with 100ms is concerning.
I would like to use something more reliable.

Comment 19

[Martijn Wargers (dead)]

[12:26]	<smaug>	mwargers: Enn ^ would know more about focus :)
[12:27]	<smaug>	mwargers: I would debug this by adding some focus listener to window and logging when things are focused
[12:27]	<smaug>	mwargers: also, perhaps worth to check what the document.activeElement is

Btw, unrelated note, the focus outline for image maps look much smaller than regular focus outlines here on my Macbook Pro with Retina display.

Comment 20

[Neil Deakin]

Does it work if you synthesize a mousemove over the image, as the example in http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/window_focus.xul#354 ?

Comment 21

[Martijn Wargers (dead)]

Attachment: 918258.diff

This is trying the tab key multiple times, if it doesn't succeed after 10 times, the test would be failing. This makes the test pass for me here on the linux VM.

(In reply to Neil Deakin from comment #20)
> Does it work if you synthesize a mousemove over the image, as the example in
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/chrome/
> window_focus.xul#354 ?

I tried that, it didn't help at all.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #19)
> [12:27]	<smaug>	mwargers: I would debug this by adding some focus listener
> to window and logging when things are focused

I did that, using window.addEventListener('focus', func, true), etc. When the bug occured, this event wouldn't fire at all, even though I saw the pre element getting focused. That seems unexplainable to me. I noticed when I delay the focus_area() by 500ms, the tests are passing.

Comment 22

[Martijn Wargers (dead)]

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=84f031582b74

Comment 23

[Masatoshi Kimura [:emk]]

Comment on attachment 809985 [details] [diff] [review]
918258.diff

>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/layout/generic/test/file_bug448987.html	Tue Sep 24 15:01:00 2013 +0200

Looks like the rename is still handled incorrectly.

Comment 24

[Daniel Holbert [:dholbert]]

Ah, good catch -- looks like the patch currently deletes the 448987* files and then [separately] adds new files.

Martijn, could you fix that?

You probably want to do something like the following [warning, I haven't tested these exact steps]:
 - qimport your current patch
 - Copy each of your file_448987* files to /tmp 
 - then "hg rm" the in-tree versions of these files
 - hg revert -r -2 layout/reftests/bugs/448987.html
 - hg revert -r -2 layout/reftests/bugs/448987-ref.html
 - hg mv layout/reftests/bugs/448987.html layout/generic/test/file_bug448987.html
 - hg mv layout/reftests/bugs/448987-ref.html layout/generic/test/file_bug448987_ref.html
 - hg cp layout/generic/test/file_bug448987_ref.html layout/generic/test/file_bug448987_notref.html
 - and then copy your "real" versions from /tmp back in
[qref as desired along the way, and at the end]

This should produce a patch that has the same final result, but with actual file-move/copy metadata (and correct hg blame annotations on the 448987 files)

Comment 25

[Martijn Wargers (dead)]

(In reply to Daniel Holbert [:dholbert] from comment #24)

Thanks, I followed these instructions, but it doesn't seem to help, I still get the wrong (old) patch format.

Comment 26

[Martijn Wargers (dead)]

Attachment: 918258.diff

I used the previous patch for this and copied the new content from the patch after to this one for the files file_bug448987.html and file_bug448987_ref.html.

Comment 27

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bf5256b5bfff

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 810465 [details] [diff] [review]
918258.diff

Looks good to me. I just diffed the results of applying this patch vs. applying the previous patch (attachment 809985 [details] [diff] [review]), and they were the same, with some additional cleanup. (looks like you removed an unnecessary id="image" in two spots, and dropped the "snapshotf2" variable; cool.)

One nit: please use SimpleTest.executeSoon(foo) instead of setTimeout(foo, 0). (If that doesn't work in the child frames, you might need to make them do a synchronous call to the parent, which can then do the executeSoon for you.)

The 100ms timeout is concerning, but it's better than having the test disabled (as long as it doesn't make this test an intermittent-fail), so I suppose I'm OK with it.

So: r=me, assuming the test passes.

Comment 29

[Martijn Wargers (dead)]

Attachment: 918258.diff

Ok, I assume you mean this, right?

(In reply to Daniel Holbert [:dholbert] from comment #28)
> The 100ms timeout is concerning, but it's better than having the test
> disabled (as long as it doesn't make this test an intermittent-fail), so I
> suppose I'm OK with it.

I don't like the 100ms setInterval timer, either, I would love to get rid of it, but I don't know how. It seems that the fact that this seems necessary worth filing a new bug for.
Regarding the risk of intermittent-failure. I could let this test rerun on tryserver 10 times, for instance.

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 811956 [details] [diff] [review]
918258.diff

Looks good to me.

>+++ b/layout/generic/test/file_bug448987.html
[...]
>+function focus_area() {
>+  document.getElementById("pre").onfocus = function() {
>+    document.getElementById("pre").onfocus = null;
>+    document.getElementById("area").onfocus = function() { 
>+      clearInterval(timer);
>+      
>+      parent.SimpleTest.executeSoon(parent.firstIframeLoaded, 0);

nit: Drop whitespace-characters-on-blank-line, on the line before executeSoon there. (Or just drop the blank line altogether.)

> I don't like the 100ms setInterval timer, either, I would love to get rid of
> it, but I don't know how. It seems that the fact that this seems necessary
> worth filing a new bug for.

Probably, yeah. Can you at least include an //XXX comment above the setInterval explaining why we need it?

I'm OK with it, as long as it doesn't trigger intermittent failures, on the philosophy that a hacky test is a step up from a completely-disabled test.

> Regarding the risk of intermittent-failure. I could let this test rerun on
> tryserver 10 times, for instance.

Sure (though more than 10 would probably be good).  Maybe do an all-platforms mochitests-only Try run, and then mash the "+" icon to retrigger a bunch more test runs on some platform where we've got test-cycles to spare (i.e. probably not mac).

Comment 31

[Martijn Wargers (dead)]

Attachment: 918258.diff (for check-in)

Ok, fixed the nit, I filed a new bug about this issue, bug 922524, and added a comment which mentions this issue and that bug nr.

Comment 32

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fdc61fad1ce1

Comment 33

[Martijn Wargers (dead)]

I let the test rerun 10 times on Linux x64 Opt and on Windows 8 Opt and both runs are all green.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8787963a438

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8787963a438