Closed Bug 685929 Opened 13 years ago Closed 12 years ago

Multiple widgets position in same toolbar not persistent after restart

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: r3gis, Assigned: ochameau)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110909 Firefox/9.0a1
Build ID: 20110909032331

Steps to reproduce:

* Create a widget that add more than one widget
* Customize widget position and put all in the navigation bar for example (important point is that it must be the same toolbar)
* Restart firefox


Actual results:

Only the first widget is now located on the navigation bar. All other widget are in addon bar.


Expected results:

All widgets should be in the navigation bar as before restart.
Possible addons to reproduce issue :
https://addons.mozilla.org/fr/firefox/addon/googlemusic-remote-control/
OR
https://addons.mozilla.org/fr/firefox/addon/grooveshark-remote-control/

But can be reproduced with a simple basic addon that create two widgets.

My first analysis is that problem is in widget.js in function _insertNodeInToolbar and introduced by the two last lines :
-------
    // Update DOM in order to save position if we remove/readd the widget
    container.setAttribute("currentset", container.currentSet);
    // Save DOM attribute in order to save position on new window opened
    this.window.document.persist(container.id, "currentset");
-------

In fact what happen is that when there is more than one widget saved in currentSet for an addon, this methods will at the first run store container.currentSet.

Problem is that at this point, container.currentSet does not contains yet the other widgets of the addon. So it store the list of widgets *without* other widgets of the addon (not yet added).
As consequence next runs of the method will not detect that the widget should go in the customized toolbar.

Commenting these two lines seems to solve the problem and to not introduce regression for use case defined as per comment and commit message that introduce these two lines. 
But I'm not sure of this last point, so should be analyzed more deeply :).


Besides, regarding my analysis, I think that it could also apply to widgets of all addons not yet added to the bar. And that one is able to customize position only of one jetpack widget in a toolbar.
Hmm, this definitely sounds like undesired behavior! :-)

Alex, Hernán: can y'all confirm this problem?
Whiteboard: [triage:followup]
(In reply to Myk Melez [:myk] from comment #2)
> Hmm, this definitely sounds like undesired behavior! :-)
> 
> Alex, Hernán: can y'all confirm this problem?

Uhm, I've seen this come up in the grooveshark addon issue tracker, but been too busy to have a look at it. I would vote for confirmed though.
Confirmed! (Feel free to do this yourself for bugs you think warrant it!)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Myk Melez [:myk] from comment #4)
> Confirmed! (Feel free to do this yourself for bugs you think warrant it!)

Yup, I warrant it and confirm it now :) I tested with latest source and GRC 0.3.2 and can reproduce on Firefox 6.0.2/Ubuntu 11.04
Priority: -- → P2
Whiteboard: [triage:followup]
We got one report from our MemChaser users who also can see this bug regularly, because he moved the widget into the navigation bar.
Any updates on this?
Attached file Pull request 388
Sorry guys for the delay, I should have looked at this way before!

So the patch from bug 656298 allowed to ensure that widget are displayed in the expected toolbar/offset when we open a new window. That's because Firefox-and-Widget.js codes are using this `currentset` DOM attribute in order to restore toolbaritems positions.
But this code ended up conflicting with itself, when multiple addons were running this code on Firefox startup, or when one addon used multiple widgets in the same toolbar.
The code is now even more complex and not 100% satisfying, but I think that the only way to implement the whole thing sanely would be to handle widgets in Firefox, or in a unique instance of a Widget module.

In the meantime, this bug is being reported in almost every major Jetpack addons, like Collusion, MemChaser, ...
We have read many angry users comments about "Widget disappearing on each startup". I think and hope it was solely because of this bug.
It sounds really important to get it fixed ASAP.


dietrich, feel free to forward this review to irakli if you do not have time to look at it!
Assignee: nobody → poirot.alex
Attachment #609719 - Flags: review?(dietrich)
Hernán, Henrik, would you have some time to give a try to this patch?
Comment on attachment 609719 [details]
Pull request 388

Trying to get this in 1.6, so we will try to get it landed quickly.
Attachment #609719 - Flags: review?(dietrich) → review?(rFobic)
Attachment #609719 - Flags: review?(rFobic) → review+
This is working!
I tested it in Firefox 11 and Aurora on OSX 10.6.
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/1430b712bc2c7f11309eb87788fefd154e0c4d46
Bug 685929 - Multiple widgets position in same toolbar not persistent after restart

https://github.com/mozilla/addon-sdk/commit/778701a9400151df2115238d8c2d64500e5467be
Merge pull request #388 from ochameau/bug/685929-fix-widget-positions

fix Bug 685929 - Multiple widgets position in same toolbar not persistent after restart r=@gozala
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: --- → 1.6
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/de78ccb1c23ea6a78675fc572151ef27d9eca58e
Bug 685929 - Multiple widgets position in same toolbar not persistent after restart

(cherry-picked from commit 1430b712bc2c7f11309eb87788fefd154e0c4d46)
You need to log in before you can comment on or make changes to this bug.