932296 - Fix and re-enable layout/generic/test/test_bug448987.html

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ed Morley [:emorley]]

layout/generic/test/test_bug448987.html is now failing with "Too often tried to focus image map, giving up", eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29835269&tree=Mozilla-Inbound
{
06:29:29     INFO -  211 INFO TEST-INFO | /tests/layout/generic/test/test_bug448987.html | must wait for load
06:29:29     INFO -  ++DOMWINDOW == 52 (0x11dda9818) [serial = 85] [outer = 0x11162f5f8]
06:29:30     INFO -  --DOCSHELL 0x11d4e46d0 == 11 [id = 11]
06:29:30     INFO -  --DOMWINDOW == 51 (0x11ed63548) [serial = 16] [outer = 0x0] [url = http://mochi.test:8888/tests/layout/generic/test/test_bug240933.html]
06:29:30     INFO -  --DOMWINDOW == 50 (0x1116f8668) [serial = 26] [outer = 0x0] [url = http://mochi.test:8888/tests/layout/generic/test/test_bug288789.html]
06:29:30     INFO -  --DOMWINDOW == 49 (0x1288abab8) [serial = 56] [outer = 0x11dd11b48] [url = data:text/html;charset=utf-8,%3Chtml%3E%0A%3Chead%3E%0A%3Cstyle%3E%0Adiv%3A%3Afirst-letter%20%7B%20%7D%0Aspan%3A%3Aafter%20%7B%20content%3A%22before%20text%22%3B%20%7D%0A%3C/style%3E%0A%3C/head%3E%0A%3Cbody%3E%0A%3Cdiv%20style%3D%22position%3A%20fixed%3B%20direction%3A%20rtl%3B%22%3E%0A%20%20%3Cspan%20style%3D%22%20direction%3A%20ltr%3B%20unicode-bidi%3A%20bidi-override%3B%20font-size%3A%2070px%3B%20%22%3E%0A%20%20%20%20%3Cspan%20style%3D%22display%3A%20table%3B%20position%3A%20fixed%3B%22%3E%3C/span%3E%0A%20%20%3C/span%3E%0A%3C/div%3E%0A%3Cscript%3E%0Afunction%20doe%28i%29%7B%0Adocument.documentElement.setAttribute%28%27style%27%2C%20%27%27%29%3B%0A%7D%0AsetTimeout%28doe%2C100%29%3B%0A%3C/script%3E%0A%3C/body%3E%0A%3C/html%3E]
06:29:30     INFO -  --DOMWINDOW == 48 (0x11dd11b48) [serial = 55] [outer = 0x0] [url = data:text/html;charset=utf-8,%3Chtml%3E%0A%3Chead%3E%0A%3Cstyle%3E%0Adiv%3A%3Afirst-letter%20%7B%20%7D%0Aspan%3A%3Aafter%20%7B%20content%3A%22before%20text%22%3B%20%7D%0A%3C/style%3E%0A%3C/head%3E%0A%3Cbody%3E%0A%3Cdiv%20style%3D%22position%3A%20fixed%3B%20direction%3A%20rtl%3B%22%3E%0A%20%20%3Cspan%20style%3D%22%20direction%3A%20ltr%3B%20unicode-bidi%3A%20bidi-override%3B%20font-size%3A%2070px%3B%20%22%3E%0A%20%20%20%20%3Cspan%20style%3D%22display%3A%20table%3B%20position%3A%20fixed%3B%22%3E%3C/span%3E%0A%20%20%3C/span%3E%0A%3C/div%3E%0A%3Cscript%3E%0Afunction%20doe%28i%29%7B%0Adocument.documentElement.setAttribute%28%27style%27%2C%20%27%27%29%3B%0A%7D%0AsetTimeout%28doe%2C100%29%3B%0A%3C/script%3E%0A%3C/body%3E%0A%3C/html%3E]
06:29:31     INFO -  212 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  213 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  214 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  215 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  216 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  217 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
06:29:31     INFO -  218 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_bug448987.html | Too often tried to focus image map, giving up
06:29:31     INFO -  JavaScript error: http://mochi.test:8888/tests/layout/generic/test/file_bug448987.html, line 36: parent.Simpletest is undefined
}

...after this landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d14f6329ca

The testing is too sensitive to timing changes and so has been disabled until fixed.

Comment 1

[Ed Morley [:emorley]]

Test disabled in:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3c2060a723

Also in the error case, there is a typo:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/file_bug448987.html?force=1#36
34       if (counter > 10) {
35         parent.ok(false, "Too often tried to focus image map, giving up");
36         parent.Simpletest.finish();
37         return;
38       }

Please s/Simpletest/SimpleTest/ as well as whatever other fixes required, before re-enabling :-)

Comment 2

[Martijn Wargers (dead)]

The timers are necessary until bug 922524 is fixed and I don't know how to fix that :(
Indeed, really bad that this started failing on MacOS X after some mochitests were disabled (bug 921635) on Mac.

Comment 3

[Martijn Wargers (dead)]

Also, did it fail across all MacOS X machines?
It may have something to do with MacOS X accessibility pref of skipping focus of things other than links and form controls. Although, I wouldn't know how this would be giving errors only now, then.

Comment 4

[Ed Morley [:emorley]]

Yeah it failed on all OS X versions:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=66d14f6329ca&jobname=osx

Comment 5

[Martijn Wargers (dead)]

I should add accessibility.tabfocus pref to 7 for the Mac:
http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug238987.html?force=1#34
That might be the reason it is failing consistently now for the Mac. No idea why they would pass beforehand, though.

Comment 6

[Ed Morley [:emorley]]

(In reply to Martijn W[:mwargers] (QA) [PTO 21-27/10] from comment #5)
> That might be the reason it is failing consistently now for the Mac. No idea
> why they would pass beforehand, though.

This test only started only failing when https://hg.mozilla.org/integration/mozilla-inbound/rev/66d14f6329ca landed, which only affected OS X.

Comment 7

[Mats Palmgren (inactive)]

Maybe one of the tests in that changeset changed accessibility.tabfocus without resetting it?

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

This appears to work:
https://tbpl.mozilla.org/?tree=Try&rev=7e04fb452d13

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 824586 [details] [diff] [review]
fix

Thanks, Mats, this looks good, I was also looking on fixing this.
You need to add
         clearInterval(timer);
after the
+        parent.SimpleTest.finish();

Also, the same fix is needed for file_bug448987_ref.html

Comment 10

[Martijn Wargers (dead)]

Possible and likely explanation of what happened:
With the removal of the dom-level tests, the chunking got changed. 
Now test_bug411236.html isn't run before test_bug448987.html anymore, while it used to. And test_bug411236.html contains this accessibility.tabfocus=7 pref code. 
Unsetting the pref to what it was doesn't work correctly, and the accessibility.tabfocus=7 behavior would continue.
I tested this with:
  SpecialPowers.setIntPref("accessibility.tabfocus", 7);
  SpecialPowers.clearUserPref("accessibility.tabfocus");
After the clearUserPref, the test would still pass, meaning the old accessibility.tabfocus=7 behavior continues.

Perhaps bug 343600 is related to this?
In any case, here are some mxr links that seems the cause of this problem:
Inside  nsFocusManager::DetermineElementToMoveFocus
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2340
nsIContent::sTabFocusModel is being set from LookAndFeel::GetInt
Here the 'listen for pref changes' code is called (Registercallback):
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsXPLookAndFeel.cpp#436
And apparently that doesn't handle removal of preferences, which would be related to bug 343600 then.

Comment 11

[Mats Palmgren (inactive)]

Attachment: fix, v2

(In reply to Martijn W[:mwargers] (QA) [PTO 21-27/10] from comment #9)

Good catch, thanks.

https://tbpl.mozilla.org/?tree=Try&rev=30b74441bb20

Comment 12

[Mats Palmgren (inactive)]

(In reply to Martijn W[:mwargers] (QA) [PTO 21-27/10] from comment #10)
> Unsetting the pref to what it was doesn't work correctly

Please file a bug about this.

Clearly DetermineElementToMoveFocus needs to take the pref into account
for example.  The OSX system pref should only be used when accessibility.tabfocus
is unset according to the documentation:
https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Preference_reference/accessibility.tabfocus

Comment 13

[Martijn Wargers (dead)]

Comment on attachment 824619 [details] [diff] [review]
fix, v2

Looks good, thanks!

Comment 14

[Martijn Wargers (dead)]

(In reply to Mats Palmgren (:mats) from comment #12)
> (In reply to Martijn W[:mwargers] (QA) [PTO 21-27/10] from comment #10)
> > Unsetting the pref to what it was doesn't work correctly
> 
> Please file a bug about this.

Ok, filed bug 932814.

Comment 15

[Ed Morley [:emorley]]

Thank you for figuring this out! :-)

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/5c3c2060a723