Page MenuHomePhabricator

Issues with OOjs UI's new WindowInstance code preventing some VE dialogs from working
Closed, ResolvedPublic8 Estimated Story Points

Event Timeline

Looks like we should have done this before merging OOUI 0.22.0. :-(

So what was marked as deprecating was actually a breaking change? Then we either need to revert or do a patch release of ooui.

Inspectors throw exceptions if I press 'cancel' - that's too broken to not revert.

Inspectors throw exceptions if I press 'cancel' - that's too broken to not revert.

Something is pretty broken in VE here… somehow ve.ui.WindowManager#closeWindow is calling itself recursively, breaking some assumptions in OOjs UI code.

That said, OOjs UI is also broken. Trying to close a window that is already being closed should fail gracefully, not cause exceptions.

Alright, OOjs UI v0.22.0 really is broken. The new code has a couple of bugs we missed, mostly related to closing windows. There's nothing wrong with the WindowInstance concept (and nothing we should need to fix in VE), they are just bugs.

I'll be submitting a few patches. We should probably release v0.22.1 today (although I'd be fine with applying local patches in MediaWiki and VisualEditor too).

matmarex renamed this task from Make VE dialogs work using OOjs UI's new WindowInstance concept to Issues with OOjs UI's new WindowInstance code preventing some VE dialogs from working.May 31 2017, 11:56 AM
matmarex added a project: OOUI.

Change 356369 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Fix check for a window already closing

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

Change 356370 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Fix incorrect checks for promise state

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

Change 356371 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Provide .done() and .fail() in the backwards-compatible promise too

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

[oojs/ui@master] WindowManager: Fix check for a window already closing https://gerrit.wikimedia.org/r/356369

This fixes the exception when closing an inspector.

[oojs/ui@master] WindowManager: Fix incorrect checks for promise state https://gerrit.wikimedia.org/r/356370

I don't think the problem here was causing any visible issues, but it's still broken. I noticed this when writing the previous patch.

[oojs/ui@master] WindowManager: Provide .done() and .fail() in the backwards-compatible promise too https://gerrit.wikimedia.org/r/356371

This fixes a test failure that James sneakily worked around with https://gerrit.wikimedia.org/r/#/c/356316/3/tests/ui/dialogs/ve.ui.FindAndReplaceDialog.test.js.

Change 356369 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix check for a window already closing

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

Change 356370 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix incorrect checks for promise state

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

Change 356395 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Fix important typo in deprecation warning

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

Change 356396 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Do not use return value of #closeWindow as promise

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

Change 356397 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] WindowManager: Fix error handling for #openWindow with string argument

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

These three patches do not affect VE specifically, but they are also fixing bugs caused by the same changes.

There are enough other consumers of OOUI windows that I wouldn't be happy with just patching VE+MWcore.

Change 356371 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Provide other jQuery.Promise methods on the b/c promise too

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

Change 356395 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix important typo in deprecation warning

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

Change 356396 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Do not use return value of #closeWindow as promise

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

Change 356397 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix error handling for #openWindow with string argument

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