696552 - I can't change a panel's contentURL property from a widget's onClick handler.

Status: RESOLVED FIXED

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

Description

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

In the linked addon on Builder, I set up a panel that shows the Yahoo homepage. Right after that is defined, I change the panel's contentURL property to show the Bing homepage.

I then create a widget that tries to change the panel's contentURL property to Google's homepage.

Line 12 shows the panel. (You never really see Yahoo in the panel because it's changed right after the show() call.)

Line 14 logs "http://www.yahoo.com" to the console.
Line 15 changes the contentURL value to Bing, and you see Bing in the panel.
Line 16 logs "http://www.bing.com" to the console.

If you uncomment line 17, you see Amazon in the panel (and you don't see Bing, because the change is too fast for it to load in time). I did this to see if it was just that the contentURL property could only be changed once, but it works for a second time here with Amazon.

Next, click outside of the panel to close it.

Now click the "Click me" widget in the addon bar. Two messages should have been logged: "Panel is showing: http://www.bing.com" and "Panel now shows: http://google.com".

But the panel's content is still Bing...

Close the panel again, and click the widget again. The panel still shows Bing, but both of the logged messages list "http://google.com".

Comment 1

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

Also interesting is https://builder.addons.mozilla.org/addon/1021933/latest/ where both the change from Yahoo to Bing and the change from Bing to Google happen in the same function.

Comment 2

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

P1 to at least figure out what's going on.

Alex, can you take a look at this?

Comment 3

[Alexandre Poirot [:ochameau]]

Actually, we can't change panel document, you luckily are able to modify the URL for something else than contentURL given in constructor because of asynchronous internal implementation.
But as soon as the document is loaded, there is no way to change it.
Panel doesn't behave like page-worker.

By reading the documentation, this seems more like a missing feature than a bug?
https://addons.mozilla.org/en-US/developers/docs/sdk/1.2/packages/addon-kit/docs/panel.html

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Implement contentURL change

If we want this feature/fix, here is a patch that do it.

FYI: we support changing panel document from the document itself, with:
  document.location = "..."

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 574868 [details] [diff] [review]
Implement contentURL change

It doesn't really matter if this is a bug or a feature because this change is usefull! (One guy on irc was trying to work around this)
Let's try to land this.
Comment for the review: this patch mimics what is done in page-worker.
(page-worker already support contentURL modification)

Comment 6

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

Comment on attachment 574868 [details] [diff] [review]
Implement contentURL change

Review of attachment 574868 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but with the mentioned note being addressed. If you'll end up doing more than moving this line or adding a comment, lets make another round.

::: packages/addon-kit/lib/panel.js
@@ +115,5 @@
>        this.contentURL = options.contentURL;
>  
>      this._init(options);
> +
> +    this.on('propertyChange', this._onChange.bind(this));

It's not clear why this listener is set after `_init`, is being called. If it needs to run after init, then there is `_onInit` method for that, otherwise I think it should be before `_init`. If there is a reason why it should run after init, please specify via comment, otherwise please move it before call to `_init`.

Comment 7

[Alexandre Poirot [:ochameau]]

Oh I forgot about this patch.
I revived it through a pull request:
https://github.com/mozilla/addon-sdk/pull/361

In order to merge it (instead of just adding a simple revision to master)

Comment 8

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/8309f59c5a659c8047edcfd354961c012364a43c
Merge pull request #361 from ochameau/bug-696552

Bug 696552: Implements panel.contentURL modification support. r=@gozala

Comment 9

[Alexandre Poirot [:ochameau]]

Documentation bug opened, bug 733763.

Comment 10

[Alexandre Poirot [:ochameau]]

Will, I gave a try to your addon and yes, it looks like it is broken again.

Comment 11

[Alexandre Poirot [:ochameau]]

Oh nevermind, I just faced the same issue than you.
This patch is only going to be in 1.7.