Closed Bug 854226 Opened 7 years ago Closed 6 years ago

Widgets removed from toolbar by user, return after add-on disabled/enabled

Categories

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

x86
Windows XP
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: morac, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:P1])

Attachments

(1 file, 1 obsolete file)

I have CacheViewer Continued installed and it adds an icon to the Add-on bar in the method described at https://addons.mozilla.org/en-US/developers/docs/sdk/1.13/dev-guide/tutorials/adding-toolbar-button.html

When I removed the icon from the toolbar, by customizing the toolbar, the icon is "restored" to it's default location after the add-on is disabled and then enabled. This includes on upgrades, browser restarts, etc.  The widget can be moved to another toolbar and it will persist, but if the widget is removed, it simply comes back.

If the user removes a widget, it should not be restored to the default location.
Jordan, can you try to reproduce this?
Assignee: nobody → jsantell
Priority: -- → P2
The problem is the Widget code always inserts a Widget into the Add-on bar, if it can't be found in another toolbar.

http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/widget.js#598


That really should only be done when an add-on is first installed.  I'm not familiar with using the Add-on SDK so I don't know if there's a way for developers to do this or not, but looking at the options that can be passed to Widget, it doesn't seem possible.

My own add-on uses a custom library that lets the developer specify a default toolbar where, if no toolbar is specified the default location is to not insert anywhere.  Then on an add-on install, I set the default toolbar.  For normal startup I do not.
Confirmed; on disable/enable, the widget appears back in the default location after being removed, checking it out
Assignee: jsantell → nobody
Blocks: 940099
Duplicate of this bug: 940083
Duplicate of this bug: 940099
This is something that we really need addressed before Australis moves to Aurora. It was a relatively unobtrusive problem when it only affected the add-on bar, given the amount of real estate that was devoted entirely to add-ons there. Now that the icons are in the navbar, it's a much more serious issue.

It's also explicitly in violation of AMO policies, which means that affected add-ons will technically need to be downgraded to preliminary review status if they're not fixed.
Priority: P2 → --
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P2
Bugzilla, DIAF.
Priority: P2 → --
Attached patch 854226-widget-restart-adding.txt (obsolete) — Splinter Review
This works, on both Australis and non-Australis (which we need to deal with because of the possibility that we might not ship Australis on 28).

