Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/44: Show mention after softBreak and some punctuation characters #61

Merged
merged 24 commits into from
May 13, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Apr 26, 2019

Suggested merge commit message (convention)

Fix: Show the Mention UI after soft break and opening punctuation characters. Closes ckeditor/ckeditor5#4648.


Additional information

  • It is a draft PR since I'm not sure how to test missing ES2018 feature. I've created a feature detection solution (try-catch block) so there's no easy way to make 100% on this path. I might:
  • override RegExp construcator and throw for this test (Chosen)
  • ignore path for covarage

What I'm missing is some automated way to showcase that this path should be invalid in the feature when FF implement this. (Probanly a follow-up). (ckeditor/ckeditor5#1749)

@mlewand
Copy link
Contributor

mlewand commented May 6, 2019

What I'm missing is some automated way to showacase that this path should be invalid in the feature when FF implemnet this.

@jodator I like the idea of tests that automatically tells whether our workaround is still needed or not.

There are couple of very simple ideas how such detection could be added. For instance we could simply add unit test for a problematic browser.

A simple way to test that would be a test like that:

describe( 'Upstream issues', () => {
	if ( env.isGecko ) {
		it( 'Doesn\' support unicode categories in RegExp', () => {
			// https://bugzilla.mozilla.org/show_bug.cgi?id=1361876
			// https://github.com/ckeditor/ckeditor5-mention/issues/44#issuecomment-487002174
			expect( () => {
				return 'Ą'.match( new RegExp( '\\p{L}', 'u' ) );
			}, 'Hoooray, Firefox shipped unicode regexp groups' ).to.throw( SyntaxError, /^invalid identity escape in regular expression/ );
		} );
	}
} );

Couple of notes:

  • URL reference to upstream issues are crucial.
  • URL to our internal comment explaining why the workaround has been added and what to do when vendor fixes this issue are also crucial (it would be nice if it was even cleaner/easier message than the one I linked here).
  • This introduces browser sniffing, but since it's in tests and used only to detect known browser issues we're good with that.

Although I don't think we should add them to our standard test suite, as imagine that in one year form here we have like 300 of such workarounds, and each week or so some browser get fixed. So it would ruin our CI experience. So I'm thinking like adding a --upstream cli flag that would be recognized by Karma and passed to the runtime. That way we could add tests like:

describe( 'Upstream issues', function() {
	if ( !myGlobalOptions.upstream ) {
		this.skip();
	}

	if ( env.isGecko ) {
		it( 'Doesn\' support unicode categories in RegExp', () => {
			// (...)
		} );
	}
} );

Then we could have a CRON that would run daily master branch with --upstream flag.

@jodator
Copy link
Contributor Author

jodator commented May 6, 2019

@mlewand I've added the tests for try-catch problem on Chrome (thus 100% CC without istanbul ignore). I would be happy to add the upstream issues as this will be two liner for those.

@jodator
Copy link
Contributor Author

jodator commented May 6, 2019

But it could be run always if you ask me - so without the --upstream flag.

@jodator jodator marked this pull request as ready for review May 7, 2019 09:09
@jodator
Copy link
Contributor Author

jodator commented May 7, 2019

OMG - there's some errors on Edge... checking.

@jodator jodator requested a review from mlewand May 7, 2019 09:46
@mlewand mlewand self-assigned this May 8, 2019
# Conflicts:
#	src/mentionui.js
@mlewand
Copy link
Contributor

mlewand commented May 9, 2019

ckeditor/ckeditor5#1749 - issue requesting for a generic approach to handle upstream issues.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had more fun with it.

  • Let's also use feature detection for MS Edge.
  • It would be nice also to include \p{Pi} group.
  • Couple of minor typos.

While reviewing I have created t/44b branch where I applied most of the changes. Feel free to merge/cherry pick them.

Once that's addressed we're good with this PR 👍

import { toWidget, viewToModelPositionOutsideModelElement } from '@ckeditor/ckeditor5-widget/src/utils';
import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';

class InlineWidget extends Plugin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for adding a placeholder-like widget in this manual test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I wanted to test how this behaves with not only a <softBreak> but also with inline widget. (potential errors, etc).

src/mentionui.js Outdated Show resolved Hide resolved
src/mentionui.js Outdated Show resolved Hide resolved
src/mentionui.js Outdated Show resolved Hide resolved
src/mentionui.js Outdated Show resolved Hide resolved
src/mentionui.js Outdated Show resolved Hide resolved
src/textwatcher.js Outdated Show resolved Hide resolved
tests/mentionui.js Outdated Show resolved Hide resolved
jodator and others added 3 commits May 10, 2019 16:34
Co-Authored-By: Marek Lewandowski <[email protected]>
Co-Authored-By: Marek Lewandowski <[email protected]>
Co-Authored-By: Marek Lewandowski <[email protected]>
@jodator jodator requested a review from mlewand May 10, 2019 15:09
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@mlewand mlewand merged commit 86262d1 into master May 13, 2019
@mlewand mlewand deleted the t/44 branch May 13, 2019 09:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot type mention inside brackets, quotes and after a soft break
2 participants