Page MenuHomePhabricator

WikiEditor dialogs kill the undo buffer
Closed, ResolvedPublic

Description

In WikiEditor, if you use the Link dialog (or any other 'dialog') to insert text into the edit box, then press control-Z, the edit does not get undone. This is true of all the supplied dialogs, including one I custom-developed.

Affected browsers:

  • Confirmed in IE 8.0.7600.16385 and Chrome 14.0.835.202m, on English Wikipedia today.
  • Works correctly in Firefox 7.0.1.
  • Chromium 68.0.3440 - even worse (ctrl+Z changes wrong text taht was never edited)
  • Firefox 62.0 - Killed the undo buffer --------------------------

See Also: T37887

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 474520 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/core@master] textSelection: Use execcommand to replace text

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

Change 474520 merged by jenkins-bot:
[mediawiki/core@master] textSelection: Use execcommand to replace text

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

Jdforrester-WMF assigned this task to TheDJ.

Worked around for everyone except Firefox. Hopefully Mozilla will eventually fix that issue, but our work here is done.

matmarex added a subscriber: matmarex.

Reverted in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/478734 due to concerns about compatibility with alternative editors that override the 'textSelection' API. This should be relatively easy to fix (see comments on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/474520), but somewhat time-consuming to verify that everything works correctly. Extensions to test: CodeEditor, CodeMirror, ProofreadPage, VisualEditor (NWE).

Change 478744 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/core@master] Revert "Revert "textSelection: Use execcommand to replace text""

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

TheDJ removed TheDJ as the assignee of this task.Jan 29 2020, 11:18 PM

I cannot reproduce this issue in IE11 and earlier IE versions are not supported as WikiEditor is a Javascript component (as I assume that it follows the MW browser support matrix). Does the title need to be updated to show that another supported browser is still having an issue or should this task be resolved?

Note that Firefox's bug with document.execCommand('insertText', ...) was fixed 3 months ago: https://bugzilla.mozilla.org/show_bug.cgi?id=1220696.

A lot of users will be extremely happy if all tests of alternative editors are done and this goes live :-)

I can’t reproduce with Firefox 91 and Chromium 90 too.

@Jack_who_built_the_house which browser do you use to reproduce this issue?

@Pols12 You can repeat the steps in any browser. You just have to disable syntax-highlighting. The problem is that functions inserting text don't use document.execCommand which was mentioned by Jack.

This Wikieditor doesn't work:
https://www.mediawiki.org/wiki/Extension:WikiEditor#/media/File:VectorEditorBasic-en.png

Easiest steps (same in Chrome 100, Firefox 98):

  1. Open private mode in your browser (or log out).
  2. Find some article to you can edit as IP e.g.: https://en.wikipedia.org/w/index.php?title=Wikipedia_Monument&action=edit
  3. Select any word.
  4. Press B(old) on the tollbar.
  5. Press Ctrl+Z on your keyboard.

Note that the problem is not in the link dialog. It is in the method of inserting text.

You can find an example of using execCommand here:
https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand#using_inserttext

The code is actually mine. You can use it freely. Note however that it is missing logic of surrounding selected text.

Sorry. I haven't noticed this used to work and was reverted. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/478734/1/resources/src/jquery/jquery.textSelection.js

In any case. Currently undo doesn't work in textarea based editor.

Maybe an easier approach would be to use a different function and gradually replace in buttons that were tested.

Change 478744 merged by jenkins-bot:

[mediawiki/core@master] Re-apply "textSelection: Use execcommand to replace text"

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

I've merged the change. I'm going to watch out for reports about anything related being broken, but I hope it sticks this time.

The recent patch says

function supportsExecCommand() {
   return $( this ).data( 'jquery.textSelection' ) === undefined &&
          typeof document.queryCommandSupported === 'function' &&
          document.queryCommandSupported( 'insertText' );
}

It is queried for function queryCommandSupported() but execCommand() is required. Both functions have different availability histories, and queryCommandSupported() is never used here.

Soemthing went wrong here.

I caught complaints that text insertion does not work any longer since Friday morning, when MW update has been distributed, with some kind of older browser.

At least it is to be checked whether execCommand() is a function which is suggested by supportsExecCommand().

Multiple sources are telling the newly introduced functions are deprecated and should not be used:

I am not involved in this development, but it seems that older browsers will not work the new way, and newer or future ones will not since the new implementation was already obsolete when inserted.

The old method worked robust and stable for a dozen years, with the shortcome of missing undo capability.

Editing and working editing tools as learnt are crucial to authors. If the new things distributed this week are not working for any reason, a robust basic functionality needs to be guaranteed as fallback.

Catching more an more complaints.

Seems strange. Maybe we can check the frontend logging see if we can find what is failing on those older browsers.

It is queried for function queryCommandSupported() but execCommand() is required. Both functions have different availability histories, and queryCommandSupported() is never used here.

But queryCommandSupported should be supported by all browsers that support execCommand.
And it is used, it's right there on the next line: document.queryCommandSupported( 'insertText' ); aka. "does execCommand support the insertText command"

I see someone reported it not working in Chrome. I tested with en.wp's 2010 wikieditor, using both special characters from the menu and with the special characters inserter below the editor. Both worked for me in Chrome 103.
Another person reported failures with Seamonkey (based on Firefox 68). I'll see if I can reproduce that.

Some more exact reproduction steps would be appreciated, we have a lot of different insert character flows, different editors and different websites.

Edit: Could reproduce on de.wp and en.wp. Happens when attempting to use the "Insert template" widget of the 2010 wikieditor when the syntax highlighting is initially disabled. Does not seem to effect insertCharacter/Symbol flows. Also works when initially uses syntaxhighlighting.

Possibly related to setting the initial selection/insertionpoint ?? Or specific to the jquery.textSelection encapsulateSelection command, which is used by the template editor ???

Thanks for looking into it @TheDJ, sorry if you weren't expecting to suddenly be reminded of this one :) If my help is still needed, I'll have a look Monday and try to fix or revert, I don't think the problem is severe enough to need more urgency.

Support for insertText via execCommand was implemented in Firefox 89. See:
https://bugzilla.mozilla.org/show_bug.cgi?id=1220696

So that would be for example all users on Windows XP that cannot use this. I guess you would have check stats on Windows XP usage. I know there are still libraries in Poland that use it... People really shouldn't be using browsers that old anyway so I wouldn't worry too much, but hey, dev-life ¯\_(ツ)_/¯.

document.queryCommandSupported( 'insertText' );

That returns true in Firefox 52.0 on Windows XP (just checked). I think it's because it used to work.

So unfortunately that is not the correct way to detect support for insertText. If you want to provide fallback that is.

Multiple sources are telling the newly introduced functions are deprecated and should not be used:

That is not entirely true. Mozilla did (re)implement this. Chrome also support this. And if you look at the MDN there is my example there. The deprecation is a notice for execCommand. Which was mostly superseded by other APIs. But there is no replacement for insertText.
https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand#examples

Note that there is also a detection of support there. And in FF52 you will see in console "paste unsuccessful, execCommand not supported".

Feel free to use any part of the code form the example.

A shorter, original example is here:
https://jsbin.com/baxusiw/edit?html,js,console,output

You'll notice I'm using ES5 in original code. Obviously that is better for IE. But reviewer insisted in using ES6 (new MDN policy ¯\_(ツ)_/¯).

On different introduction of features:

Obviously some assumptions about behaviour of browser versions in the field are wrong, even with three years old tools.

I would feel much more calmed by

function supportsExecCommand() {
   return $( this ).data( 'jquery.textSelection' ) === undefined &&
      typeof document.execCommand === 'function' &&
      typeof document.queryCommandSupported === 'function' &&
      document.queryCommandSupported( 'insertText' );
}

However, I would prefer a more robust and defensive approach, inspired by MDN:

replaceSelection: function ( value ) {
   return this.each( function () {
      var allText, currSelection, startPos, endPos,
          legacy = ! supportsExecCommand();
      if ( ! legacy ) {
         try {
            if ( ! document.execCommand( "insertText", false, value ) ) {
               legacy = true;
            }
         } catch ( e ) {
            legacy = true;
         }
      }
      if ( legacy ) {
         allText = $( this ).textSelection( 'getContents' );
         // ... etc.
      }
   } );
},

Actually, supportsExecCommand() is not needed any longer. Those are caught anyway, and if supported it saves a millisecond.

The (non-?)standard functions have apparently no standardization how to behave for readonly or disabled input fields. Syntaxhilite = CodeMirror and e.g. wikEd are using a disabled textarea. $.val() will always change programmtically without caring about interactive access nor visibility.

The advanced generation of this module should detect whether CodeMirror is active and forward all requests to the CodeMirror API rather than <textarea>. Then it can be used as a neutral layer where all character insertion tools or selection finders can operate on a neutral interface, not caring whether CodeMirror has been swittched on or off.

Edit: Could reproduce on de.wp and en.wp. Happens when attempting to use the "Insert template" widget of the 2010 wikieditor when the syntax highlighting is initially disabled. Does not seem to effect insertCharacter/Symbol flows. Also works when initially uses syntaxhighlighting.

Possibly related to setting the initial selection/insertionpoint ?? Or specific to the jquery.textSelection encapsulateSelection command, which is used by the template editor ???

I can't reproduce this.

In T33780#8098575, @Nux wrote:

A shorter, original example is here:
https://jsbin.com/baxusiw/edit?html,js,console,output

You'll notice I'm using ES5 in original code. Obviously that is better for IE. But reviewer insisted in using ES6 (new MDN policy ¯\_(ツ)_/¯).

The code in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/478744/8/resources/src/jquery/jquery.textSelection.js checks the return value in the same way, and implements a fallback. Are we supposed to do try…catch? I thought that's only needed for copy/cut/paste.

If anyone here can reproduce the problem, and understands the problem, please provide a patch. I will do my best to get it merged.

Otherwise, I am going to revert the change tomorrow (in 24 hours from now), and decline the task.

The code in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/478744/8/resources/src/jquery/jquery.textSelection.js checks the return value in the same way, and implements a fallback. Are we supposed to do try…catch? I thought that's only needed for copy/cut/paste.

Oh, in that case that should work fine... Works on Windows XP on Firefox 52 and Chrome 49 (last versions). Coincidently even undo buffer works there because browsers broken it in a later version.

I guess there could be some edge case where document.execCommand is not defined and document.queryCommandSupported is defined but I kind of doubt that...

Tried to repeat DJ's steps for adding templates on enwiki, but also works fine (added via button):

obraz.png (1×1 px, 157 KB)

AFAIK syntax highlighting creates a separate contenteditable field (not a textarea) and then reverts back to plain textarea. So I doubt switching could affect this. You might have an unfocsed field maybe, but that don't seem cause any problems too.

Also I actually have tested execCommand in WP;SK for quite some time and it works fine. Most popular widget on pl.wiki so should have some reports by now. I'm doing try-catch but that is mostly a guard from undefined document.execCommand.
https://pl.wikipedia.org/w/index.php?title=MediaWiki:Gadget-sk.js&oldid=67456666#L-208

One thing that could be added is to check if document.execCommand is a function I guess. Or even add try-catch just in case, but to my knowledge

Change 816862 had a related patch set uploaded (by Gerrit Patch Uploader; author: Nux):

[mediawiki/core@master] Fix for T33780. Should support more edge cases.

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

Added a patch that should harden this.

@matmarex
I noticed that previously only setContents used this.select(). In a patched version this.select() should always be executed. So that might fix some kind of changes of focus. Hopefully ':-)

Also I added try-catch just to harden this. Even thought I'm almost sure this shouldn't be required. All weird cases should be covered now.

Change 816863 had a related patch set uploaded (by Gerrit Patch Uploader; author: Nux):

[mediawiki/core@master] Fix for fix for T33780

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

Focus. Focus was missing, not select. Ups 🙈

Change 816862 abandoned by Bartosz Dziewoński:

[mediawiki/core@master] Fix for T33780. Should support more edge cases.

Reason:

Superseded by https://gerrit.wikimedia.org/r/816863 (in the future, you can include the 'Change-Id: ...' line in your commit message to update the existing change on Gerrit, rather than create a new one)

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

Change 816863 had a related patch set uploaded (by Bartosz Dziewoński; author: Nux):

[mediawiki/core@master] jquery.textSelection: Support more edge cases of document.execCommand

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

Change 816863 merged by jenkins-bot:

[mediawiki/core@master] jquery.textSelection: Support more edge cases of document.execCommand

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

Thanks for the patch @Nux, I'll backport it tomorrow and we'll see if it resolves issues reported by @PerfektesChaos.

Change 817232 had a related patch set uploaded (by Bartosz Dziewoński; author: Nux):

[mediawiki/core@wmf/1.39.0-wmf.21] jquery.textSelection: Support more edge cases of document.execCommand

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

Change 817232 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.21] jquery.textSelection: Support more edge cases of document.execCommand

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

Mentioned in SAL (#wikimedia-operations) [2022-07-26T13:35:54Z] <taavi@deploy1002> Synchronized php-1.39.0-wmf.21/resources/src/jquery/jquery.textSelection.js: backporting gerrit r817231 r817232 for wmf.21, T33780 (duration: 03m 02s)

The fix should be live on Wikimedia wikis. @PerfektesChaos Can you ask the folks who experienced problems if they are resolved now?

I just tried out the fix in WikiEditor on en.wikipedia.org using the latest Chrome on Linux. The behavior is better than before, but still strange. Here are steps to reproduce:

  1. Edit a brand new article.
  2. Type one two three
  3. Insert a link using the Link Editor. I linked to the article wombat which inserted the string [[wombat]] into the editor.
  4. Type four five six. The editor now contains the text: one two three[[wombat]]four five six. Everything looks good so far.
  5. Press ctrl-Z to undo. The string four five six disappears. This is correct behavior.
  6. Press ctrl-Z again. The string [[wombat]] disappears. This is correct behavior.
  7. Press ctrl-Z again. Nothing happens. This is incorrect behavior. I expected one two three to disappear, in whole or in part.
  8. Press ctrl-Z four more times. Nothing happens. This is also incorrect behavior.
  9. Press ctrl-Z again. The string one two three disappears!

So, to my layperson's eye, it looks like 5 empty edits are being appended to the Undo buffer when you use the Link Editor.

@maiden that seems like a bug in Chrome. Or maybe intended behaviour for some reason... In any case browser is controlling the undo buffer. You can only do so much on JS side... AFAIK cke for example creates its own undo buffer, but they operate on contenteditable so that's a very different story.

Any way if undo works at all then inserting text worked. You can file a request to Chromium to make this work differently. It's been at least 5 years since undo buffer was broken and in this years I haven't seen any standard on insert and undo/redo emerging. Some think undo should be global (in all form fields) others think it should only be about currently focused field... Don't seem like there is a hero that can resolve this 😉

Thanks @Nux. I tried the same operations in Firefox on Linux and they work fine. So it does seem like a Chrome issue.

I just tried it in Chrome on Windows and the problem is present.

I pinpointed the issue with the template dialog. It's only when you add a parameter. In that case the string is replaced/inserted in the textfield for specifying the parameter instead.

This is because of execInsertText used by replaceSelection, which does a field.focus() (and the focus within the encapsulateSelection). The OOUI window however has a focus trap and thus you cannot focus the textarea as it is not inside the dialog, instead the focus will return to the parameter's textfield. I suspect we can counter this by delaying the encapsulateSelection which the templatedialog executes to after the closing of the dialog.

Change 817396 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/TemplateWizard@master] Delay template insertion until after closing the dialog

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

Change 817397 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Thanks @TheDJ, that makes sense. Either of the patches above will fix that issue, can you review? (The TemplateWizard patch will make undo work as well, the mediawiki/core will make it use the fallback mode with broken undo, but at least it'll insert. That's intended for any other code that could have the same bug.)

I couldn't reproduce it previously because I was inserting the template without adding any parameters, so I didn't put the focus into the dialog, and execCommand still affected the edit field.

Change 817397 merged by jenkins-bot:

[mediawiki/core@master] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Change 817847 had a related patch set uploaded (by Bartosz Dziewoński; author: Nux):

[mediawiki/core@wmf/1.39.0-wmf.22] jquery.textSelection: Support more edge cases of document.execCommand

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

Change 817848 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.39.0-wmf.21] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Change 817849 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.39.0-wmf.22] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Change 817850 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/TemplateWizard@wmf/1.39.0-wmf.21] Delay template insertion until after closing the dialog

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

Change 817851 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/TemplateWizard@wmf/1.39.0-wmf.22] Delay template insertion until after closing the dialog

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

Change 817847 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.22] jquery.textSelection: Support more edge cases of document.execCommand

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

Change 817848 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.21] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Change 817849 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.22] jquery.textSelection: Use non-execCommand when we can't focus the field

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

Change 817850 merged by jenkins-bot:

[mediawiki/extensions/TemplateWizard@wmf/1.39.0-wmf.21] Delay template insertion until after closing the dialog

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

Change 817851 merged by jenkins-bot:

[mediawiki/extensions/TemplateWizard@wmf/1.39.0-wmf.22] Delay template insertion until after closing the dialog

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

Mentioned in SAL (#wikimedia-operations) [2022-07-27T21:17:41Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.22/resources/src/jquery/jquery.textSelection.js: Backport: [[gerrit:817847|jquery.textSelection: Support more edge cases of document.execCommand (T33780)]] (duration: 03m 10s)

Mentioned in SAL (#wikimedia-operations) [2022-07-27T21:21:12Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.21/resources/src/jquery/jquery.textSelection.js: Backport: [[gerrit:817848|jquery.textSelection: Use non-execCommand when we can't focus the field (T33780)]] (duration: 03m 09s)

Mentioned in SAL (#wikimedia-operations) [2022-07-27T21:25:02Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.22/resources/src/jquery/jquery.textSelection.js: Backport: [[gerrit:817849|jquery.textSelection: Use non-execCommand when we can't focus the field (T33780)]] (duration: 03m 22s)

Mentioned in SAL (#wikimedia-operations) [2022-07-27T21:28:56Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.21/extensions/TemplateWizard/resources/ext.TemplateWizard.Dialog.js: Backport: [[gerrit:817850|Delay template insertion until after closing the dialog (T33780)]] (duration: 03m 27s)

Mentioned in SAL (#wikimedia-operations) [2022-07-27T21:32:48Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.22/extensions/TemplateWizard/resources/ext.TemplateWizard.Dialog.js: Backport: [[gerrit:817851|Delay template insertion until after closing the dialog (T33780)]] (duration: 03m 36s)

The second round of fixes should now be live on Wikimedia wikis.

I'm still waiting for a response from @PerfektesChaos. If I hear nothing, I will optimistically assume that this fixed the problem.

Change 817396 merged by jenkins-bot:

[mediawiki/extensions/TemplateWizard@master] Delay template insertion until after closing the dialog

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

TheDJ claimed this task.