Compatibility with Australis (as a CustomizaleUI widget)

[Quicksaver]

Hi, I'm the developer of an add-on called The Puzzle Piece, and a user brought to my attention a problem with NoScript's toolbar button. With the preference "Open permissions menu when mouse hovers icon" enabled, as is by default, and if you place the NoScript button in any toolbar other than the default ones, for example in the toolbar created by TPP, initially the button will have a dropmarker which will disappear once you hover it.

This is because the button's "type" attribute is only set if the button is found during NoScript initialization, which isn't true, since at this time the toolbar created by TPP doesn't exist yet.

I think this is better fixed on NoScript's side, because it will likely happen with every toolbar created using the new CustomizableUI system. Basically, when a toolbar is created dynamically like this, the widgets are moved from the palette to the toolbar automatically without firing the "aftercustomization" event. Australis also introduces other ways to customize the toolbars which don't require entering customize mode and thus don't fire this event either.

The fix is simple, the same way there's a listener placed for this event that initializes the button, there should also be a listener for onWidgetAdded or onWidgetAfterDOMChange (https://developer.mozilla.org/en-US/doc ... #Listeners) that does the same when the button is actually moved somewhere, both during startup or afterwards at any point (for example, a restartless add-on like mine is enabled by the user and NoScript's button is inserted in its toolbar).

If you need any extra info about this just let me know, I'll be happy to help.
Luís Miguel

[therube]

> add-on called The Puzzle Piece

URL?

[Quicksaver]

Sorry, I completely forgot to post a link. https://addons.mozilla.org/en-US/firefo ... zle-piece/

[Quicksaver]

Bump. Is this even in the plans?

[Giorgio Maone]

Sorry, this dropped out of my radar because of higher priorities. I'll try to fix it in next release, thanks.

[Quicksaver]

I'm glad to hear it. Let me know if you need any help or any extra info.

[Giorgio Maone]

Please check latest development build 2.6.8.37rc3, thanks.

[Quicksaver]

That seems to be working great, the button is initialized immediately as it should be as soon as it's added to the toolbar. Thanks for your work on this!

Just a small note about the code. Right now it's running noscriptOverlay.initPopups() on every widget that's added. The recursion protection you've added is good at keeping the method in check to not run a bunch at a time unnecessarily. But I think you could restrict it further still, by adding a check in the onWidgetAdded handler to make it only act when the NoScript's buttons are involved. Something like the following:

Code: Select all

onWidgetAdded: function(aWidgetId) {
  for(var i=0; i<buttons.length; i++) {
    if(buttons[i] && buttons[i].id == aWidgetId) {            
      noscriptOverlay.initPopups();
      return;
    }
  }
}

[trowe]

This was fixed, but 2.6.8.40 has broken it

[Quicksaver]

It seems it was a coincidence only that it was working before, because it was firing for every onWidgetAdded, it wouldn't initialize when noScript's button itself was added but only afterwards when something else was added.

There are three reasons why it's not working now. First, onWidgetAdded is fired and your loop runs just fine, but if the button is in a custom toolbar that is dynamically created only afterwards, the loop won't find the node because it hasn't been appended yet (no toolbar node to append to yet, even though the area is already registered with CustomizableUI). And when the actual toolbar node is created, CustomizableUI doesn't fire onWidgetAdded, in that case it only fires onWidgetAfterDOMChange. I'm sorry, I didn't realize this when I suggested using onWidgetAdded.

Second, that handler will always use the buttons array from the initPopups() call where it was created. So, if you call it once during startup, tbb will be null because the button won't exist yet. And when the onWidgetAdded/onWidgetAfterDOMChange fires, it still uses that same reference instead of checking again, because it's not the whole initPopups() that's being called, but only the listener itself that was created within that scope.

Third, when onWidgetAfterDOMChange fires, neither the widget node or the toolbar node are in the DOM tree yet. This is because the toolbar is first created/imported into the document, then CustomizableUI restores its widgets if it can which will fire the corresponding onWidgetAfterDOMChange, and only after that it is appended to the DOM tree. I also didn't know this happened, took me a while to figure it out to be honest. So, by running getElementById() it won't find the button yet, because it's not in the DOM tree yet.

So, you have two solutions. Either revert back to not checking for the id's of added widgets (which I still think can be a bit overkill to call initPopup() on every widget change like that), or change that piece of code with the following which I've tested and seems to work correctly for me:

Code: Select all

      const buttonIds = ['noscript-tbb', 'noscript-statusLabel'];
      if (install && "CustomizableUI" in window) {
        CustomizableUI.addListener({
          onWidgetAfterDOMChange: function(aWidget) {
            for each(let b in buttonIds) {
              if(b == aWidget.id) {
                window.setTimeout(function() { noscriptOverlay.initPopups(); }, 0);
                return;
              }
            }
          }
        });
      }

[Giorgio Maone]

Please check latest development build 2.6.8.41rc1, thank you.

[Quicksaver]

Please check latest development build 2.6.8.41rc1, thank you.

Sorry for the delay on this. It's still not working, you forgot to define the buttonIds array:

Code: Select all

      const buttonIds = ['noscript-tbb', 'noscript-statusLabel'];

[Giorgio Maone]

Please check latest development build 2.6.8.41rc3, thanks.

[Quicksaver]

Seems to be working great for me. Thanks for the quick work!