Inline toolbar context feature #1195

mlewand · 2017-11-21T13:41:34Z

What is the purpose of this pull request?

New feature

This PR contains

Unit tests
Manual tests

What changes did you make?

Added context classes for inline toolbar. It makes stupid easy to maintain multiple inline toolbars in a single editor.

See CKEDITOR.editor.inlineToolbar (local docs link) for more details.

f1ames

Overall nice and solid piece of code 👍 Just some minor stuff here and there.

Also one test failing on Safari:

f1ames · 2017-11-21T15:57:51Z

plugins/inlinetoolbar/plugin.js

@@ -37,6 +39,10 @@
 	/**
 	 * Class representing instance of inline toolbar.
 	 *
+	 * The easiest way to create an inline toolbar is by using {@link CKEDITOR.editor.inlineToolbar#create} function.
+	 *
+	 * However it's possible to maintain it by manually, like below:


to maintain it by manually...

f1ames · 2017-11-21T15:58:58Z

plugins/inlinetoolbar/plugin.js

+		},
+
+		/**
+		 * @param {CKEDITOR.dom.element} [pointedElement] Element that should be pointed by the inline toolbar.


Missing method description.

f1ames · 2017-11-21T16:12:10Z

plugins/inlinetoolbar/plugin.js

+			this.toolbar.show();
+		},
+
+		hide: function() {


Same here, missing method description.

f1ames · 2017-11-21T16:14:41Z

plugins/inlinetoolbar/plugin.js

+
+	Context.prototype = {
+		/**
+		 * Destroy inline toolbar context


I have a doubt about this description, what this method does is basically destroying the toolbar which is bound to this context (same with show and hide), so maybe it can be rephrased to emphasize this fact?

I agree, corrected the description so that it clearly says that it hides toolbar controlled by the context.

f1ames · 2017-11-21T16:30:17Z

plugins/inlinetoolbar/plugin.js

+		 * Checks if any of `options.widgets` widgets is currently focused.
+		 *
+		 * @private
+		 * @returns {Boolean/CKEDITOR.dom.element} `true` if currently selected widget matches any tracked by this


From what I see, this function may return false, undefined or CKEDITOR.dom.element. So true is never returned. Maybe the return type should be {CKEDITOR.dom.element/null} (it is more natural when the function returns an instance it may also return null which means there is no instance). Mixing Boolean with CKEDITOR.dom.element looks a little strange (same for _matchElement below).

Well, in that particular case describing true does not make sense. As you pointed out it's not returned. So it make sense to rewrite it.

However in general _matchWidget has return API compatible with other matching functions.

Meaning that if it returns:

true - manager will determine what element should be pointed by the toolbar

false - element is simply not matched

CKEDITOR.dom.element - entity is positively matched, and function explicitly says what element should be pointed by the toolbar

Having that said I'd stick to current return API.

It makes sense to keep things consistent this way.

However, one thing I see is that _matchWidget/_matchElement and _matchRefresh are a little different. All three are internal functions. But the first two reflects/works on simple properties (string or array) passed in contextDefiniton object, while the _matchRefresh reflects refresh function from contextDefinition thus also reflecting its public API.
And as I said before mixing Boolean and CKEDITOR.dom.element as return values seems a little unintuitive to me.
One more thing is that this API is compatible only by definition (well, maybe that's what compatibility means:D), because in real use cases _matchRefresh usually will return true when element matches and the other two will always return CKEDITOR.dom.element.

Anyway, I'm not convinced that one solution is far better than the other so we may leave it as is.

Ok this discussion was moved to #1195 (comment) and #1195 (comment)

f1ames · 2017-11-22T10:43:24Z

tests/plugins/inlinetoolbar/context/integration.js

+				return el.getName() === 'strong';
+			} );
+		},
+


Shouldn't we test a case when more than one matching option is defined to check how it behaves (and to prevent situation that after some changes/refactoring it will start behaving differently)?
Or it is just treated as misuse and we simply don't handle it?

Not sure if I got it right, but if you're asking about multiple combinations of options.widget/cssSelector/refresh, then they are already tested in priorities test suite.

On top of that integration tests adds up some extra protection against that, so I'd say it's pretty well covered.

I meant the case when you have more than one match option defined for the same context like:

{ buttons: 'Bold,Italic', refresh: sinon.stub().returns( true ), widgets: [ 'foo', 'bar' ] }

f1ames · 2017-11-22T10:48:40Z

tests/plugins/inlinetoolbar/context/manual/complex.html

+</style>
+
+<textarea id="editor1" cols="10" rows="10">
+	<p>Sample paragraph with <em>emphasize</em>, <strong>strong</strong>, <u>underline</u> and <s>strike thourgh</s>.</p>


Typo: strike through

Looks like it was not updated 🤔

f1ames · 2017-11-22T10:49:42Z

tests/plugins/inlinetoolbar/context/manual/complex.html

+
+</style>
+
+<textarea id="editor1" cols="10" rows="10">


Have you consider addink links/images to initial content so test can be performed easier/faster (without a need to add those elements)?

Make sense, I have added a link to the set.

Looks like it was not updated 🤔

f1ames · 2017-11-22T10:57:04Z

tests/plugins/inlinetoolbar/context/priorities.js

+			} );
+		},
+
+		// This case is yet to be decided.


AFAIR options.widgets has priority over options.cssSelector so this test could be added (uncommented) here.

Actually I had to remove it as this test didn't have sense anymore.

It assumed that options.widget will take effect if the selection is within nested editable, say a strong (and this case is covered with unit test in widget.js).

However after research we have implemented it in a way that options.widget takes effect only when the widget itself is focused.

Looks like it was not updated 🤔

f1ames · 2017-11-22T11:08:49Z

tests/plugins/inlinetoolbar/context/refresh.js

+		 * manager, to recheck all the contexts.
+		 * @returns {CKEDITOR.plugins.inlinetoolbar.context}
+		 */
+		_getContextStub: function( refreshCallback, autoRefresh ) {


This function is also duplicated in cssselector.js test file.

To be honest, each function like that is slightly different, and after making a common interface it would not be as usable for each suite.

Thus I don't see too much value in doing that. WDYT?

Assert was repeated the same way everywhere, and it made a perfect sense, however I find it a slightly different case.

I see, you would have to make generic function which can take refresh, widgets and cssSelector and create specific context based on that. As it will overcomplicate thinks, lets leave it as is 👍

mlewand · 2017-11-22T14:16:57Z

Fixed failing test on Safari. Turned out there was a typo in test case, causing wrong selection in that browser.

f1ames

It seems you didn't push all changes you have made (assuming based on comments)?

Please also check #1195 (comment) and #1195 (comment) and #1195 (comment).

f1ames · 2017-11-22T18:06:40Z

tests/plugins/inlinetoolbar/context/cssselector.js

+			this.editorBot.setHtmlWithSelection( '<p><strong data-foo-bar="1">foo^ bar</strong></p>' );
+
+			this._assertToolbarVisible( false, context );
+		},


