Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot type mention inside brackets, quotes and after a soft break #4648

Closed
pomek opened this issue Apr 5, 2019 · 5 comments · Fixed by ckeditor/ckeditor5-mention#61
Closed
Assignees
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@pomek
Copy link
Member

pomek commented Apr 5, 2019

Try to open mention's feed after typing (@. At this moment it does not work.

See https://github.com/ckeditor/ckeditor5-mention/issues/44#issuecomment-491752334 for fix summary.

@oleq
Copy link
Member

oleq commented Apr 5, 2019

The feature has been implemented so far to prevent triggering in the middle of the word ([email protected]).

IMO we need to whitelist certain structures, like for instance

\s[
    \[\("'
]@

to allow typing (@foo), [@foo], "@foo", '@foo' etc..

@jodator
Copy link
Contributor

jodator commented Apr 5, 2019

Also <softBreak>foo might not work now.

@oleq oleq changed the title Cannot type mention inside brackets ( Cannot type mention inside brackets, quotes, and after a soft break Apr 8, 2019
@jodator
Copy link
Contributor

jodator commented Apr 25, 2019

I've implemented this in a simple/naive way - just by adding some arbitrary characters to the regex:

const IGNORED_CHARACTERS = ' ([{\'"';

The implementation (hence const name) is secondary to my comment here so keep this in mind.

There are couple of issues here:

  1. Typing after <softBreak> is not working - 🐛
  2. Typing after an inline element (defined in schema as isInline = true) - right now I see that you might require a space after an inline widget and I'd left this as-is
  3. Typing inside quotes/parenthesis - kinda feature to me not a 🐛 - so I'd like to open a discussion for this.

So ATM I've used reported characters to enable this feature and as stated above it is very naive approach.

Note: The Mention feature only interprets text before caret so only opening parenthesis/quotes are taken under consideration.

The full-blown approach should allow every opening punctuation. The hand-crafted regex for this will be ugly since JavaScript RegExp engine does not support Unicode categories:

const regex = new Regex( '\\(\\[\\{\u0F3A\u0F3C\u169B\u201A\u201E\u2045\u207D\u208D\u2308\u230A\u2329\u2768\u276A\u276C\u276E\u2770\u2772\u2774\u27C5\u27E6\u27E8\u27EA\u27EC\u27EE\u2983\u2985\u2987\u2989\u298B\u298D\u298F\u2991\u2993\u2995\u2997\u29D8\u29DA\u29FC\u2E22\u2E24\u2E26\u2E28\u2E42\u3008\u300A\u300C\u300E\u3010\u3014\u3016\u3018\u301A\u301D\uFD3F\uFE17\uFE35\uFE37\uFE39\uFE3B\uFE3D\uFE3F\uFE41\uFE43\uFE47\uFE59\uFE5B\uFE5D\uFF08\uFF3B\uFF5B\uFF5F\uFF62' );

while other languages we have access to unicode group switches:

preg_match( '/\p{Ps}/', '[', $out );

@mlewand / @Reinmar:
So the question here is: Do we want to support them all or only some arbitrary subset of this group?

ps.: I've alredy talked with @oleq and we came to a conclusion that sooner or later we will need an util for creating unicode regexes.

Also I've created a task for general util: https://github.com/ckeditor/ckeditor5-utils/issues/283.

@mlewand
Copy link
Contributor

mlewand commented Apr 26, 2019

I just checked the state of things.

Support for the unicode category escape character has been introduced in es2018 and it is already supported by Chrome and Safari. Ticket for support in FireFox is pending and is blocked on irregexp bump issue in FF but it should come sooner than later.

It is possible very easily to feature detect it:

  • Edge throws Invalid regular expression: invalid escape in unicode pattern when constructing RegExp( /\p{Letter}/u ).
  • Firefox throws SyntaxError: invalid identity escape in regular expression when constructing RegExp( /\p{Letter}/u ).

Given that it is already specified and it is a matter of time when it gets to FF (and Edge is migrating to Blink) I would implement it based on a feature detection. If unicode category escape is available, use it to provide best experience and full utf support (same thing might apply to #4642). In case it is not supported by the client, we fall back to naive behavior, in this case simplified handcrafted regexp.

I don't see a need to intrudce an extra dependency for that (xRegExp).

Regarding ckeditor/ckeditor5-utils#283 let's implement this handling first in the mentions alone, and then as ckeditor/ckeditor5-utils#283 is closed we'll use the new API here as I'm afraid it might not get solved as quickly as this task.

@mlewand mlewand changed the title Cannot type mention inside brackets, quotes, and after a soft break Cannot type mention inside brackets, quotes and after a soft break May 10, 2019
mlewand referenced this issue in ckeditor/ckeditor5-mention May 13, 2019
@mlewand
Copy link
Contributor

mlewand commented May 13, 2019

To sum up, the fix uses two strategies:

  • generic for browsers that are lacking regexp group support - you can a mention after list of predefined characters like (, [, ", '.
  • full for modern browsers that support regexp groups (today Chrome only) and in addition to above it supports fancy utf quotes like etc.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-mention Oct 9, 2019
@mlewand mlewand added this to the iteration 24 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:mention labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants