Make browser_quit_disabled.js more robust
Categories
(Firefox :: Keyboard Navigation, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox88 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
I realized that we wouldn't notice if the quit-application-requested
observer wasn't called.
Assignee | ||
Comment 1•3 years ago
|
||
I realized that we wouldn't notice if the quit-application-requested observer wasn't called.
Updated•3 years ago
|
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b17481ba70ba Make browser_quit_disabled.js more robust. r=Gijs
Comment 3•3 years ago
|
||
Backed out for causing bc failures in browser_quit_disabled
Backout link: https://hg.mozilla.org/integration/autoland/rev/ea98b384468e40aba13b07c717d02d88bc0b2f40
Failure log:https://treeherder.mozilla.org/logviewer?job_id=329988007&repo=autoland&lineNumber=6049
"INFO - TEST-START | browser/components/tests/browser/browser_quit_disabled.js
[task 2021-02-15T11:40:27.196Z] 11:40:27 INFO - TEST-INFO | started process screencapture
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - TEST-INFO | screencapture: exit 0
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - Buffered messages logged at 11:40:26
[task 2021-02-15T11:40:27.247Z] 11:40:27 INFO - Entering test bound test_appMenu_quit_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - TEST-PASS | browser/components/tests/browser/browser_quit_disabled.js | No quit button with shortcut key -
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Leaving test bound test_appMenu_quit_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Entering test bound test_quit_shortcut_disabled
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Buffered messages finished
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - TEST-UNEXPECTED-FAIL | browser/components/tests/browser/browser_quit_disabled.js | Expected quit state - Got false, expected true
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - Stack trace:
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - chrome://mochikit/content/browser-test.js:test_is:1351
[task 2021-02-15T11:40:27.248Z] 11:40:27 INFO - chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:testQuitShortcut:52
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - TEST-PASS | browser/components/tests/browser/browser_quit_disabled.js | Expected quit state -
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - Leaving test bound test_quit_shortcut_disabled
[task 2021-02-15T11:40:27.562Z] 11:40:27 INFO - GECKO(1852) | MEMORY STAT | vsize 7053MB | residentFast 334MB | heapAllocated 154MB
[task 2021-02-15T11:40:27.563Z] 11:40:27 INFO - TEST-OK | browser/components/tests/browser/browser_quit_disabled.js | took 1405ms
[task 2021-02-15T11:40:27.565Z] 11:40:27 INFO - checking window state
[task 2021-02-15T11:40:27.670Z] 11:40:27 INFO - TEST-START | browser/components/tests/browser/browser_startup_homepage.js"
Assignee | ||
Comment 4•3 years ago
|
||
Seems like the test only fails on macOS. I won't be able to debug this. However :arai told me before that the pref worked for them. So this might just be a test problem.
Assignee | ||
Comment 5•3 years ago
|
||
Would you mind taking a look? I don't own any Apple hardware.
Comment 6•3 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #5)
Would you mind taking a look? I don't own any Apple hardware.
I have a mac, and I can reproduce. I tried replacing the synthesizeKey
with synthesizeNativeKey
with the associated magic incantations:
if (AppConstants.platform == "macosx") {
EventUtils.synthesizeNativeKey({
name: "US",
Mac: 0,
Win: 0x00000409,
hasAltGrOnWin: false,
}, 0x0c /* MAC_VK_ANSI_Q */, {metaKey: 1}, "q", "q", null, win);
} else {
EventUtils.synthesizeKey("q", modifiers, win);
}
(don't mind the formatting)
The good news is that fixes the issue, the bad news is it breaks the test the other way, ie the quit shortcut "works" when it shouldn't. This is because the code in the hidden window hasn't run, and macOS, when failing to find a workable shortcut in the window against which we run it, tries the no-window shortcuts (this is probably a factor of our cocoa integration, but anyway). So it tries to quit anyway:
Unexpected Results
------------------
browser/components/tests/browser/browser_quit_disabled.js
FAIL Quit shortcut should NOT have worked -
Stack trace:
chrome://mochikit/content/browser-test.js:test_ok:1323
chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:observe:32
chrome://global/content/globalOverlay.js:canQuitApplication:50
chrome://global/content/globalOverlay.js:goQuitApplication:65
chrome://browser/content/hiddenWindowMac.xhtml:oncommand:1
FAIL Expected quit state - Got true, expected false
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1351
chrome://mochitests/content/browser/browser/components/tests/browser/browser_quit_disabled.js:testQuitShortcut:61
At this point I would suggest re-landing with the test disabled on macOS. Alternatively, you could make the mac hidden window code use a pref observer to enable/disable the shortcut and then things should work...
Assignee | ||
Comment 7•3 years ago
|
||
I have to admit I don't really understand the mac hidden window stuff. I've heard of it, but it seems really strange. So does that mean browser.quitShortcut.disabled=true
sometimes doesn't disable the shortcut on mac? Or is this a test only problem?
Comment 8•3 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #7)
I have to admit I don't really understand the mac hidden window stuff. I've heard of it, but it seems really strange. So does that mean
browser.quitShortcut.disabled=true
sometimes doesn't disable the shortcut on mac? Or is this a test only problem?
Well, the pref is only checked once for each window. So the new window being created after the pref has changed gets the effect. But the hidden window (which is created early during startup) reads the initial value of the pref, before the test changes it, and thus doesn't have the shortcut removed.
If you made the pref's effect "fully restartless", as it were (so you could toggle it and existing windows would update), then I suspect you could test it. But that's a bunch more work...
Assignee | ||
Comment 9•3 years ago
|
||
Thanks for the explanation. Making the pref "fully restartless" would involve having some way to actually going back from disable to enabled. That seems like a bunch of work, as you said, for something that we probably don't need to support for normal usage. I am fine with disabling the test on macOS.
Comment 10•3 years ago
|
||
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/autoland/rev/900e66a5ebae Make browser_quit_disabled.js more robust. r=Gijs
Comment 11•3 years ago
|
||
bugherder |
Description
•