I'm not seeing those test, maybe you just didn't push them?

f1ames · 2017-11-22T18:14:47Z

tests/plugins/inlinetoolbar/context/cssselector.js

+		},
+
+		/*
+		 * Returns a Context instance with toolbar show/hide methods stubbed.


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:14:58Z

tests/plugins/inlinetoolbar/context/cssselector.js

+
+		/*
+			* @param {Boolean} expected What's the expected visibility? If `true` toolbar must be visible.
+			*/


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:15:09Z

tests/plugins/inlinetoolbar/context/integration.js

+
+			this.editorBot.setHtmlWithSelection( '<p>Test^ <strong>Foo</strong><em>bar</em></p>' );
+
+			emElem = this.editor.editable().findOne( 'em' );


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:16:05Z

tests/plugins/inlinetoolbar/context/integration.js

+			// However later on a cssSelector listener, will hint element B.
+			// Make sure B is pointed by the toolbar.
+
+			this.editorBot.setHtmlWithSelection( '<p><em>foo<strong>b^ar</strong>baz</em</p>' );


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:16:22Z

tests/plugins/inlinetoolbar/context/integration.js

+		/*
+		 * @param {Boolean} expected What's the expected visibility? If `true` toolbar must be visible.
+		 */
+		_assertToolbarVisible: function( expected, context, msg ) {


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:16:35Z

tests/plugins/inlinetoolbar/context/manual/complex.html

+
+</style>
+
+<textarea id="editor1" cols="10" rows="10">


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:16:40Z

tests/plugins/inlinetoolbar/context/manual/complex.html

+</style>
+
+<textarea id="editor1" cols="10" rows="10">
+	<p>Sample paragraph with <em>emphasize</em>, <strong>strong</strong>, <u>underline</u> and <s>strike thourgh</s>.</p>


Looks like it was not updated 🤔

f1ames · 2017-11-22T18:17:00Z

tests/plugins/inlinetoolbar/context/priorities.js

+			} );
+		},
+
+		// This case is yet to be decided.


Looks like it was not updated 🤔

f1ames

Looks good 👍 I just found some places where code could be further simplified after _matchRefresh refactoring.

I also don't see links added to manual/complex test as stated in this comment - https://github.com/ckeditor/ckeditor-dev/pull/1195/files#r152571754.

And I'm not sure if you have seen this comment #1195 (comment)?

f1ames · 2017-11-23T10:51:33Z

plugins/inlinetoolbar/plugin.js

+				return this.editor.widgets.focused.element;
+			} else {
+				return null;
+			}


