Implement window.close() for browser action popup HTML

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: kmag)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

34 Branch
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Priority: -- → P2
(Reporter)

Updated

2 years ago
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Keywords: dev-doc-needed

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Flags: blocking-webextensions+

Comment 1

2 years ago
Also affects pageAction popup.html:

> Scripts may not close windows that were not opened by script.
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Updated

2 years ago
Blocks: 1237377
(Assignee)

Comment 2

a year ago
This should have been fixed by bug 1217129, but it still needs tests.
Depends on: 1217129

Updated

a year ago
Whiteboard: triaged
(Assignee)

Updated

a year ago
Iteration: --- → 47.3 - Mar 7
(Assignee)

Comment 3

a year ago
Created attachment 8724493 [details]
MozReview Request: Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r?rpl

Review commit: https://reviewboard.mozilla.org/r/37047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37047/
Attachment #8724493 - Flags: review?(lgreco)

Updated

a year ago
Attachment #8724493 - Flags: review?(lgreco)

Comment 4

a year ago
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.
(Assignee)

Comment 5

a year ago
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)
(Assignee)

Comment 6

a year ago
https://reviewboard.mozilla.org/r/37047/#review33677

Good catch. I should have checked that the tests failed without calling close().

Updated

a year ago
Attachment #8724493 - Flags: review?(lgreco)

Comment 7

a year ago
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.
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Comment 9

a year ago
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 10

a year ago
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+
(Assignee)

Comment 11

a year ago
https://hg.mozilla.org/integration/fx-team/rev/09d8e3caf51daf1554673799609b575de4383a02
Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl
(Assignee)

Comment 12

a year ago
https://hg.mozilla.org/integration/fx-team/rev/38d23f0168980b27bb88bf20a8cba7f907bc495e
Backed out changeset 09d8e3caf51d (bug 1190663) for intermittent mochitest failures on Windows
(Assignee)

Comment 13

a year ago
https://hg.mozilla.org/integration/fx-team/rev/afa990f7034a12b78e1cba02b532a254c2d1990f
Bug 1190663: [webext] Add tests for window.close() in browserAction and pageAction popups. r=rpl

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afa990f7034a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

a year ago
Duplicate of this bug: 1260490

Updated

a year ago
Blocks: 1253374
You need to log in before you can comment on or make changes to this bug.