1026000 - Documentation for setting `checked` property in ToggleButton is wrong

Status: RESOLVED FIXED

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

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

I'll close it as INVALID.
The code is actually doing what expected; the test was made changing the global state of a toggle button from the button itself, after clicking on a different window, without removing the window state, and that's where the discrepancy happens. To be clear:

1. Buttons have a hierarchy of states; global, per window, per tab: a tab state for a specific property override the window state for the same property, and so on.

2. Toggle Button have a special property called "checked", because can be set by user interaction just clicking on the button. Clicking on the button set the "checked" property on the window state to `true` or `false`.

So basically if we set a global checked property (toggle.checked = true) where we already clicked on some checked button, the window state will takes the precedence, looks like the UI is not updated (see the subject).

So, it's correct that `checked` can be set by both global property (`toggle.state`) and `state` method (`toggle.state`), because that was intended to: it's also the only way we have to create toggle button per tab and per "browser" (globally). But, because it's also a property that can be set by User Interaction, and because the hierarchy of the states, developers needs to be careful about that.
This discussion was made during the development of this API, with Jeff and Mossop, and was decided to makes simple the major use case (per window), and let the developers have the possibility to deal with the rest (tab, global), and take care of the discrepancy.

We should probably document that, and add some example to obtain a global toggle button and a tab toggle button, as follow:

const { ToggleButton } = require('sdk/ui/button/toggle');

let globalToggle = ToggleButton({
  id: 'my-global-toggle',
  label: 'global function',
  icon: './foo.png',
  onChange: function() {
    // delete the window state for the current window,
    // automatically set when the user click on the button
    this.state('window', null);
    
    // now that the state hierarchy is clean, set the
    // global state
    this.checked = !this.checked;
  }
});


let tabToggle = ToggleButton({
  id: 'my-global-toggle',
  label: 'global function',
  icon: './foo.png',
  onChange: function() {
    // delete the window state for the current window,
    // automatically set when the user click on the button
    this.state('window', null);
    
    // now that the state hierarchy is clean, set the
    // tab state for the current tab
    let { checked } = tab.state('tab');
    tab.state('tab', {checked: !checked});
  }
});

Will, should I open a new bug for documenting that? Thanks!

Comment 2

[Will Bamberg [:wbamberg]]

Let's make this into a docs bug, then :). Thanks for the clarification Matteo, it makes sense. This stuff is a bit confusing though.

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Will Bamberg [:wbamberg] from comment #2)
> Let's make this into a docs bug, then :). Thanks for the clarification

Can I assign this bug to you then?

Comment 4

[Will Bamberg [:wbamberg]]

Yes!

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

There is a typo in the second, example, here the correct version:

let tabToggle = ToggleButton({
  id: 'my-global-toggle',
  label: 'global function',
  icon: './foo.png',
  onChange: function() {
    // delete the window state for the current window,
    // automatically set when the user click on the button
    this.state('window', null);
    
    // now that the state hierarchy is clean, set the
    // tab state for the current tab
    let { checked } = this.state('tab');
    this.state('tab', {checked: !checked});
  }
});

Comment 6

[Will Bamberg [:wbamberg]]

Attachment: mdn.html

I've expanded the section on updating state and added a new section on updating `checked`, including the 2 examples you wrote here. Let me know if this is right...

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8508954 [details]
mdn.html

LGTM, thanks!