Rob--W / browser-action-jplib

Jetpack module to add a Browser action badge to the toolbar, using the chrome.browserAction syntax from Chromium
21 stars 9 forks source link

Button removal is not preserved across restarts #10

Closed tmiw closed 10 years ago

tmiw commented 10 years ago

When I remove a button created by this library from the toolbar by dragging it into the Customize dialog box, it removes the button as expected. However, after restarting Firefox, the button gets re-added. This is not ideal as the user will need to continually hide the button.

From my brief look at the issue, it appears that toolbarwidget saves the current state of the toolbar (namely, the list of buttons currently on there) using simple-storage, but this state includes the just-removed button. Even if it didn't, there is code in moveWidgetToToolbar() around lines 116-128 that will ensure that the button gets re-added if the button isn't in the saved state. Mozilla documentation seems to imply that currentset is updated live whenever a toolbar changes, so I'm not sure what's going on there.

This is all with Firefox 27, btw.

Rob--W commented 10 years ago

This bug also exists in the sdk/widget module: https://bugzilla.mozilla.org/show_bug.cgi?id=854226

According to the bug report, it has been fixed in a recent version of the Add-on SDK. I haven't checked though.

tmiw commented 10 years ago

Tried both the latest SDK in their github repository and with 1.15 with the same results. With the latest in their github repository, simple-storage does seem to be storing the correct info, but as stated above, there is code to add the button back on startup.

Is there anything I need to specify to BrowserAction to allow this behavior to function correctly? I'm currently creating the button with the following code:

if (typeof require !== "undefined" /*&& options.loadReason === "install"*/)
{
    // Use BrowserAction package to simulate Chrome behavior on Firefox.
    // Button creation should only take place on install.
    controller._browserInterface._badge = require('browserAction').BrowserAction({
        default_icon: 'icon_16.png',
        default_title: 'newsrdr.us'
    });
}
tmiw commented 10 years ago

Contents of simple-storage after button removal but before restart:

screenshot 2014-01-07 08 49 10

Contents after restart (the button is at the far right):

screenshot 2014-01-07 08 50 56

Rob--W commented 10 years ago

The sdk/widget module will always render the Widget. The toolbarwidget defines the target position at one of the following lines:

The issue you've reported is caused by the third case. toolbarwidget cannot find the position of the widget, so it falls back to default behavior. Unfortunately, it seems that the Add-on SDK's widget module does not respect the user's visibility preference (any more).

A way to fix the problem is to store the visibility of the widget in sstorage during customization: https://github.com/Rob--W/toolbarwidget-jplib/blob/master/lib/toolbarwidget.js#L298-L322
Then, when the ToolbarWidget is initialized, the widget somehow needs to be moved back to the customization pallette. I cannot think of a way of doing this at the moment, but it's probably possible. To develop the module, just look in Firefox's source code (http://dxr.mozilla.org/) and the add-on SDK's source code (/opt/addon-sdk/lib/sdk ?).

I do not have the time to look into it at the moment, so it would be great if you want to look into it and send a Pull request.

tmiw commented 10 years ago

Apparently simple-storage will take up to five minutes by default before data is synched to the hard drive. This is adjustable as a user preference but ideally we would be able to sync manually. As a result, sstorage may be empty if I drag the button off the toolbar and restart the browser too quickly.

I'm looking at options but this may require switching to something like simple-prefs or IndexedDB for reliability.

Rob--W commented 10 years ago

@tmiw Data is synchronized every 5 minutes, and on unload: https://github.com/mozilla/addon-sdk/blob/38851bf46c0950780b3b28065bfccac4451b67c1/lib/sdk/simple-storage.js#L123-L133.

tmiw commented 10 years ago

Yep. I was still using the github version of addon-sdk and not 1.15 :) Anyway, created a pull request for this issue over in toolbarwidget. https://github.com/Rob--W/toolbarwidget-jplib/pull/7

Rob--W commented 10 years ago

Fixed in version 1.3.3 of toolbarwidget, see https://github.com/Rob--W/toolbarwidget-jplib/compare/dee2ba36c4...e5ef114be8.

Please confirm and close the issue :)

tmiw commented 10 years ago

Looks like that did the trick. However, a new issue seems to have appeared. The vertical height of the button seems to shrink after being moved to the customization palette. When moved back to a toolbar and Firefox restarted, the button height returns to normal.

Rob--W commented 10 years ago

Could you attach your test case to a new issue?

tmiw commented 10 years ago

Yep, can do that.

tmiw commented 10 years ago

Looks like everything is good now :) Resubmitted my extension for full approval (which should be granted now, but we'll see). Will let you know if I run into any other issues.