Could be simplified to:

var ret = null; if ( this.options.widgets ) { var widgetNames = this.options.widgets, curWidgetName = this.editor.widgets && this.editor.widgets.focused && this.editor.widgets.focused.name; if ( typeof widgetNames === 'string' ) { widgetNames = widgetNames.split( ',' ); } if ( CKEDITOR.tools.array.indexOf( widgetNames, curWidgetName ) !== -1 ) { ret = this.editor.widgets.focused.element; } } return ret;

well, the number of lines is the same. I find it a little more readable, but I suppose it is the matter of preferences. If you feel the same you may change it.

I agree that it might add a bit to the readability. Also I have extracted widgetNames before the first if as it make sense to use widgetNames instead of this.options.widgets there already.

f1ames · 2017-11-23T10:54:18Z

plugins/inlinetoolbar/plugin.js

+			var ret = null;
+
+			if ( this.options.refresh ) {
+				ret = this.options.refresh( this.editor, path, selection );


If options.refresh returns false the whole function will also return false instead of null. I added and pushed one unit test checking return type of this function.

I have adjusted this test case a bit in c1dc8cd, just by using more precise assertions for type checking in .

f1ames · 2017-11-23T11:37:45Z

plugins/inlinetoolbar/plugin.js

+
+							contextMatched = curContext;
+						}
+					}


Now as _refresh* functions API got simpler and returns null or element, couldn't this be simplified to:

if ( !contextMatched || contextMatched.options.priority > curContext.options.priority ) { highlightElement = matchingFunction( curContext, matchingArg1 ); if ( !!highlightElement ) { contextMatched = curContext; } }

?

We can't simplify it that much as it would break test case when subsequent (higher priority) context returns null again. In that case it would cause highlightElement to be reset back to null.

Covered this with a test case in 8e89c0330836e0519bf6ff2f26e5185b13b5beb0.

Nonetheless we could simplify it now a bit, which I did in e382734.

f1ames · 2017-11-23T11:41:46Z

plugins/inlinetoolbar/plugin.js

+				}
+
+				contextMatched.show( highlightElement );
+			}


If matchEachContext above can be simplified, also this part can as contextMatched is set only when highlightElement is present so you can just go with:

if ( contextMatched ) { contextMatched.show( highlightElement ); }

Now due to this part: https://github.com/ckeditor/ckeditor-dev/blob/e382734/plugins/inlinetoolbar/plugin.js#L425-L428

It's guaranteed that highlightElement is an element, so that gives me the confidence to remove this defensive bit 🙂

f1ames

Everything looks good 👍

However I found one issue related to inline editor and drag/drop. When link is selected and toolbar appears and you drag it and then unlink - error is thrown:

Wasn't able to reproduce it on t/933, there panel follows the dropped content 🤔

Tried with managing toolbar like:

editor.on( 'selectionChange', function( evt ) {
	var lastElement = evt.data.path.lastElement;

	if ( lastElement && lastElement.getName() == 'a' ) {
		panel.attach( lastElement );
	} else {
		panel.hide();
	}
} );

Still not sure if it's related to Context API changes.

mlewand · 2017-11-23T15:59:21Z

@f1ames above is actually an upstream issue, looks to me it's due to invalid selection restoration for inline editors.

See #1219 to repro the case.

f1ames

LGTM 👍

…ve features, major fixes.

…olbar namespace.

Misc fixes to correct the API.

…ements path is used.

… elements path in the context.refresh function.

…possible to disable listeners in some unit tests. It was causing a lot of extra toolbar.show / hide calls, which in turn required me to call toolbar.show.reset() in every TC. That was not an elegant solution, so instead I made it easier to disable. What's more, I'm thinking about removing it completely from the constructor, and attach only on first show() method. That would be even more performance-friendly solution. And we'll be able to remove these _attachListeners stubs from init functions.

…oes not explode when loaded in IE8.

Notes: Actually I had to remove it as this test didn't have sense anymore. It assumed that options.widget will take effect if the selection is within nested editable, say a strong (and this case is covered with unit test in widget.js). However after research we have implemented it in a way that options.widget takes effect only when the widget itself is focused.

…ed helper methods.

…ment or null. No booleans anymore.

…ger a bool value. Added an extra TC that was not covered before.

…ill not reset last previously returned (correct) pointed element.

…ossible for it not to be an element. Since it's guaranteed by https://github.com/ckeditor/ckeditor-dev/blob/e382734/plugins/inlinetoolbar/plugin.js#L425-L428 (it's the only place where contextMached is set).

Inline toolbar context feature

mlewand requested a review from f1ames November 21, 2017 13:41

mlewand force-pushed the t/933-2996 branch from b96e8f7 to faf3e02 Compare November 21, 2017 15:49

f1ames suggested changes Nov 22, 2017

