Page MenuHomePhabricator

Document the differences between MultioptionWidget, MultiselectWidget, SelectWidget
Open, MediumPublic

Description

In OOUI, we have three "base" classes that define the behavior of choosing between multiple items:

  • OO.ui.SelectWidget
  • OO.ui.MultiselectWidget
  • OO.ui.MultioptionWidget

All of the above inherit directly from OO.ui.Widget and are standalone, so even OO.ui.MultiselectWidget is not a SelectWidget. That's another confusing factor.

It is extremely unclear what the differences are between them, and they have weirdly different behaviors. Some of those behaviors may be the valid differences, but some may "just" be a matter of inconsistencies that we should fix, and it's really hard to tell which is which.

For example,

  • What is the difference between OO.ui.CheckboxMultioptionWidget vs OO.ui.CheckboxMultiselectWidget ?
  • Event mismatch;
    • ButtonSelectWidget and OO.ui.RadioSelectWidget inherit from SelectWidget and emit either choose or select events
    • OO.ui.CheckboxMultioptionWidget inherits from OO.ui.MultioptionWidget and emits a change event that has the same purpose as choose/select but no distinction between the originator.
    • OO.ui.CheckboxMultiselectWidget inherits from OO.ui.MultiselectWidget and emits either a change or a select event, but change is emitted not just for change of state, but also for change of elements

The inconsistencies with event and with the purpose is making it really hard to decide when to use which widget and which base widget to use to extend and create another selection-based widget.

We need to review those and decide:

  1. Is it valid to have three types of base selection widgets at all?
  2. If it is valid, how can we make sure that we properly document the differences between the base widgets and the top widgets that use either?
  3. Is it reasonable to merge terminology and make sure that a "change" and "select" (and, potentially, "choose") event are relatively consistent with what the user of those widget expects? can we consolidate terminology and behavior to be more consistent?

Event Timeline

CheckboxMultiselectWidget implementation task was T117782.

From IRC:

MatmaRex> mooeypoo: CheckboxMultioptionWidget represents one option inside a CheckboxMultiselectWidget. same as WhateverOptionWidget represents one option inside a WhateverSelectWidget.
<•MatmaRex> i wanted TagMultiselectWidget and CheckboxMultiselectWidget to inherit from MultiselectWidget, but i never had time to work on doing this for TagMultiselectWidget. that's why MultiselectWidget exists with only one subclass

Change 489139 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[oojs/ui@master] docs: Fix URI in description

https://gerrit.wikimedia.org/r/489139

Okay, so I think that it might be a good idea to slightly rename things, and potentially get a bit of consistency with the events.

From IRC:

MatmaRex> mooeypoo: CheckboxMultioptionWidget represents one option inside a CheckboxMultiselectWidget. same as WhateverOptionWidget represents one option inside a WhateverSelectWidget.

If that's the case, then we should maybe renamed CheckboxMultioptionWidget to CheckboxOptionWidget or CheckboxItemWidget, since either one of those are understood to be "an item inside something else", while the word "multioption" sounds like it contains multiple options.

I know that technically this widget isn't extending the OptionWidget so calling it "CheckboxOptionWidget" is potentially misleading, but we have other widgets that don't quite follow the extend pattern (see 'MultiselectWidget, which doesn't really extend SelectWidget) and I think the current situation is a lot more confusing than if we changed the name to CheckboxOptionWidget.

<•MatmaRex> i wanted TagMultiselectWidget and CheckboxMultiselectWidget to inherit from MultiselectWidget, but i never had time to work on doing this for TagMultiselectWidget. that's why MultiselectWidget exists with only one subclass

I don't know if TagMultiselectWidget can inherit from MultiselectWidget,that's an interesting idea we should look into, if needed.

That said, the original reason why I looked into it is because of event mismatch and confusion, specifically with Checkbox*Widget(s). I think we should fix things in batches:

  1. For consistency and clarity, change CheckboxMultioptionWidget to CheckboxOptionWidget
  2. Make sure the "holder" widgets -- the ones that hold groups of items -- MultiselectWidget in this case, emits the same events that are expected from SelectWidget (meaning -- 'choose' for intentional user action and 'select' for all selection changes) so the API for both is the same consistency.
  3. Make Checkbox*Widget (input and non input) separate the events to user-created vs code-created with choose/select (see T215546: OO.ui.CheckboxInputWidget should emit a different event for user-action which is where this investigation began)

Change 489139 merged by jenkins-bot:
[oojs/ui@master] docs: Fix URI in description

https://gerrit.wikimedia.org/r/489139

Change 491943 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] Update OOUI to v0.30.3

https://gerrit.wikimedia.org/r/491943

Change 491943 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.30.3

https://gerrit.wikimedia.org/r/491943

Volker_E triaged this task as Medium priority.Feb 25 2019, 11:07 PM