Change Details

>>! In T433188#12169809, @Bhsd wrote: >>>! In T433188#12169804, @DLynch wrote: >> I can't reproduce not being able to open the dialog with a selection. > > In my test, the issue exists in both Firefox and Chrome on a MacBook. Neither the menu button nor the keyboard shortcut opens the CodeMirror preference dialog when the selection in non-empty. >>! In T433188#12169860, @DLynch wrote: > Okay, the dialog not opening is more interesting. > > There's one option that's actually a long-standing VE-source bug: the options dialog is a FragmentDialog, and unless explicitly told //not// to, when in source mode they convert your selection from wikitext into a VE data model via a server round-trip to get it from Parsoid. This is a no-op if you have no selection, but can take a short while before it'll open the dialog. Crucially, it can also fail, and if it does then the dialog won't open. > > If this is your issue, you'll have a network request in the background to api.php for action=visualeditor paction=parsefragment that's flagged as a non-200 response. (Other questions: does it fail the same way with syntax highlighting turned off? How much text were you selecting? Any console errors? If you do this in the console first does that fix it: `ve.ui.MWMetaDialog.static.handlesSource = true`?) Further conversation revealed that this was an outdated parsoid version, causing all parsefragment calls to fail. This is unusually bad -- normally this would only happen in situations where the network connection was getting dropped, where the user might at least be less surprised that things started going wrong. The more common bad case is probably slow connections where things take a while to open in NWE for no obvious reason. There's a `TODO: Show progress bar without breaking WindowAction` which would expose that were it ever implemented. Technically, `ve.ui.MWMetaDialog.static.handlesSource = true` would have solved this specific case. However, although //we// don't do this internally, codesearch suggests that there's other extensions out there that do (and probably in a buggy way; looks like the BlueSpice cluster would maybe cause some weird side-effects if someone with them installed used the options dialog while in source mode) that wouldn't respond well to that changing. Since this failure is unrecoverable (the dialog claims to need the fragment, and it cannot be generated without that API call), the initial problem is that we give no feedback, and actively swallow the error. We should make it so that a failed fragment conversion logs the that an error happened to the console instead of failing silently. It's non-obvious, but people can find it and report it rather than having literally no evidence of what happened. (There's an additional side-effect that the conversion's position in the chain stalls the WindowAction#open promise... but nothing in this chain seems to actually be bothered by that. Probably still good to clean it up, though.) Theoretically we could be more proactive about saying a request failed if we wanted, via `mw.notify` or similar, but it's also not a situation where we can really give someone actionable advice. "We know something went wrong, sorry" might be appreciated, I guess...

>>! In T433188#12169809, @Bhsd wrote: >>>! In T433188#12169804, @DLynch wrote: >> I can't reproduce not being able to open the dialog with a selection. > > In my test, the issue exists in both Firefox and Chrome on a MacBook. Neither the menu button nor the keyboard shortcut opens the CodeMirror preference dialog when the selection in non-empty. >>! In T433188#12169860, @DLynch wrote: > Okay, the dialog not opening is more interesting. > > There's one option that's actually a long-standing VE-source bug: the options dialog is a FragmentDialog, and unless explicitly told //not// to, when in source mode they convert your selection from wikitext into a VE data model via a server round-trip to get it from Parsoid. This is a no-op if you have no selection, but can take a short while before it'll open the dialog. Crucially, it can also fail, and if it does then the dialog won't open. > > If this is your issue, you'll have a network request in the background to api.php for action=visualeditor paction=parsefragment that's flagged as a non-200 response. (Other questions: does it fail the same way with syntax highlighting turned off? How much text were you selecting? Any console errors? If you do this in the console first does that fix it: `ve.ui.MWMetaDialog.static.handlesSource = true`?) Further conversation revealed that this was an outdated parsoid version, causing all parsefragment calls to fail. This is unusually bad -- normally this would only happen in situations where the network connection was getting dropped, where the user might at least be less surprised that things started going wrong. The more common bad case is probably slow connections where things take a while to open in NWE for no obvious reason. There's a `TODO: Show progress bar without breaking WindowAction` which would expose that were it ever implemented. Technically, `ve.ui.MWMetaDialog.static.handlesSource = true` would have solved this specific case. However, although //we// don't do this internally, codesearch suggests that there's other extensions out there that do (and probably in a buggy way; looks like the BlueSpice cluster would maybe cause some weird side-effects if someone with them installed used the options dialog while in source mode) that wouldn't respond well to that changing. Since this failure is unrecoverable (the dialog claims to need the fragment, and it cannot be generated without that API call), the initial problem is that we give no feedback, and actively swallow the error. We should make it so that a failed fragment conversion logs the that an error happened to the console instead of failing silently. It's non-obvious, but people can find it and report it rather than having literally no evidence of what happened. (There's an additional side-effect that the conversion's position in the chain stalls the WindowAction#open promise... but nothing in this chain seems to actually be bothered by that. Probably still good to clean it up, though.) Theoretically we could be more proactive about saying a request failed if we wanted, via `mw.notify` or similar, but it's also not a situation where we can really give someone actionable advice. "We know something went wrong, sorry" might be appreciated, I guess... --- # Derived Requirement Ensure that when wikitext fragment conversion fails in source mode, the failure is logged to the browser console instead of failing silently. ([Phabricator][1]) **Test Steps** **Preconditions** 1. Open a page in the 2017 wikitext editor. 2. Open the browser Developer Tools console. 3. Create conditions that cause the `action=visualeditor&paction=parsefragment` request to fail. **Test Case 1: Ensure failed fragment conversion is logged to the console** 1. Select non-empty wikitext in the source editor. 2. Trigger a dialog that requires conversion of the selected wikitext fragment. 3. Cause the fragment conversion request to fail. 4. Observe that the dialog does not open. 5. Review the browser console. 6. **✅❓❌⬜ **AC1:** A failed wikitext fragment conversion produces an error in the browser console instead of failing without any logged error.** == QA Results - PROD | **AC** | **Status** | **Details** | | ----- | ----- | ----- | | 1 | ✅|T433560#12254544

