WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#37693 closed defect (bug) (fixed)

TinyMCE: backtics not handled correctly in paragraphs that already contain backticks

Reported by: borekb Owned by: iseulde
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.3
Component: TinyMCE Keywords: has-patch
Focuses: javascript Cc:
PR Number:

Description

Steps to reproduce:

  1. Have a paragraph that contains literal backticks. For example, copy this to TinyMCE: In this paragraph with `backticks`, try to add some more.
  2. Anywhere in this paragraph, enter `test` and press a spacebar.

It should convert it to <code>test</code> but it doesn't.

My guess is that a regex or something similar doesn't handle multiple backtics in a paragraph correctly.

Attachments (13)

37693.patch (2.3 KB) - added by iseulde 3 years ago.
37693.2.patch (5.8 KB) - added by iseulde 3 years ago.
37693.3.patch (1.0 KB) - added by iseulde 3 years ago.
37693.4.patch (1.0 KB) - added by iseulde 3 years ago.
37693.5.patch (2.0 KB) - added by iseulde 3 years ago.
37693.6.patch (2.1 KB) - added by azaozz 3 years ago.
37693.7.patch (2.1 KB) - added by azaozz 3 years ago.
37693.8.patch (4.3 KB) - added by iseulde 3 years ago.
37693.9.patch (4.0 KB) - added by iseulde 3 years ago.
37693.10.patch (3.7 KB) - added by azaozz 3 years ago.
37693.11.patch (5.0 KB) - added by iseulde 3 years ago.
37693.12.patch (7.7 KB) - added by azaozz 3 years ago.
37693.13.patch (8.2 KB) - added by iseulde 3 years ago.

Download all attachments as: .zip

Change History (38)

This ticket was mentioned in Slack in #core-tinymce by borekb. View the logs.


3 years ago

#2 @swissspidy
3 years ago

  • Version changed from 4.6 to 4.3

#3 @iseulde
3 years ago

  • Milestone changed from Awaiting Review to 4.7

Confirmed.

#4 @iseulde
3 years ago

  • Keywords needs-patch added

Related: #36459.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


3 years ago

#8 @jbpaul17
3 years ago

  • Owner set to iseulde
  • Status changed from new to assigned

@iseulde
3 years ago

@iseulde
3 years ago

#9 @iseulde
3 years ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed
  • Status changed from assigned to reviewing

This last patch should work well. @borekb, could you please test the patch, if you can? @azaozz, could you please test and review?

A test case has been added, and because of the scope of the change, I added a test case for patterns with two characters. There was once a test for that, but was removed after removing the bold pattern. We should still test it though as it should work.

#10 @iseulde
3 years ago

#36459 was marked as a duplicate.

#11 @iseulde
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 39075:

TinyMCE: wptextpattern: Handle unconverted inline patterns

Make sure the right text is matched when it already contains characters of the pattern.
Adds two more unit tests.

Fixes #37693.

@iseulde
3 years ago

@iseulde
3 years ago

@iseulde
3 years ago

@azaozz
3 years ago

#12 @azaozz
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Yeah, that broke multi-character patterns when they use the same characters as single-character pattern, like ** and *. I know we don't use any of there at the moment, but may in the future, or a plugin can add them.

In 37693.6.patch attempting to fix this. Expects that the patterns are sorted by priority: longer patterns first. This also changes the behaviour to support the pattern characters to appear in the string. Example:

  • __ and _ are registered;
  • Typing __ $var_name __ should properly replace the starting and ending __ and leave the _ in the middle.

This still needs testing and is a change in behaviour. Few tests may need updating too (fail at the moment).

@azaozz
3 years ago

#13 @azaozz
3 years ago

  • Keywords needs-unit-tests added

In 37693.7.patch: a little optimization.

#14 @iseulde
3 years ago

  • Status changed from reopened to reviewing

This ticket was mentioned in Slack in #core by helen. View the logs.


3 years ago

@iseulde
3 years ago

@iseulde
3 years ago

@azaozz
3 years ago

#16 follow-up: @azaozz
3 years ago

37693.9.patch looks good. 37693.10.patch adds some cleanup and changes it to test for a white space or start of line before the pattern, instead of looking for the first char of the pattern (as we discussed in #core-editor). So if the pattern is *, test* test* will not trigger it, and test *test* will.

#17 @azaozz
3 years ago

  • Keywords needs-unit-tests removed

#18 in reply to: ↑ 16 @iseulde
3 years ago

Replying to azaozz:

37693.9.patch looks good. 37693.10.patch adds some cleanup and changes it to test for a white space or start of line before the pattern, instead of looking for the first char of the pattern (as we discussed in #core-editor). So if the pattern is *, test* test* will not trigger it, and test *test* will.

That looks good. The only case I have where I would expect it to trigger is something like some*thing*. I agree that we shouldn't allow it when there is whitespace after the starting pattern though. So maybe some* thing* should not trigger it, but some *thing* and some*thing* should.

@iseulde
3 years ago

#19 @iseulde
3 years ago

Updated one rule and added 3 unit tests.

#20 @iseulde
3 years ago

But the last patch does again not allow this:

Typing __ $var_name __ should properly replace the starting and ending __ and leave the _ in the middle.

It seems we need to choose. Either we allow test*test* or we allow the above. We can't do both I think.

#21 @iseulde
3 years ago

Also, let's keep in mind that we added this to allow simple shortcuts, not to fully support markdown. I think bold and italic inside words will be more useful than allowing delimiter characters inside the text. It cannot be solved anyway for something like _$var_name_, only __$var_name__, so the rule is a bit weird in my opinion. Additionally I think * delimiters are bit better for this, and if _ is used, the conversion can still be undone easily.

#22 @iseulde
3 years ago

Example text: "It's WordPress, not Wordpress."

It can be used to emphasise mistakes, emphasise a syllable or a free morpheme in a compound.

This ticket was mentioned in Slack in #core by iseulde. View the logs.


3 years ago

@azaozz
3 years ago

#24 @azaozz
3 years ago

In 37693.12.patch: 7693.11.patch plus small fix for multi-character delimiters, added some inline docs and few more tests.

This looks and works very well now, thinking its ready :)

@iseulde
3 years ago

#25 @iseulde
3 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 39150:

TinyMCE: wptextpattern: Fix inline patterns.

  • Allow spaces inside inline pattern text, unless the delimiter stands alone.
  • Add more unit tests.
  • Add more inline docs.

Part props azaozz.
Fixes #37693.

Note: See TracTickets for help on using tickets.