940227 - Australis: Lastpass re-inserts itself when removed

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[derekchiang93]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20131118094134

Steps to reproduce:

1. Install LastPass add-on
2. Click the menu button
3. Select "Customize"
4. Drag LastPass into the main screen (a.k.a. remove it from the toolbar)
5. Close the customize tab (at this point, LastPass does not show in the toolbar)
6. Open a new window


Actual results:

LastPass is now back on the toolbar, not only in the new window, but also in the old window.


Expected results:

LastPass shouldn't appear on the toolbar.

Comment 1

[:Gijs (he/him)]

I strongly suspect this is a lastpass problem, but we can track it here until that's been investigated.

Comment 2

[Sören Hentzschel]

It affects the same add-ons as is bug 940099.

Comment 3

[:Gijs (he/him)]

(In reply to Sören Hentzschel from comment #2)
> It affects the same add-ons as is bug 940099.

Good point.

Comment 4

[:Gijs (he/him)]

So this isn't actually an SDK add-on, but it *is* a lastpass problem.

A relevant bit of pretty-printed code (thanks, browser toolbox!) with some slight modifications by yours truly to make it slightly more readable (single-line ternary statements to execute code (in lieu of an if/else statement) are rather tedious, and it was rather obvious that 'c' was a document...):

var d = document.getElementById('nav-bar');
if (d) {
    var g = document.getElementById('lpt_lastpass-compact-btn');
    if (g && g.parentNode == d && !e) {
      g.style.display = 'none';
    } else if (g && g.parentNode == d && e) {
      g.style.display = '';
    } else if ((!g || 'BrowserToolbarPalette' == g.parentNode.id) && e && 'undefined' != typeof d.insertItem) {
      d.insertItem('lpt_lastpass-compact-btn', null, null, !1);
      var h = d.currentSet;
      if ('string' == typeof h && '' != h && 'lpt_lastpass-compact-btn' != h && - 1 != h.indexOf('lpt_lastpass-compact-btn')) {
        d.setAttribute('currentset', h);
        document.persist('nav-bar', 'currentset');
      } else {
        f = !1;
      }
    }
}


'e' here is the value of the lastpass user pref 'useCompactBtn' (which you can presumably toggle to make this stop). Either way, this code is still wrong, because now it also inserts itself when the button is added to the panel and the panel hasn't yet been opened (as will normally be the case when the browser has just been opened).

Additionally, hiding (rather than removing) your button when toggling a pref just makes things worse, because now it'll just mess up algorithms trying to figure out where stuff gets dragged (e.g. bug 968565, which yes, we can fix, but ideally add-ons wouldn't be doing this in the first place).

Comment 5

[Andrew Zitnay]

IIRC (and it's been many years, so I could be a little off), we attempted to not force the button onto the toolbar every time, but we ended up with thousands of users claiming the button disappeared on them for no reason (which we couldn't reproduce).  So, we came up with this method of ensuring it's on the toolbar as long as the preference is set.

I'm open to suggestions on how we can improve this, but I can't simply revert to our old method, since thousands of users could be impacted by such a change.

Comment 6

[:Gijs (he/him)]

(In reply to Andrew Zitnay from comment #5)
> IIRC (and it's been many years, so I could be a little off), we attempted to
> not force the button onto the toolbar every time, but we ended up with
> thousands of users claiming the button disappeared on them for no reason
> (which we couldn't reproduce).  So, we came up with this method of ensuring
> it's on the toolbar as long as the preference is set.
> 
> I'm open to suggestions on how we can improve this, but I can't simply
> revert to our old method, since thousands of users could be impacted by such
> a change.

Hi Andrew, thanks for responding. A good first step would be to add code that works with the panel we added in Australis (currently on Aurora, for version 29). You'd want something like:


let isPresent = !!document.getElementById("lpt_lastpass-compact-btn");
if (!isPresent && ('CustomizableUI' in window)) {
  isPresent = !!CustomizableUI.getPlacementOfWidget("lpt_lastpass-compact-btn");
}

to ensure that the button hasn't been moved to the panel by the user (I've added a check for CustomizableUI to ensure the add-on would also work on non-Australis, ie current Firefox releases).


As for ensuring that people can remove the button to the palette... a pref is the canonical solution to this - you insert the button on firstrun, and set a pref to never automatically reinsert the button after that. I'm not sure if that's what you're referring to when talking about the 'old method'. From version 29 onwards, we store the location of items in a preference ourselves (they're currently in localstore.rdf), so at least when CustomizableUI is present, I think that you should be able to rely on this method.

Let me know if you have any questions.

Comment 7

[Andrew Zitnay]

Thanks, I'm looking into this now.

One thing I'm not clear on...  Once I drag the button elsewhere (for example, the menu panel), how can I get the XUL element of lpt_lastpass-compact-btn?  document.getElementById("lpt_lastpass-compact-btn") returns null at that point.

Comment 8

[:Gijs (he/him)]

(In reply to Andrew Zitnay from comment #7)
> Thanks, I'm looking into this now.
> 
> One thing I'm not clear on...  Once I drag the button elsewhere (for
> example, the menu panel), how can I get the XUL element of
> lpt_lastpass-compact-btn? 
> document.getElementById("lpt_lastpass-compact-btn") returns null at that
> point.

The button will stay in the palette until the user first opens the panel. This is done for performance reasons. You can either get it from the palette (e.g. gNavToolbox.palette.querySelector("#lpt_lastpass-compact-btn") ) or you can use a CustomizableUI method, e.g.:

CustomizableUI.getWidget("lpt_lastpass-compact-btn").forWindow(window).node;

Comment 9

[Andrew Zitnay]

Alright, it was a ton of work, but I think I have moving LastPass to the menu panel working pretty well...  If anyone wants to try it, I've updated our latest pre-build:

https://lastpass.com/dlpre/

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

The latest update was approved on AMO, fixing this issue.