>>! In T433188#12169809, @Bhsd wrote: >>>! In T433188#12169804, @DLynch wrote: >> I can't reproduce not being able to open the dialog with a selection. > > In my test, the issue exists in both Firefox and Chrome on a MacBook. Neither the menu button nor the keyboard shortcut opens the CodeMirror preference dialog when the selection in non-empty. >>! In T433188#12169860, @DLynch wrote: > Okay, the dialog not opening is more interesting. > > There's one option that's actually a long-standing VE-source bug: the options dialog is a FragmentDialog, and unless explicitly told //not// to, when in source mode they convert your selection from wikitext into a VE data model via a server round-trip to get it from Parsoid. This is a no-op if you have no selection, but can take a short while before it'll open the dialog. Crucially, it can also fail, and if it does then the dialog won't open. > > If this is your issue, you'll have a network request in the background to api.php for action=visualeditor paction=parsefragment that's flagged as a non-200 response. (Other questions: does it fail the same way with syntax highlighting turned off? How much text were you selecting? Any console errors? If you do this in the console first does that fix it: `ve.ui.MWMetaDialog.static.handlesSource = true`?) Further conversation revealed that this was an outdated parsoid version, causing all parsefragment calls to fail. This is unusually bad -- normally this would only happen in situations where the network connection was getting dropped, where the user might at least be less surprised that things started going wrong. The more common bad case is probably slow connections where things take a while to open in NWE for no obvious reason. There's a `TODO: Show progress bar without breaking WindowAction` which would expose that were it ever implemented. Technically, `ve.ui.MWMetaDialog.static.handlesSource = true` would have solved this specific case. However, although //we// don't do this internally, codesearch suggests that there's other extensions out there that do (and probably in a buggy way; looks like the BlueSpice cluster would maybe cause some weird side-effects if someone with them installed used the options dialog while in source mode) that wouldn't respond well to that changing. Since this failure is unrecoverable (the dialog claims to need the fragment, and it cannot be generated without that API call), the initial problem is that we give no feedback, and actively swallow the error. We should make it so that a failed fragment conversion logs the that an error happened to the console instead of failing silently. It's non-obvious, but people can find it and report it rather than having literally no evidence of what happened. (There's an additional side-effect that the conversion's position in the chain stalls the WindowAction#open promise... but nothing in this chain seems to actually be bothered by that. Probably still good to clean it up, though.) Theoretically we could be more proactive about saying a request failed if we wanted, via `mw.notify` or similar, but it's also not a situation where we can really give someone actionable advice. "We know something went wrong, sorry" might be appreciated, I guess... --- # Derived Requirement Ensure that when wikitext fragment conversion fails in source mode, the failure is logged to the browser console instead of failing silently. ([Phabricator][1]) **Test Steps** **Preconditions** 1. Open a page in the 2017 wikitext editor. 2. Open the browser Developer Tools console. 3. Create conditions that cause the `action=visualeditor&paction=parsefragment` request to fail. **Test Case 1: Ensure failed fragment conversion is logged to the console** 1. Select non-empty wikitext in the source editor. 2. Trigger a dialog that requires conversion of the selected wikitext fragment. 3. Cause the fragment conversion request to fail. 4. Observe that the dialog does not open. 5. Review the browser console. 6. **✅❓❌⬜ **AC1:** A failed wikitext fragment conversion produces an error in the browser console instead of failing without any logged error.** == QA Results - PROD | **AC** | **Status** | **Details** | | ----- | ----- | ----- | | 1 | ✅|T433560#12254544