View reviewed changes

mlewand requested a review from f1ames November 22, 2017 14:43

f1ames suggested changes Nov 22, 2017

View reviewed changes

f1ames self-requested a review November 23, 2017 10:21

f1ames suggested changes Nov 23, 2017

View reviewed changes

f1ames self-requested a review November 23, 2017 14:40

f1ames suggested changes Nov 23, 2017

View reviewed changes

f1ames self-requested a review November 23, 2017 16:10

f1ames approved these changes Nov 23, 2017

View reviewed changes

wojtekw92 and others added 17 commits November 23, 2017 17:13

Plugin function added.

750bdc3

Some major changes

ac36046

Button creation from name in context

5d87621

Button creation rework

93e383e

handle editor destroy

11a856a

Inline toolbar context first documentation draft, unit tests for acti…

2605295

…ve features, major fixes.

Introduced CKEDITOR.plugins.inilnetoolbar and editor.plugins.inlineto…

a5deb75

…olbar namespace.

Added first tests for context widget matching.

9df83b0

Misc fixes to correct the API.

Temporarily comment out previous tests.

920a4dd

Tests for options.refresh.

e27bf9c

Tests: added tests for context.refresh - making sure that a proper el…

d0f99fe

…ements path is used.

Added a basic implementation for Inline Toolbar's options.refresh and…

32b0411

… elements path in the context.refresh function.

Tests: added unit tests for options.refresh.

1371654

Tests: added integration test for inline toolbar context.

dcda283

Tests: improved and simplified integration test.

dc423e5

Tests: added a dedicated manual test for Inline Toolbar Context.

6e47ea0

mlewand and others added 25 commits November 23, 2017 17:13

Ignored IE8 in tests. Added a logic that will make sure that plugin d…

062162a

…oes not explode when loaded in IE8.

Docs: docs corrections, added missing method descriptions.

4cbc636

Removed unused method.

d8b71e0

Fixed a typo in a method name.

782094c

Referenced element.matches request issue.

34f6f80

Tests: cleanup code style/typos.

db8c5c0

Extracted common _assertToolbarVisible assertion to a helper file.

87c96ee

Tests: fixed inline toolbar context unit test failing on Safari, reus…

a78511b

…ed helper methods.

Tests: reused shared assertion in one more test suite.

4141986

Tests: added links to a manual test.

666d889

Tests: added more cases for CSS matching.

564b942

Docs: corrected API docs for _matchWidget method.

63df01f

Improved docs for inline toolbar context.

a82a4df

Refactoring: _matchElement and _matchWidget can now return either ele…

9922788

…ment or null. No booleans anymore.

Reduced returns amount in _matchElement function.

913a458

Refactoring: _matchRefresh will now return element or null and no lon…

69daf10

…ger a bool value. Added an extra TC that was not covered before.

Unit test checking return type of '_matchRefresh'.

e6fb230

Ensure that _matchRefresh returns null if nothing was not matched.

335db6c

Tests: used more precise assertions for type checking.

9339aab

Refactoring: simplified _matchWidget funciton.

dd14a54

Tests: added extra unit test ensuring that subsequent false matches w…

71f80e5

…ill not reset last previously returned (correct) pointed element.

Refactoring: simplified matching logic in Context Manager.

9b90fb5

Removed an extra fallback logic for highlightElement, as now it's imp…

240df27

…ossible for it not to be an element. Since it's guaranteed by https://github.com/ckeditor/ckeditor-dev/blob/e382734/plugins/inlinetoolbar/plugin.js#L425-L428 (it's the only place where contextMached is set).

Docs rewording.

f00142e

f1ames force-pushed the t/933-2996 branch from 950c957 to f00142e Compare November 23, 2017 16:17

f1ames merged commit 143c2d6 into t/933 Nov 23, 2017

f1ames deleted the t/933-2996 branch November 23, 2017 16:32

mlewand pushed a commit that referenced this pull request Nov 29, 2017

Merge pull request #1195 from ckeditor/t/933-2996

0a19238

Inline toolbar context feature

mlewand mentioned this pull request Dec 4, 2017

Balloon Toolbar #933

Closed


		this.editorBot.setHtmlWithSelection( '<p>Test^ <strong>Foo</strong><em>bar</em></p>' );

		emElem = this.editor.editable().findOne( 'em' );

Inline toolbar context feature #1195

Inline toolbar context feature #1195

Conversation

mlewand commented Nov 21, 2017

What is the purpose of this pull request?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented Nov 22, 2017

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

mlewand commented Nov 23, 2017

f1ames left a comment

Choose a reason for hiding this comment

f1ames Nov 22, 2017 •

edited

Loading

mlewand Nov 22, 2017 •

edited

Loading

mlewand Nov 22, 2017 •

edited

Loading