879068 - Panel is stuck open, I cannot hide it

Status: RESOLVED WORKSFORME

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Steps to reproduce:

1. install https://addons.mozilla.org/en-US/firefox/addon/page-portal/
2. Go to github.com and portal to the login/logout page
3. submit the login/logout form
4. try to close the panel

Expected:

Clicking the browser window, a tab, or the 'esc' button would close the open panel

Actual:

The panel never closes, and because of bug 851318 it's on top of every window that I open..

Bug 859012 may resolve the issue, but I suspect that there is a bug somewhere in the SDK or platform caused this.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

We could also add a 'Close Panel' default menuitem in bug 858399

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

I suspect this is due to bug 888688, but we should at least get a test in before closing this bug.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Ideally the esc hotkey listener would not be panel (or loader) dependent, if a `panel` module is loaded, then pressing `ESC`

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> Ideally the esc hotkey listener would not be panel (or loader) dependent, if
> a `panel` module is loaded, then pressing `ESC`

Don't know why that posted..

Ideally the esc hotkey listener would not be panel (or loader) dependent, if a `panel` module is loaded, then pressing `ESC` would close any open sdk panel no matter what loader it came from.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

This should be urgent imo.

Comment 6

[Dave Townsend [:mossop]]

Erik, the best way to get us to re-evaluate a bug's priority is to de-prioritise it

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1075

Pointer to Github pull-request

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 770375 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1075

I've added tests for this issue, one test fails:

error: addon-sdk: fail: the panel is destroyed - undefined == "open"
info: addon-sdk: Traceback (most recent call last):
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/window/helpers.js", line 45, in eventHandler
    deferred.resolve(target);
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/core/promise.js", line 187, in resolve
    result.then(observer.resolve, observer.reject);
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/core/promise.js", line 45, in then
    return { then: function then(fulfill) { fulfill(value); } };
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/core/promise.js", line 120, in resolve
    deferred.resolve(onFulfill ? onFulfill(value) : value);
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path.tests/test-panel.js", line 121, in exports.testPanelHidesOnNewWindowWithDestroyOnHide/last/</</<
    assert.equal(panelEle.state, undefined, 'the panel is destroyed');
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/test/assert.js", line 125, in equal
    operator: "=="
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/test/assert.js", line 89, in fail
    this._log.fail(message);
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/deprecated/unit-test.js", line 88, in fail
    this.console.trace();
  File "resource://extensions.modules.106d3b09-41a3-479a-a298-6d8be8e340f9-at-jetpack.commonjs.path/sdk/lang/functional.js", line 84, in partial/<
    return function() fn.apply(this, args.concat(Array.slice(arguments)));
info: addon-sdk: pass: destroying the panel

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 770375 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1075

Erik, behavior of removed panel node is not a specified anywhere and I'm not sure that assumption that `xulPanel.state === undefined` is always true. I would rather prefer to test weather `!xulPanel.ownerDocument` is truthy since we just remove panel element from document on unload or destroy. It could be that removing panel from Document tree is not enough, but I'd then I'd consider that to be a platform bug.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

So this issue appears to be a regression from 1.13 to 1.14 so it was probably introduced in the detraitification of panel.

The test I wrote fails on 1.13 tho too even tho that appears to be working correctly when I test it visually/manually.

The test does illustrate the issue, I'm just not sure how to write the assertions to reflect that atm..

I'm going to assign this to Irakli.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #10)
> So this issue appears to be a regression from 1.13 to 1.14

* So this issue appears to be a regression from 1.13 to master

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Here is an example snippet:


require("sdk/panel").Panel({
  width: 180,
  height: 180,
  contentURL: "https://en.wikipedia.org/w/index.php?title=Jetpack&useformat=mobile",
  onShow: function() {
  	let destroy = this.destroy.bind(this);
  	require('sdk/timers').setTimeout(function() {
  	  destroy();
  	}, 500)
  }
}).show();

The page-portal add-on example is obviously a more real world use case, but this is the most simple, automated, and reproducible example that I can provide at the moment.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Hmm this example is more interesting and real world (the message could be send at any time and in reaction to a user interaction):


require("sdk/panel").Panel({
  width: 180,
  height: 180,
  contentURL: "https://en.wikipedia.org/w/index.php?title=Jetpack&useformat=mobile",
  contentScriptWhen: 'end',
  contentScript: 'self.postMessage("loaded")',
  onMessage: function(msg) {
  	this.destroy();
  }
}).show();


you can see the content, but cannot interact with it too..

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #13)
> Hmm this example is more interesting and real world (the message could be
> send at any time and in reaction to a user interaction):
> 
> 
> require("sdk/panel").Panel({
>   width: 180,
>   height: 180,
>   contentURL:
> "https://en.wikipedia.org/w/index.php?title=Jetpack&useformat=mobile",
>   contentScriptWhen: 'end',
>   contentScript: 'self.postMessage("loaded")',
>   onMessage: function(msg) {
>   	this.destroy();
>   }
> }).show();
> 
> 
> you can see the content, but cannot interact with it too..

Also, the document is not in memory, so that would explain the latter issue.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

This seems to be working for me now, and I never managed to get a satisfactory test written when it was busted.  Seems like we should wontfix this.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

Actually wfm is better.