Page MenuHomePhabricator

OOUI window ready process runs before window is actually ready (Category drop down list gets sticky when it's open while closing the dialog)
Closed, ResolvedPublic

Description

Steps to reproduce:

  1. Go to Page Settings>Categories
  2. Type something to search for a category
  3. When the drop down appears, close the dialog
  4. Open the dialog again

Observed Result:
The drop down is still open in a displaced manner.

Details

Event Timeline

Ryasmeen created this task.Jan 29 2018, 9:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 29 2018, 9:14 PM
matmarex claimed this task.Jun 21 2018, 7:50 PM
matmarex edited projects, added VisualEditor (Current work), OOUI; removed VisualEditor.
matmarex added a subscriber: matmarex.

This superficially looks similar to T155976, but I think it has a different root cause. Looks like there is a bug in OOUI where it won't wait for a window to be fully open before focussing fields inside of it :(

Change 441482 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] Ensure window ready process runs after window is made visible

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

@Ryasmeen It seems that this has always been broken, but no one noticed before you :)

One important takeaway from the refactor: If the ready process adds some content to the window, this will now be visibly delayed (by 250 ms). Code doing that should be in the setup process. I see a few places in VE where we obviously do that wrong, but there may be others (or in other code), so this is something worth QA-ing after the OOUI change is deployed.

Change 441489 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] Fix confusion between #getSetupProcess and #getReadyProcess

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

Change 441490 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Fix confusion between #getSetupProcess and #getReadyProcess

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

Volker_E moved this task from Backlog to Doing on the OOUI board.Jun 27 2018, 7:59 AM
matmarex triaged this task as Medium priority.Jul 5 2018, 4:16 PM

Change 441482 merged by jenkins-bot:
[oojs/ui@master] Ensure window ready process runs after window is made visible

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

@Ryasmeen The OOUI refactoring has also revealed a few other minor issues in VisualEditor, which are now visible on https://en.wikipedia.beta.wmflabs.org/:

  • Page options → Templates used (and other options, except the first one) opens the first panel first, then visibly flashes to the right panel
  • Insert → Template opens as empty dialog first, then the form flashes in
  • Welcome dialog first opens with three buttons, then some of them disappear (depending on whether page is visually-editable)

Quick recording for reference: https://drive.google.com/file/d/1mJhmKQOe8eEblFmQhnsNqB57tsEOdAyJ/view?usp=sharing (too large for Phabricator)

All of these are fixed by my patches above (pending review).


Similar issues might appear in other code using OOUI dialogs. I think it's unnecessary to try to check them all (it's a lot of code, and the issues, if any, are just minor visual glitches like these).

However, if QA or anyone else spots a problem like this (contents or buttons of a dialog somehow rearranging themselves immediately after it opens), please know that this is the root cause, and the issue is fixed by moving initialization code from #getReadyProcess to #getSetupProcess. #getReadyProcess should basically only contain code that focusses/activates something in the dialog. I'm not sure if we should try to announce this somewhere.

Change 441489 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Fix confusion between #getSetupProcess and #getReadyProcess

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

Change 441490 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Fix confusion between #getSetupProcess and #getReadyProcess

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

Change 446218 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (8d0ab0587)

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

@Ryasmeen The OOUI refactoring has also revealed a few other minor issues in VisualEditor, which are now visible on https://en.wikipedia.beta.wmflabs.org/:

  • Page options → Templates used (and other options, except the first one) opens the first panel first, then visibly flashes to the right panel
  • Insert → Template opens as empty dialog first, then the form flashes in
  • Welcome dialog first opens with three buttons, then some of them disappear (depending on whether page is visually-editable)

Quick recording for reference: https://drive.google.com/file/d/1mJhmKQOe8eEblFmQhnsNqB57tsEOdAyJ/view?usp=sharing (too large for Phabricator)

All of these are fixed by my patches above (pending review).

Similar issues might appear in other code using OOUI dialogs. I think it's unnecessary to try to check them all (it's a lot of code, and the issues, if any, are just minor visual glitches like these).
However, if QA or anyone else spots a problem like this (contents or buttons of a dialog somehow rearranging themselves immediately after it opens), please know that this is the root cause, and the issue is fixed by moving initialization code from #getReadyProcess to #getSetupProcess. #getReadyProcess should basically only contain code that focusses/activates something in the dialog. I'm not sure if we should try to announce this somewhere.

@matmarex : I checked the original issue and the issues you mentioned above. They all look fixed to me. However, today I found an issue with media settings dialog T199841 which sounds like what you described as buttons of a dialog rearranging themselves immediately after it opens. Not sure if it is relevant? Didn't observe this issue in any other dialog though.

@Ryasmeen Thanks, that is caused by this change as well! I did not notice the problem in that dialog.

Change 446516 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWMediaDialog: Fix confusion between #getSetupProcess and #getReadyProcess

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

Change 446218 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (8d0ab0587)

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

Change 446516 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWMediaDialog: Fix confusion between #getSetupProcess and #getReadyProcess

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

Change 446529 had a related patch set uploaded (by Jforrester; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.13] ve.ui.MWMediaDialog: Fix confusion between #getSetupProcess and #getReadyProcess

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

matmarex renamed this task from Category drop down list gets sticky when it's open while closing the dialog to OOUI window ready process runs before window is actually ready (Category drop down list gets sticky when it's open while closing the dialog).Jul 18 2018, 10:52 PM

Change 446529 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.13] ve.ui.MWMediaDialog: Fix confusion between #getSetupProcess and #getReadyProcess

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

Mentioned in SAL (#wikimedia-operations) [2018-07-18T23:33:02Z] <thcipriani@deploy1001> Synchronized php-1.32.0-wmf.13/extensions/VisualEditor/modules/ve-mw/ui/dialogs/ve.ui.MWMediaDialog.js: SWAT: [[gerrit:446529|ve.ui.MWMediaDialog: Fix confusion between #getSetupProcess and #getReadyProcess]] T185944 T199841 (duration: 00m 56s)

Deskana closed this task as Resolved.Jul 30 2018, 3:26 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJul 30 2018, 3:26 PM