Closed Bug 1190663 Opened 9 years ago Closed 9 years ago

Implement window.close() for browser action popup HTML

Categories

(WebExtensions :: Untriaged, defect, P2)

34 Branch
defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: billm, Assigned: kmag)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

No description provided.
Priority: -- → P2
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Blocks: webext
Flags: blocking-webextensions+
Also affects pageAction popup.html: > Scripts may not close windows that were not opened by script.
Assignee: nobody → kmaglione+bmo
This should have been fixed by bug 1217129, but it still needs tests.
Depends on: 1217129
Whiteboard: triaged
Iteration: --- → 47.3 - Mar 7
Attachment #8724493 - Flags: review?(lgreco)
Comment on attachment 8724493 [details] MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl https://reviewboard.mozilla.org/r/37047/#review33677 If I'm not wrong, this updated test case is supposed to fail if the popup is not closed by the "window.close()" called from inside the popup extension page. I tried to comment out the "window.close()" to ensure that the test fails when it should, but all the test assertions passes successfully even when that line is commented out, and so I tried to find where the popup is being closed before the "window.close()" has been called. Follows some notes about the lines that seem to be related the issue described above: ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:135 (Diff revision 1) > - yield closeBrowserAction(extension); > + yield closeBrowserAction(extension); This line seems to close the browserAction's popup immediatelly after the test case which opens the popup, and so the popup is never closed by the "window.close()". ::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:143 (Diff revision 1) > panel.hidePopup(); Same here, but for the pageAction popups: This line seems to always hide the popup after the test case which opens it, and so the popup is never closed by the "window.close()" as supposed by this updated version of the test case.
Comment on attachment 8724493 [details] MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37047/diff/1-2/
Attachment #8724493 - Flags: review?(lgreco)
https://reviewboard.mozilla.org/r/37047/#review33677 Good catch. I should have checked that the tests failed without calling close().
Attachment #8724493 - Flags: review?(lgreco)
Comment on attachment 8724493 [details] MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl https://reviewboard.mozilla.org/r/37047/#review33919 Besides a small nit, the "browser_ext_browserAction_popup.js" test file logs an error when it runs twice in a row (which, even if the test passes when it happens, is probably something worth to check) ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:5 (Diff revisions 1 - 2) > function* testInArea(area) { If we try to run "browser_ext_browserAction_popup.js" twice in a row (e.g. by running it with "./mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_popup.js --repeat 2"), the following error is logged: JavaScript error: chrome://browser/content/ext-utils.js, line 303: TypeError: this.browser is null The error is raised from the popup method |resizeBrowser|, which has been called at the start of the second run of the test case (and this.browser is null). On the contrary, this doesn't happen if we try to run with "--repeat 2" the pageAction popup tests. ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup.js:17 (Diff revisions 1 - 2) > "popup-a.html": `<script src="popup-a.js"></script>`, Nit: in the browserAction popup test cases, we should define the extension html pages using the same technique used in the pageAction popup tests: let scriptPage = url => \\\`<html><head><meta charset="utf-8"><script src="$\\\{url\\\}"></script></head></html>\\\`; ... files: \\\{ "pupup-a.html": scriptPage("popup-a.js"), ... to reduce noise related to the logged console messages related to the missing page charset definition.
https://reviewboard.mozilla.org/r/37047/#review33919 > If we try to run "browser_ext_browserAction_popup.js" twice in a row (e.g. by running it with "./mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_popup.js --repeat 2"), the following error is logged: > > JavaScript error: chrome://browser/content/ext-utils.js, line 303: TypeError: this.browser is null > > The error is raised from the popup method |resizeBrowser|, which has been called at the start of the second run of the test case (and this.browser is null). > > On the contrary, this doesn't happen if we try to run with "--repeat 2" the pageAction popup tests. That error isn't new. It happens because, in those cases, we close the popup so clickly that the callback to resize the browser happens after it's already closed. It's not exactly ideal, but it's also not really an issue.
Comment on attachment 8724493 [details] MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37047/diff/2-3/
Attachment #8724493 - Flags: review?(lgreco)
Comment on attachment 8724493 [details] MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl https://reviewboard.mozilla.org/r/37047/#review34145
Attachment #8724493 - Flags: review?(lgreco) → review+
https://hg.mozilla.org/integration/fx-team/rev/09d8e3caf51daf1554673799609b575de4383a02 Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl
https://hg.mozilla.org/integration/fx-team/rev/38d23f0168980b27bb88bf20a8cba7f907bc495e Backed out changeset 09d8e3caf51d (bug 1190663) for intermittent mochitest failures on Windows
https://hg.mozilla.org/integration/fx-team/rev/afa990f7034a12b78e1cba02b532a254c2d1990f Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
I've added a bit on using window.close() in the intro docs for browser actions: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Anatomy_of_a_WebExtension#Browser_actions_2
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: