WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#36585 closed defect (bug) (fixed)

Editor: wptextpattern: space-triggered patterns are unreliable

Reported by: jnylen0 Owned by: iseulde
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.3
Component: TinyMCE Keywords: has-patch has-unit-tests
Focuses: javascript Cc:

Description

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

Attachments (5)

36585.diff (994 bytes) - added by jnylen0 5 years ago.
36585.2.diff (5.6 KB) - added by jnylen0 4 years ago.
Existing tests pass, new tests added.
36585.3.diff (6.5 KB) - added by jnylen0 4 years ago.
Issues with some tests in Firefox (though the actual functionality seems fine)
36585.patch (1.5 KB) - added by iseulde 4 years ago.
36585.2.patch (2.3 KB) - added by iseulde 4 years ago.

Download all attachments as: .zip

Change History (20)

@jnylen0
5 years ago

#1 @swissspidy
5 years ago

  • Keywords has-patch added

#2 @iseulde
5 years ago

  • Keywords needs-unit-tests added

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.

#3 @iseulde
5 years ago

  • Milestone changed from Awaiting Review to Future Release

#4 @DrewAPicture
5 years ago

  • Version changed from trunk to 4.3

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


5 years ago

#6 @iseulde
5 years ago

  • Keywords reporter-feedback added

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

#7 @jnylen0
4 years ago

  • Keywords reporter-feedback removed

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
4 years ago

Existing tests pass, new tests added.

#8 @jnylen0
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This ticket was mentioned in Slack in #core-editor by jnylen. View the logs.


4 years ago

#10 @jnylen0
4 years ago

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.

Last edited 4 years ago by jnylen0 (previous) (diff)

@jnylen0
4 years ago

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

This ticket was mentioned in Slack in #core-editor by jnylen. View the logs.


4 years ago

@iseulde
4 years ago

#12 @iseulde
4 years ago

@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?

Last edited 4 years ago by iseulde (previous) (diff)

@iseulde
4 years ago

#13 @iseulde
4 years ago

  • Milestone changed from Future Release to 4.6

#14 @jnylen0
4 years ago

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).

#15 @iseulde
4 years ago

  • Owner set to iseulde
  • Resolution set to fixed
  • Status changed from new to closed

In 37668:

TinyMCE: wptextpattern: fix for fast typing

Props jnylen0, iseulde.

Fixes #36585.

Note: See TracTickets for help on using tickets.