Page MenuHomePhabricator

DropdownInputWidget with MenuSectionOptionWidget triggers endless change events
Closed, ResolvedPublic

Description

I've experienced an endless loop when selecting an item in the browser in a DropdownInputWidget that contains a MenuSectionOptionWidget. The loop repeats

  • selectItem (SelectWidget.js:755)
  • setValue (DropdownInputWidget.js:104)
  • onMenuSelect (DropdownInputWidget.js:92)
  • emit (jQuery)

Looking at the code, I found one possible cause of this: in SelectWidget.selectItem, the check is this.item[ i ].isSelected() === selected. However, for MenuSectionOptionWidget items, the selected property is absent, leading to the variable changed in selectItem to be always set (because false !== undefined), triggering the "select" event emission.

My suggestion for a fix would be to set this.selected to false in the MenuSectionOptionWidget constructor. Relying on the parent constructors wouldn't work since they call this.setSelected in the constructor, which skips setting the selected property when the static selectable property is set.

Another possible fix would be to change the isSelected method to return !!this.selected. However, the double negation could have performance impacts.

My colleagues could not reproduce my error by manually user testing. On the current Wikipedia the error is also absent. But several pull requests for AdvancedSearch, with changes unrelated to the dropdown selection, have started to fail:

I can provide a patch for MenuSectionOptionWidget, but would also be interested in the "root cause" - the OOUI change that triggered this. So far I did not find any.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2019, 7:00 PM
Volker_E triaged this task as Normal priority.Mar 20 2019, 2:46 AM
gabriel-wmde updated the task description. (Show Details)Mar 20 2019, 1:22 PM

Change 497805 had a related patch set uploaded (by Gabriel Birke; owner: Gabriel Birke):
[oojs/ui@master] Avoid select events for MenuSectionOptionWidget

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

Thanks for investigating, it certainly looks like you're right!

There were some changes recently to allow multiple selection in SelectWidget – maybe that somehow uncovered the issue.

Change 497805 merged by jenkins-bot:
[oojs/ui@master] Avoid select events for MenuSectionOptionWidget

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

Volker_E moved this task from Backlog to OOUI-0.31.1 on the OOUI board.Mar 20 2019, 8:39 PM
Volker_E edited projects, added OOUI (OOUI-0.31.1); removed OOUI.

Change 497947 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] OOUI: Bring forward UBN fix for DropdownInputWidget with MenuSectionOptionWidget

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

There were some changes recently to allow multiple selection in SelectWidget – maybe that somehow uncovered the issue.

Actually, those are not related, these changes were made after the 0.31.0 release.

The actual problem is this change: https://gerrit.wikimedia.org/r/c/oojs/ui/+/496078/1/src/widgets/OptionWidget.js causing this.selected to not always be set.

Change 497947 merged by jenkins-bot:
[mediawiki/core@master] OOUI: Bring forward UBN fix for DropdownInputWidget with MenuSectionOptionWidget

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

Change 497949 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@wmf/1.33.0-wmf.22] OOUI: Bring forward UBN fix for DropdownInputWidget with MenuSectionOptionWidget

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

Change 497949 merged by jenkins-bot:
[mediawiki/core@wmf/1.33.0-wmf.22] OOUI: Bring forward UBN fix for DropdownInputWidget with MenuSectionOptionWidget

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

Mentioned in SAL (#wikimedia-operations) [2019-03-20T23:49:24Z] <jforrester@deploy1001> Synchronized php-1.33.0-wmf.22/resources/lib/ooui/oojs-ui-core.js: SWAT T218722 T218830 Bring forward UBN OOUI fix (duration: 00m 57s)

Jdforrester-WMF closed this task as Resolved.Mar 20 2019, 11:50 PM
Jdforrester-WMF claimed this task.
Jdforrester-WMF reassigned this task from Jdforrester-WMF to gabriel-wmde.
Jdforrester-WMF removed a project: Patch-For-Review.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Gabriel gets the glory, not me. :-)

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

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

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

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