The only thing is that this breaks the addon-bar hide test. I don't understand how that test ever worked. It's now 3am and this patch is approximately 20 times as big as I thought it was going to be because the test and module required a lot of extra fixes (like disambiguating all the test IDs away from "foo", and making it actually deal correctly with inserting something in multiple windows on non-Australis, and blah). So I'll just be leaving this here.
Attachment #8335011 - Flags: review?(zer0)
(In reply to :Gijs Kruitbosch from comment #9)
> Created attachment 8335011 [details] [diff] [review]
> 854226-widget-restart-adding.txt
> 
> This works, on both Australis and non-Australis (which we need to deal with
> because of the possibility that we might not ship Australis on 28).
> 
> The only thing is that this breaks the addon-bar hide test. I don't
> understand how that test ever worked. It's now 3am and this patch is
> approximately 20 times as big as I thought it was going to be because the
> test and module required a lot of extra fixes (like disambiguating all the
> test IDs away from "foo", and making it actually deal correctly with
> inserting something in multiple windows on non-Australis, and blah). So I'll
> just be leaving this here.

So the reason this test worked was that it never uncollapsed the addon toolbar on the other window, so it didn't need to uncollapse it. My changes made sure it *did* uncollapse the toolbar on the other window, and because the code to uncollapse it is broken for whatever reason, it doesn't do that and the test fails.
(In reply to :Gijs Kruitbosch from comment #9)
> Created attachment 8335011 [details] [diff] [review]
> 854226-widget-restart-adding.txt
> 
> This works, on both Australis and non-Australis (which we need to deal with
> because of the possibility that we might not ship Australis on 28).
> 
> The only thing is that this breaks the addon-bar hide test. I don't
> understand how that test ever worked. It's now 3am and this patch is
> approximately 20 times as big as I thought it was going to be because the
> test and module required a lot of extra fixes (like disambiguating all the
> test IDs away from "foo", and making it actually deal correctly with
> inserting something in multiple windows on non-Australis, and blah). So I'll
> just be leaving this here.

Now with less confusion:

(In reply to :Gijs Kruitbosch from comment #10)
> So the reason this test worked was that it never uncollapsed the addon
> toolbar on the other window, so it didn't need to collapse it. My changes
> made sure it *did* uncollapse the toolbar on the other window, and because
> the code to collapse it is broken for whatever reason, it doesn't do that
> and the test fails.
Attached file Github PR
So the test in question about the add-on bar is thoroughly broken. I've added assertions that prove this and disabled the test completely because I don't think it's worth fixing as we'll be getting rid of this code in 0-2 cycles anyway. Let's just focus on the future instead of the past.

Dave and Matteo, this bug is getting more dupes than I can count (not all of them were duped here, see also bug 940099) since Australis landed. It'd be really great if we could get this fixed and on the trunk ASAP.
Attachment #8335011 - Attachment is obsolete: true
Attachment #8335011 - Flags: review?(zer0)
Attachment #8335713 - Flags: review?(zer0)
Attachment #8335713 - Flags: review?(dtownsend+bugmail)
it is Not just the add-on bar. I can Not get Firefox items added to this 'new menu' launcher thing on the right edge of Navbar. I submitted a bugreport (https://bugzilla.mozilla.org/show_bug.cgi?id=940099), but it was marked as dup of https://bugzilla.mozilla.org/show_bug.cgi?id=940099 and it is marked a dup of this one. 
Mine does Not seem to be a dup as it primarily deals with, again this 'menu button' on right edge of navbar.

Mine does Not seem to be a small issue that can be put off to Aurora.

Landis.
my bugreport, again: https://bugzilla.mozilla.org/show_bug.cgi?id=940099
The add-on bar was removed with the introduction of Australis. Widgets which were previously added to it now wind up in the navbar instead. This is the same issue.
Duplicate of this bug: 941594
Priority: -- → P1
Attachment #8335713 - Flags: review?(zer0) → review+
Duplicate of this bug: 941880
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0387cbd6f5a1a158d663efa2ed55841ecfbac450
Bug 854226 - keep check of widgets having been inserted by id, r?zer0

https://github.com/mozilla/addon-sdk/commit/e65dadeb45dd7cdf0051cd8eb92df67a4dfc7efb
Merge pull request #1300 from gijsk/854226-widget-restart-adding

Bug 854226: keep check of widgets having been inserted by id. r=zer0
Attachment #8335713 - Flags: review?(dtownsend+bugmail)
loading and scrolling this page has taken Several minutes.
Other open windows scroll normally.
clicked on 'funky menu' button (right side of nav-bar).
Added, by dragging 'Miscellaneous' from left to empty box on right.
Close 'Customize' by clicking 'customize' again.
Restart FF
The window (1 of 3) that the 'customize' action was done, is Very un-responsive (after several minutes, this page is Still flashing as if not loaded and scrolling is impossible.) Slow and the 'funky menu' button does Nothing (will Not activate, no drop-down).
Other 2 windows scroll and the 'funky menu' button activate, but the addition of 'Miscellaneous' is Missing.
Right-Click context menu does not work.
Again, page above this 'Additional Comments' form is still flashing ever 10-20 seconds as if still loading or script is in loop. But, spellcheck as i type seems to be working well, it will under line misspelled words immediately, but again, right-click context menu to find 'recomended' corrections will Not activate, hence the misspelled 'recomended'.

Landis.
Quite FF, Start FF
this page scrolls normal, IF 'funky menu' on right edge of nav-bar is Not clicked.
Landis.
I'm seeing various similar issues with addon buttons returning to the same position after browser restart no matter where they were moved (such an example addon is Wappalyzer) or even completely vanishing from both the toolbar as well as from the customize tab. Is this the same bug? Is there a change that must be communicated to addon developers so that they know what needs to be done on their part?
(In reply to klonos from comment #21)
> I'm seeing various similar issues with addon buttons returning to the same
> position after browser restart no matter where they were moved (such an
> example addon is Wappalyzer)

I had a look, and Wappalyzer directly overlays the add-on bar. I didn't think of this case when I wrote the shim that manages stuff that gets added to the add-on bar, so it's probably not handled in the best way. I'll investigate this further in a separate bug.

> or even completely vanishing from both the
> toolbar as well as from the customize tab.

Was this adblockplus? If so, that's a known issue. I suspect other add-ons may have copied its code, too, but it's impossible to say without knowing which add-on is concerned.

> Is this the same bug?

This specific bug is about widgets added using the Add-on SDK (aka jetpack).

It's impossible to say whether that explains all your other add-ons' behaviour without knowing which add-ons are concerned. Every other add-on has their own fancy way of adding toolbar buttons (one of the many reasons we're trying to standardize on something by adding specific APIs in Australis). If they don't do anything too stupid, they should continue to work, but equally they might not. We're working on add-on developer documentation (some of which is already at https://developer.mozilla.org/en-US/Firefox/australis-add-on-compat-draft ).

The best way to be sure is to file a new bug with a complete list detailing for which add-on buttons you're seeing what kind of behaviour. CC me and I'll have a look.
(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to klonos from comment #21)
> > I'm seeing various similar issues with addon buttons returning to the same
> > position after browser restart no matter where they were moved (such an
> > example addon is Wappalyzer)
> 
> I had a look, and Wappalyzer directly overlays the add-on bar. I didn't
> think of this case when I wrote the shim that manages stuff that gets added
> to the add-on bar, so it's probably not handled in the best way. I'll
> investigate this further in a separate bug.

Filed bug 943285 and put up a patch there. Thanks for reporting this!
Duplicate of this bug: 912656
SDK commits:

(In reply to [github robot] from comment #17)
> Commits pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> 0387cbd6f5a1a158d663efa2ed55841ecfbac450
> Bug 854226 - keep check of widgets having been inserted by id, r?zer0
> 
> https://github.com/mozilla/addon-sdk/commit/
> e65dadeb45dd7cdf0051cd8eb92df67a4dfc7efb
> Merge pull request #1300 from gijsk/854226-widget-restart-adding
> 
> Bug 854226: keep check of widgets having been inserted by id. r=zer0

m-c commits with fx-team pushlog times (see also bug 941933):

https://hg.mozilla.org/mozilla-central/rev/116c5049e8f4 (initial uplift, Thu Nov 21 20:42:22 2013 -0800)
https://hg.mozilla.org/mozilla-central/rev/de8163b494e9 (backout for orange, Thu Nov 21 23:20:48 2013 -0800)
https://hg.mozilla.org/mozilla-central/rev/e90c7b40f24b (second uplift, Wed Nov 27 16:43:27 2013 -0800)

which was merged to m-c today, and should be in tomorrow's Nightly -> FIXED!

(Thanks Dave, and happy Thanksgiving everyone :-) )
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Sorry for the late reply.

Thanx for working on this bug. but it seems that neither fixing this one nor bug 943285 actually resolved the issue. The specific addon I'm seeing this behavior with (buttons removed after restart and not showing up in the customize tab either) is Session Manager in combination with Classic Theme Restorer. Please see the related bug report:

https://www.mozdev.org/bugs/show_bug.cgi?id=25543
...I also mention this in the bug report linked to from my previous comment, but just to clarify:

After troubleshooting this I managed to narrow it down: it seems that the buttons are having this behavior only when moved to a toolbar other than the navigation bar.
(In reply to klonos from comment #27)
> ...I also mention this in the bug report linked to from my previous comment,
> but just to clarify:
> 
> After troubleshooting this I managed to narrow it down: it seems that the
> buttons are having this behavior only when moved to a toolbar other than the
> navigation bar.

OK. Session Manager isn't using the widget library for these buttons, so this bug isn't actually relevant to that add-on. I'll see if I can dig up my mozdev account and will comment on the add-on's bugreport. Let's keep the further diagnostics there for now.
Duplicate of this bug: 950403
You need to log in before you can comment on or make changes to this bug.