Details
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • demon | T166829 MW-1.30.0-wmf.4 deployment blockers | |||
| Resolved | • matmarex | T166631 Issues with OOjs UI's new WindowInstance code preventing some VE dialogs from working |
Event Timeline
So what was marked as deprecating was actually a breaking change? Then we either need to revert or do a patch release of ooui.
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).
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
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
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
This fixes the exception when closing an inspector.
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.
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
Change 356370 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix incorrect checks for promise state
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
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
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
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
Change 356395 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix important typo in deprecation warning
Change 356396 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Do not use return value of #closeWindow as promise
Change 356397 merged by jenkins-bot:
[oojs/ui@master] WindowManager: Fix error handling for #openWindow with string argument
Change 994733 had a related patch set uploaded (by Jforrester; author: Jforrester):
[oojs/ui@master] [BREAKING CHANGE] WindowManager: Drop Thenable wrapper for lifecycle, deprecated in v0.22.0