1092958 - CustomizableUI.jsm Changing toolbarbutton type in a custom widget, after moving it to the palette area, doesn't re-render the button, when 2 or more windows are open.

Status: RESOLVED WORKSFORME

(Firefox :: Untriaged, defect)

Description

[Maciej Ćwiek]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36

Steps to reproduce:

1. Create a custom widget with a toolbarbutton with type of "panel".
2. Set the defaultArea to AREA_NAVBAR.
2. Create an onWidgetRemoved handler and change the button's type to "button" within the handler.
3. Install the extension and open up two FF windows.
4. Go to customization panel in one of the open windows and move the widget to the palette area.


Actual results:

In the window that's been used to move the widget, the button type has changed and its overall look & feel has been updated, to make it look like a button (no arrow next to the icon, which we get for panels). Now if we switch over to the other window, we'll notice that the widget has also been moved, what is a desired behavior, but the way the button looks like, is still wrong, as it has the arrow and still looks like a panel widget, even though the type of the button changes successfully, what's been confirmed by debugging the onWidgetRemoved handler.


Expected results:

When we programmatically change the type of a toolbarbutton, its look and feel should update across all open windows.

Comment 1

[:Gijs (he/him)]

Just to be clear, do you mean the type attribute on a <toolbarbutton> or the type property of an API-provided widget specification ( https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/CustomizableUI.jsm#Widgets ) ? I'm assuming you mean the former...


How are you updating the type attribute on the toolbarbutton?

The onWidgetRemoved handler only gives you the widget ID, not the button, so you have to get the button somehow. It sounds like the problem is with your code - you probably do something like: document.getElementById("widget") or whatever, and then you only update the button in that one window. 

If you want a handler that gets called for each instance, try onWidgetAfterDOMChange (although you'll need to do some checks to make sure the widget isn't just being moved from one area to another). Alternatively, you can use the CustomizableUI.windows iterator to go through all the windows and do your updating.

Comment 2

[Maciej Ćwiek]

(In reply to :Gijs Kruitbosch from comment #1)
> Just to be clear, do you mean the type attribute on a <toolbarbutton> or the
> type property of an API-provided widget specification (
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> CustomizableUI.jsm#Widgets ) ? I'm assuming you mean the former...
> 
> 
> How are you updating the type attribute on the toolbarbutton?
> 
> The onWidgetRemoved handler only gives you the widget ID, not the button, so
> you have to get the button somehow. It sounds like the problem is with your
> code - you probably do something like: document.getElementById("widget") or
> whatever, and then you only update the button in that one window. 
> 
> If you want a handler that gets called for each instance, try
> onWidgetAfterDOMChange (although you'll need to do some checks to make sure
> the widget isn't just being moved from one area to another). Alternatively,
> you can use the CustomizableUI.windows iterator to go through all the
> windows and do your updating.

Hello,

Yeah, sorry for the confusion and not being clear on this. It's about changing the type attribute of the toolbarbutton, not the widget. The widget type is set to "custom" and remains untouched.

Basically I have a global array, which stores references to toolbarbutton instances created in onBuild handler. Whenever I want to change the type, I trigger a method, which iterates through that array and updates the type attribute of each instance of the button. Since I've got that array, I don't need any additional information in onWidgetRemoved handler, apart from the ID, to check whether the handler has been triggered with relation to my widget.

Moreover, what I didn't mention in the original description, this solutions works like a charm, regardless to how many windows I have open at a time, when I move the widget to the panel area, for which I also change the type attribute using exactly the same approach. The only difference is for the panel area I'm using onWidgetAdded handler instead, but to me, it should make no difference in terms of the final result, which is supposed to be identical for both areas.

I hope this is more clear now, but if not, please let me know.

Comment 3

[:Gijs (he/him)]

(In reply to Maciej Ćwiek from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Just to be clear, do you mean the type attribute on a <toolbarbutton> or the
> > type property of an API-provided widget specification (
> > https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> > CustomizableUI.jsm#Widgets ) ? I'm assuming you mean the former...
> > 
> > 
> > How are you updating the type attribute on the toolbarbutton?
> > 
> > The onWidgetRemoved handler only gives you the widget ID, not the button, so
> > you have to get the button somehow. It sounds like the problem is with your
> > code - you probably do something like: document.getElementById("widget") or
> > whatever, and then you only update the button in that one window. 
> > 
> > If you want a handler that gets called for each instance, try
> > onWidgetAfterDOMChange (although you'll need to do some checks to make sure
> > the widget isn't just being moved from one area to another). Alternatively,
> > you can use the CustomizableUI.windows iterator to go through all the
> > windows and do your updating.
> 
> Hello,
> 
> Yeah, sorry for the confusion and not being clear on this. It's about
> changing the type attribute of the toolbarbutton, not the widget. The widget
> type is set to "custom" and remains untouched.
> 
> Basically I have a global array, which stores references to toolbarbutton
> instances created in onBuild handler. Whenever I want to change the type, I
> trigger a method, which iterates through that array and updates the type
> attribute of each instance of the button. Since I've got that array, I don't
> need any additional information in onWidgetRemoved handler, apart from the
> ID, to check whether the handler has been triggered with relation to my
> widget.
> 
> Moreover, what I didn't mention in the original description, this solutions
> works like a charm, regardless to how many windows I have open at a time,
> when I move the widget to the panel area, for which I also change the type
> attribute using exactly the same approach. The only difference is for the
> panel area I'm using onWidgetAdded handler instead, but to me, it should
> make no difference in terms of the final result, which is supposed to be
> identical for both areas.
> 
> I hope this is more clear now, but if not, please let me know.

Not really, no... so you're changing the attribute, and you can verify that the attribute is changed on the DOM node in both windows, but the appearance is correct in one and wrong in the other? (NB: XUL attribute, not JS property!)

Code or a working (well, broken) example would help here.

Comment 4

[Maciej Ćwiek]

(In reply to :Gijs Kruitbosch from comment #3)
> Not really, no... so you're changing the attribute, and you can verify that
> the attribute is changed on the DOM node in both windows, but the appearance
> is correct in one and wrong in the other? (NB: XUL attribute, not JS
> property!)
> 
> Code or a working (well, broken) example would help here.

Yes, that's exactly what's happening. Have a look at the example below. It's based on actual product source shortened for the purpose of tackling the problem.

============ CoffeeScript source - easier to read than what's it compiled to (see below) ===================

define [], () ->
  CU = CustomizableUI

  class MyExtensionButton
    _id: 'my-extension'
    _buttons: []

    @_area_ids: [CU.AREA_NAVBAR, CU.AREA_TABSTRIP, CU.AREA_MENUBAR, CU.AREA_BOOKMARKS, CU.AREA_PANEL, CU.TYPE_TOOLBAR, CU.TYPE_MENU_PANEL, 'default']

    # A cross-reference object mapping desired button types by area IDs.
    _btn_type_by_area: _.object @_area_ids, ['panel', 'panel', 'panel', 'panel', 'button', 'panel', 'button', 'button']

    _getButtonTypeByArea: (area) ->
      type = @_btn_type_by_area[area]
      type ?= @_btn_type_by_area['default']
      type

    _setButtonTypeByArea: (area) ->
      for btn in @_buttons
        btn.type = @_getButtonTypeByArea area

    constructor: ->

    add: ->
      CU.createWidget
        id: @_id
        defaultArea: CU.AREA_NAVBAR
        type: 'custom'
        onBuild: (aDocument) =>
          btn = aDocument.createElement 'toolbarbutton'
          props =
            id: @_id
            label: 'My Extension'
            title: 'My Extension'
            tooltiptext: 'My Extension'
            # aId is undefined is thrown on this, but _getButtonTypeByArea provides fallback, so not an issue in this case
            type: @_getButtonTypeByArea CU.getWidget(@_id).areaType
            align: 'center'
            class: 'toolbarbutton-1 chromeclass-toolbar-additional'
            image: 'path/to/icon.png'

          btn.setAttribute k, v for k, v of props

          @_buttons.push btn

          btn

      CU.addListener
        onWidgetAdded: (id, area) =>
          return if id isnt @_id
          @_setButtonTypeByArea area

        onWidgetRemoved: (id) =>
          return if id isnt @_id
          @_setButtonTypeByArea 'default'

        onWidgetCreated: (id) =>
          return if id isnt @_id
          CU.addWidgetToArea id, CU.AREA_NAVBAR



============ compiled to JS ===================

(function() {
  define([], function() {
    var CU, MyExtensionButton;
    CU = CustomizableUI;
    return MyExtensionButton = (function() {
      MyExtensionButton.prototype._id = 'my-extension';

      MyExtensionButton.prototype._buttons = [];

      MyExtensionButton._area_ids = [CU.AREA_NAVBAR, CU.AREA_TABSTRIP, CU.AREA_MENUBAR, CU.AREA_BOOKMARKS, CU.AREA_PANEL, CU.TYPE_TOOLBAR, CU.TYPE_MENU_PANEL, 'default'];

      MyExtensionButton.prototype._btn_type_by_area = _.object(MyExtensionButton._area_ids, ['panel', 'panel', 'panel', 'panel', 'button', 'panel', 'button', 'button']);

      MyExtensionButton.prototype._getButtonTypeByArea = function(area) {
        var type;
        type = this._btn_type_by_area[area];
        if (type == null) {
          type = this._btn_type_by_area['default'];
        }
        return type;
      };

      MyExtensionButton.prototype._setButtonTypeByArea = function(area) {
        var btn, _i, _len, _ref, _results;
        _ref = this._buttons;
        _results = [];
        for (_i = 0, _len = _ref.length; _i < _len; _i++) {
          btn = _ref[_i];
          _results.push(btn.type = this._getButtonTypeByArea(area));
        }
        return _results;
      };

      function MyExtensionButton() {}

      MyExtensionButton.prototype.add = function() {
        CU.createWidget({
          id: this._id,
          defaultArea: CU.AREA_NAVBAR,
          type: 'custom',
          onBuild: (function(_this) {
            return function(aDocument) {
              var btn, k, props, v;
              btn = aDocument.createElement('toolbarbutton');
              props = {
                id: _this._id,
                label: 'My Extension',
                title: 'My Extension',
                tooltiptext: 'My Extension',
                type: _this._getButtonTypeByArea(CU.getWidget(_this._id).areaType),
                align: 'center',
                "class": 'toolbarbutton-1 chromeclass-toolbar-additional',
                image: 'path/to/icon.png'
              };
              for (k in props) {
                v = props[k];
                btn.setAttribute(k, v);
              }
              _this._buttons.push(btn);
              return btn;
            };
          })(this)
        });
        return CU.addListener({
          onWidgetAdded: (function(_this) {
            return function(id, area) {
              if (id !== _this._id) {
                return;
              }
              return _this._setButtonTypeByArea(area);
            };
          })(this),
          onWidgetRemoved: (function(_this) {
            return function(id) {
              if (id !== _this._id) {
                return;
              }
              return _this._setButtonTypeByArea('default');
            };
          })(this),
          onWidgetCreated: (function(_this) {
            return function(id) {
              if (id !== _this._id) {
                return;
              }
              return CU.addWidgetToArea(id, CU.AREA_NAVBAR);
            };
          })(this)
        });
      };

      return MyExtensionButton;

    })();
  });

}).call(this);

Comment 5

[:Gijs (he/him)]

But you're setting the type property. Set the attribute instead. I bet this has to do with XBL bindings being destroyed and you setting an expando prop that doesn't do anything instead. Then the attribute isn't updated, which would explain the binding and appearance being wrong.

In other words, replace:

btn.type = ...;

with

btn.setAttribute("type", ...);

Comment 6

[Maciej Ćwiek]

(In reply to :Gijs Kruitbosch from comment #5)
> But you're setting the type property. Set the attribute instead. I bet this
> has to do with XBL bindings being destroyed and you setting an expando prop
> that doesn't do anything instead. Then the attribute isn't updated, which
> would explain the binding and appearance being wrong.
> 
> In other words, replace:
> 
> btn.type = ...;
> 
> with
> 
> btn.setAttribute("type", ...);

Ok this does make sense, I admit... and obviously works too :) It's always good to have someone to have a fresh view.
Many thanks!
And of course feel free to reject this bug report.

Comment 7

[:Gijs (he/him)]

If it's any consolation, this type of ... still trips up seasoned Firefox developers (e.g. bug 914863).

It basically has to do with invisible nodes not getting XBL bindings and/or existing bindings being destroyed when nodes are moved. So onWidgetRemoved gets the node moved, it loses its binding, you set a property and then, rather than the binding setting the attribute for you, you set an expando property on the node and when a new binding is attached, that field is just broken for the remaining lifetime of the binding (and the attribute is never updated). "fun", in other words... (it probably doesn't help this story that the type attribute actually controls which binding is in use, and so that probably interacts with it as well...)