Editor: wptextpattern: space-triggered patterns are unreliable

[jnylen0]

In wptextpattern, the ​keyboard shortcuts for adding lists are somewhat unreliable.

For example, typing * or - starts an unordered list. This reformatting is performed upon receiving the keyup event of the space key (​ref), and it is only performed if the current cursor position matches the length of the * patttern (​ref).

This means that the following sequence of key events does not result in conversion of the current line to a list:

• (Start on a blank line)
• Press and release -
• Press the spacebar but do not release it
• Press a
• Release the spacebar
• Release a

The attached patch removes the check for the cursor position. I'm not sure why this check is there, and I couldn't find any breakage from removing it. Also, I'm not sure why we need to call setCursorLocation, but this doesn't work well in the situation I've described above (it puts the cursor before the additional a character).

cc @iseulde @azaozz

[iseulde]

Thanks for the bug report and patch! This will need additional unit tests, and the patch also causes an existing one to fail. We look at the cursor position as we don't want it to convert later on when the user cancelled the conversion.

See ​https://make.wordpress.org/core/handbook/testing/automated-testing/qunit/.
You can also run them with Grunt.

[iseulde]

Is this something you run into when typing fast? Or in what scenario do you press two keys at once?

[jnylen0]

Yes, according to my testing (using ​this page for example) overlapping keypresses are quite common during normal/fast typing:

keydown  keyCode=81  (Q)   which=81  (Q)   charCode=0        
keypress keyCode=113 (q)   which=113 (q)   charCode=113 (q)  
keydown  keyCode=85  (U)   which=85  (U)   charCode=0        
keypress keyCode=117 (u)   which=117 (u)   charCode=117 (u)  
keydown  keyCode=73  (I)   which=73  (I)   charCode=0        
keypress keyCode=105 (i)   which=105 (i)   charCode=105 (i)  
keyup    keyCode=81  (Q)   which=81  (Q)   charCode=0        
keyup    keyCode=85  (U)   which=85  (U)   charCode=0        
keyup    keyCode=73  (I)   which=73  (I)   charCode=0        
keydown  keyCode=67  (C)   which=67  (C)   charCode=0        
keypress keyCode=99  (c)   which=99  (c)   charCode=99  (c)  
keydown  keyCode=75  (K)   which=75  (K)   charCode=0        
keypress keyCode=107 (k)   which=107 (k)   charCode=107 (k)  
keyup    keyCode=75  (K)   which=75  (K)   charCode=0        
keyup    keyCode=67  (C)   which=67  (C)   charCode=0        
keydown  keyCode=76  (L)   which=76  (L)   charCode=0        
keypress keyCode=108 (l)   which=108 (l)   charCode=108 (l)  
keydown  keyCode=89  (Y)   which=89  (Y)   charCode=0        
keypress keyCode=121 (y)   which=121 (y)   charCode=121 (y)  
keyup    keyCode=76  (L)   which=76  (L)   charCode=0        
keyup    keyCode=89  (Y)   which=89  (Y)   charCode=0        

Thanks for the additional explanation. Unfortunately that means we'll need a smarter approach. I'll work on an updated patch including tests.

[jnylen0]

Existing tests pass, new tests added.

[jnylen0]

Uploading another patch per discussion in Slack:

• Inline patterns (so far, code tags) are also affected
• Previous patch did not account for backslash characters (keyCode 220) and probably others
• Suggested approach: "So I think it might be better to 'normalise' the events so the code processes them in order"

This is almost a really nice solution, except that Firefox doesn't support the selectionchange event which I'm using to detect when the editor content has changed as a result of the keypress we're interested in. I've hacked around this by adding a setTimeout, which seems to work, but breaks the tests because some of the logic is now asynchronous.

Is there a better way to detect when the editor content has finished changing as a result of a keypress?

If not, we will need to either refactor the failing tests to use setTimeout in the right places, or try another approach.

[jnylen0]

Issues with some tests in Firefox (though the actual functionality seems fine)

[iseulde]

@jnylen0 Looks good! I've updated the patch to use only a timeout, and the tests for backspace to have a double timeout to account for the conversion and the canUndo flag. Could you test?

[jnylen0]

Nice and simple! Works well in my testing.

I think the double setTimeout thing in the tests needs an explanatory comment (and an extra level of indentation).

[iseulde]

In 37668:

TinyMCE: wptextpattern: fix for fast typing

Props jnylen0, iseulde.

Fixes #36585.