Browse Source

Merge branch 'master' of github.com:ckeditor/ckeditor5-mention

Anna Tomanek 6 years ago
parent
commit
333c2b991b

+ 5 - 0
packages/ckeditor5-mention/.travis.yml

@@ -12,6 +12,7 @@ branches:
   except:
   - stable
 before_install:
+- export START_TIME=$( date +%s )
 - export DISPLAY=:99.0
 - sh -e /etc/init.d/xvfb start
 - npm i -g yarn
@@ -22,7 +23,11 @@ script:
 - ckeditor5-dev-tests-travis
 after_success:
 - ckeditor5-dev-tests-save-revision
+after_script:
+- export END_TIME=$( date +%s )
+- ckeditor5-dev-tests-notify-travis-status
 env:
   global:
   - secure: ZQA0E73V4IrxrhGCqVMnBDZR8gP0P8MlCgdOy2qUtSB2yR+syefZBcBVoa6YYTle8UmePO4ETta4y0FMH0zZdnSETEqQ5odKGdclhkFU8I4O9+7ZXGQEfQPVriZxXHjdCX66gRShphW+6tcg5M+jGs1TlBtmgNuMn4MU7Z16lLAouO4gaicOFfDQO/FF2yO5YJURY47ThO/IVfUJB5n1a5rAkRN9WmbAubkYqtC53wOg/Ipl7I6bquxaUAcPLd+2kT1T6g+SWISoOhfOTYwwaN9Tqapzdx3wwM47DgDmjZ3tDRuhIBPLTWx7Fk70VF+8wnRi0ydhXaZYoRh6dBbjZPO9IwOArMmV6SRANWt5KAmxW3glFuvjDih/ypR1r1YzDf2kKvLijPZCQpQKzeWPZ6X+kaVGeIe8mdRz0MCh2TBvCXVRBfmh9BNFmnLhg68ympJK1zBczFtTVdoNiTJ8eV4AsKb+NED6ZRdD0Ol7ydD4LOcl2LsIpWaYj04tXuvIe+hN7AjTGiOBFqEY0VGHSu9WyGziBCHR9yboDmy8il/ChBBgWOFYJXZNGnw+VHhFrkxAXDUYbLreVIZLPUMRscB31y982R0oiU0pVsGJTSP1v6i0JMAjNJ3ZsL7V4IoFwnqk2NbKj28JxTq2BMBx5g3ry9EW9WQ1fgzxJuWOo2I=
   - secure: dOp+yn/un5AyUPO7Zv7EY5LlEH59ASFb27DkyoIjKytcERp6iCMuHqGgbatsR1tOzSs8GF1Gqzh3K061tRjLWnIwq5iyEHlswy6o0SchHtQtsIk5Jfa/6PGwuYqsKGER9avxl4MZ4Nctdl0ljo5nZCifuRcXRB5aJFVMLDyG3KscYq38MN6RLo/YdNZ53NwaeBUwUHC6EuDEyOc7F80tCzjibpKUUonCM9P0h07BCNn8xT9vdQHRZCDlsbqHuOvIsPD//WY8q9GPzp+jQ2LrwQ1xCy2JhMAhFJ0zk1+h5O1D52nRf8G/PlqtIEWVyhr5XV6iYGocJM2jRYSvEKV0EB1kGk3rv/AVlKj2CxUrUUY/OV0JQUWo7YXPua6ivNVvEyXMsywCtHixuYUvSISVz0MgA6Y4LOcxcj7P9buwi67PsMS909iGCpyW5L+cBBvj5hdp12MZ71lhlCBwuGWNQarg9MYpI8ZFqMYBWHnCrkGC/OUQi6yg0x6cd7opydFyI94NmY673s1RddJ48Zfl35pJpqG5hD2lWPaMcnIayZ5uhrnqC+3No31g0Kb70AzoiEM09Rmn6S9vTwHJ9hChCwAyDCR2ZVQok4ypNRzUJdZjFfaL9jM6jVb8HBOrkDsMIv7GdjQPMQ45OFWj0U1ty1VxB6dBEv079WG88tQLYws=
+  - secure: II9fBZm1I7WDUkmN4402a4Fn5i44+W7DXczw49S68tAE6fqj1o7zKakWaFMbn+5ShAXmH/WVkn7bRMt2rBD9PT12osPzWXzo1+kCXnEi8u1auFGYz/Em6Rs6MOt0MWr0YPQEYyALBGVocwErJ6fw721aXUsMomoH0PsG005FOQGkqSrQL7QWjT9fIjsQhYx7RXDBQVeKX7kagqsuy9QPUSfcTHVPP9wiG+Q0oFBRXr5MZYguJ6zXB5sSFJ7uzAWL9OfrUmhH3n7jzRQq5lilTKLrLzwsxmFqQ9sU2+CGhzh9hoAVkLs1QIF8ufwV5DIQRPSQkVLVHnXAZNzrzIaMTiQ6mxpOoSpYVKXaHbKBF/dGj1IYIsLce8n3nwHATt4gNPRQUoZGlLpdMjDo+LiajDud8CaduLtZdiOB2x3ssCvPwInLRR2e80mIyAeGHew+CQ8R780my/+HaS/9Q2Vig/IfbV006dEi/AcTCX9V+JTVd9hPgP3bWUrhaOFxn/BuWgweZFnxtvEj9liczQXlCFVLDGOzjx1I/Ruy0u2IbI+0R8St9QJa6VJU8dWyxaIxvu1X/SRWop5iXEAEL3nTmBkZrdIweaT9p8ZvgCsXo7Hg+AUkx42fzXnWinXMCNWxuDhMQoXD0jf8UfgV0y4yy9Ky2pi8cconBhGV4ZKFXVQ=

+ 12 - 7
packages/ckeditor5-mention/src/mentionui.js

@@ -282,8 +282,14 @@ export default class MentionUI extends Plugin {
 
 			const selection = editor.model.document.selection;
 
-			const hasMention = selection.hasAttribute( 'mention' );
-			const nodeBefore = selection.focus.nodeBefore;
+			const focus = selection.focus;
+
+			// The text watcher listens only to changed range in selection - so the selection attributes are not yet available
+			// and you cannot use selection.hasAttribute( 'mention' ) just yet.
+			// See https://github.com/ckeditor/ckeditor5-engine/issues/1723.
+			const hasMention = focus.textNode && focus.textNode.hasAttribute( 'mention' );
+
+			const nodeBefore = focus.nodeBefore;
 
 			if ( hasMention || nodeBefore && nodeBefore.is( 'text' ) && nodeBefore.hasAttribute( 'mention' ) ) {
 				return;
@@ -294,8 +300,8 @@ export default class MentionUI extends Plugin {
 			const matchedTextLength = marker.length + feedText.length;
 
 			// Create a marker range.
-			const start = selection.focus.getShiftedBy( -matchedTextLength );
-			const end = selection.focus.getShiftedBy( -feedText.length );
+			const start = focus.getShiftedBy( -matchedTextLength );
+			const end = focus.getShiftedBy( -feedText.length );
 
 			const markerRange = editor.model.createRange( start, end );
 
@@ -447,8 +453,7 @@ export default class MentionUI extends Plugin {
 
 				return null;
 			},
-			positions: getBalloonPanelPositions( positionName ),
-			fitInViewport: true
+			positions: getBalloonPanelPositions( positionName )
 		};
 	}
 }
@@ -505,8 +510,8 @@ function getBalloonPanelPositions( positionName ) {
 	// By default return all position callbacks.
 	return [
 		positions.caret_se,
-		positions.caret_ne,
 		positions.caret_sw,
+		positions.caret_ne,
 		positions.caret_nw
 	];
 }

+ 2 - 2
packages/ckeditor5-mention/src/textwatcher.js

@@ -52,8 +52,8 @@ export default class TextWatcher {
 	_startListening() {
 		const editor = this.editor;
 
-		editor.model.document.selection.on( 'change', ( evt, { directChange } ) => {
-			// Indirect changes (i.e. on typing) are handled in the document's change event.
+		editor.model.document.selection.on( 'change:range', ( evt, { directChange } ) => {
+			// Indirect changes (i.e. when the user types or external changes are applied) are handled in the document's change event.
 			if ( !directChange ) {
 				return;
 			}

+ 10 - 4
packages/ckeditor5-mention/src/ui/mentionsview.js

@@ -85,18 +85,24 @@ export default class MentionsView extends ListView {
 		}
 
 		const item = this.items.get( indexToGet );
-		item.highlight();
 
-		// Scroll the mentions view to the selected element.
-		if ( !this._isItemVisibleInScrolledArea( item ) ) {
-			this.element.scrollTop = item.element.offsetTop;
+		// Return early if item is already selected.
+		if ( this.selected === item ) {
+			return;
 		}
 
+		// Remove highlight of previously selected item.
 		if ( this.selected ) {
 			this.selected.removeHighlight();
 		}
 
+		item.highlight();
 		this.selected = item;
+
+		// Scroll the mentions view to the selected element.
+		if ( !this._isItemVisibleInScrolledArea( item ) ) {
+			this.element.scrollTop = item.element.offsetTop;
+		}
 	}
 
 	/**

+ 146 - 42
packages/ckeditor5-mention/tests/mentionui.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* global window, document, setTimeout, Event */
+/* global document, setTimeout, Event */
 
 import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
@@ -15,11 +15,12 @@ import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';
 import EventInfo from '@ckeditor/ckeditor5-utils/src/eventinfo';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+import env from '@ckeditor/ckeditor5-utils/src/env';
 
 import MentionUI from '../src/mentionui';
 import MentionEditing from '../src/mentionediting';
 import MentionsView from '../src/ui/mentionsview';
-import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 describe( 'MentionUI', () => {
 	let editor, model, doc, editingView, mentionUI, editorElement, mentionsView, panelView;
@@ -151,9 +152,8 @@ describe( 'MentionUI', () => {
 			return waitForDebounce()
 				.then( () => {
 					const pinArgument = pinSpy.firstCall.args[ 0 ];
-					const { target, positions, limiter, fitInViewport } = pinArgument;
+					const { target, positions, limiter } = pinArgument;
 
-					expect( fitInViewport ).to.be.true;
 					expect( positions ).to.have.length( 4 );
 
 					// Mention UI should set limiter to the editable area.
@@ -177,8 +177,8 @@ describe( 'MentionUI', () => {
 					expect( mentionMarker.getRange().isEqual( range ), 'Should position to mention marker.' );
 
 					const caretSouthEast = positions[ 0 ];
-					const caretNorthEast = positions[ 1 ];
-					const caretSouthWest = positions[ 2 ];
+					const caretSouthWest = positions[ 1 ];
+					const caretNorthEast = positions[ 2 ];
 					const caretNorthWest = positions[ 3 ];
 
 					expect( caretSouthEast( caretRect, balloonRect ) ).to.deep.equal( {
@@ -187,18 +187,18 @@ describe( 'MentionUI', () => {
 						top: 121
 					} );
 
-					expect( caretNorthEast( caretRect, balloonRect ) ).to.deep.equal( {
-						left: 501,
-						name: 'caret_ne',
-						top: -53
-					} );
-
 					expect( caretSouthWest( caretRect, balloonRect ) ).to.deep.equal( {
 						left: 301,
 						name: 'caret_sw',
 						top: 121
 					} );
 
+					expect( caretNorthEast( caretRect, balloonRect ) ).to.deep.equal( {
+						left: 501,
+						name: 'caret_ne',
+						top: -53
+					} );
+
 					expect( caretNorthWest( caretRect, balloonRect ) ).to.deep.equal( {
 						left: 301,
 						name: 'caret_nw',
@@ -344,38 +344,91 @@ describe( 'MentionUI', () => {
 					writer.insertText( '@', doc.selection.getFirstPosition() );
 				} );
 
+				const arrowDownEvtData = {
+					keyCode: keyCodes.arrowdown,
+					preventDefault: sinon.spy(),
+					stopPropagation: sinon.spy()
+				};
+
+				const arrowUpEvtData = {
+					keyCode: keyCodes.arrowup,
+					preventDefault: sinon.spy(),
+					stopPropagation: sinon.spy()
+				};
+
+				const mentionElementSpy = testUtils.sinon.spy( mentionsView.element, 'scrollTop', [ 'set' ] );
+
 				return waitForDebounce()
 					.then( () => {
-						expect( panelView.isVisible ).to.be.true;
+						// The scroll test highly depends on browser styles.
+						// Some CI test environments might not load theme which will result that tests will not render on CI as locally.
+						// To make this test repeatable across different environments it enforces mentions view size to 100px...
+						const reset = 'padding:0px;margin:0px;border:0 none;line-height: 1em;';
 
-						expectChildViewsIsOnState( [ true, false, false, false, false, false, false, false, false, false ] );
+						mentionsView.element.style = `min-height:100px;height:100px;max-height:100px;${ reset };`;
 
-						const arrowDownEvtData = {
-							keyCode: keyCodes.arrowdown,
-							preventDefault: sinon.spy(),
-							stopPropagation: sinon.spy()
-						};
+						// ...and each list view item size to 25px...
+						Array.from( mentionsView.items ).forEach( item => {
+							const listItemElement = item.children.get( 0 ).element;
 
-						const arrowUpEvtData = {
-							keyCode: keyCodes.arrowup,
-							preventDefault: sinon.spy(),
-							stopPropagation: sinon.spy()
-						};
+							listItemElement.style = `min-height:unset;height:25px;max-height:25px;${ reset };min-width:12em;`;
+						} );
+
+						// ...so after those changes it is safe to assume that:
+						// - base offset is 0
+						// - only 4 items are visible at once
+						// - if scrolled to the last element scrollTop will be set to 150px. The 150px is the offset of the 7th item in the
+						//   list as last four (7, 8, 9 & 10) will be visible.
+						expect( panelView.isVisible ).to.be.true;
+						expectChildViewsIsOnState( [ true, false, false, false, false, false, false, false, false, false ] );
+
+						// Edge browser always tries to scroll in tests environment: See ckeditor5-utils#282.
+						if ( !env.isEdge ) {
+							sinon.assert.callCount( mentionElementSpy.set, 0 );
+						}
 
 						fireKeyDownEvent( arrowDownEvtData );
-						expect( mentionsView.element.scrollTop ).to.equal( 0 );
 
 						expectChildViewsIsOnState( [ false, true, false, false, false, false, false, false, false, false ] );
+						// Edge browser always tries to scroll in tests environment: See ckeditor5-utils#282.
+						if ( !env.isEdge ) {
+							expect( mentionsView.element.scrollTop ).to.equal( 0 );
+							sinon.assert.callCount( mentionElementSpy.set, 0 );
+						}
 
 						fireKeyDownEvent( arrowUpEvtData );
+
+						expectChildViewsIsOnState( [ true, false, false, false, false, false, false, false, false, false ] );
+						expect( mentionsView.element.scrollTop ).to.equal( 0 );
+
+						// Edge browser always tries to scroll in tests environment: See ckeditor5-utils#282.
+						if ( !env.isEdge ) {
+							sinon.assert.callCount( mentionElementSpy.set, 0 );
+						}
+
 						fireKeyDownEvent( arrowUpEvtData );
 
 						expectChildViewsIsOnState( [ false, false, false, false, false, false, false, false, false, true ] );
-						expect( mentionsView.element.scrollTop ).to.be.not.equal( 0 );
+
+						// We want 150, but sometimes we get e.g. 151.
+						expect( mentionsView.element.scrollTop ).to.be.within( 140, 160, 'last item highlighted' );
+
+						// Edge browser always tries to scroll in tests environment: See ckeditor5-utils#282.
+						if ( !env.isEdge ) {
+							sinon.assert.callCount( mentionElementSpy.set, 1 );
+						}
 
 						fireKeyDownEvent( arrowDownEvtData );
+
 						expectChildViewsIsOnState( [ true, false, false, false, false, false, false, false, false, false ] );
-						expect( mentionsView.element.scrollTop ).to.equal( 0 );
+
+						// We want 0, but sometimes we get e.g. 1. (Firefox)
+						expect( mentionsView.element.scrollTop ).to.be.within( 0, 10 );
+
+						// Edge browser always tries to scroll in tests environment: See ckeditor5-utils#282.
+						if ( !env.isEdge ) {
+							sinon.assert.callCount( mentionElementSpy.set, 2 );
+						}
 					} );
 			} );
 		} );
@@ -432,9 +485,9 @@ describe( 'MentionUI', () => {
 			} );
 
 			it( 'should not show panel when selection is inside a mention', () => {
-				setData( model, '<paragraph>foo [@John] bar</paragraph>' );
+				setData( model, '<paragraph>foo [@Lily] bar</paragraph>' );
 				model.change( writer => {
-					writer.setAttribute( 'mention', { id: '@John', _uid: 1234 }, doc.selection.getFirstRange() );
+					writer.setAttribute( 'mention', { id: '@Lily', _uid: 1234 }, doc.selection.getFirstRange() );
 				} );
 
 				model.change( writer => {
@@ -449,9 +502,9 @@ describe( 'MentionUI', () => {
 			} );
 
 			it( 'should not show panel when selection is at the end of a mention', () => {
-				setData( model, '<paragraph>foo [@John] bar</paragraph>' );
+				setData( model, '<paragraph>foo [@Lily] bar</paragraph>' );
 				model.change( writer => {
-					writer.setAttribute( 'mention', { id: '@John', _uid: 1234 }, doc.selection.getFirstRange() );
+					writer.setAttribute( 'mention', { id: '@Lily', _uid: 1234 }, doc.selection.getFirstRange() );
 				} );
 
 				model.change( writer => {
@@ -522,9 +575,9 @@ describe( 'MentionUI', () => {
 			} );
 
 			it( 'should not show panel when selection is after existing mention', () => {
-				setData( model, '<paragraph>foo [@John] bar[]</paragraph>' );
+				setData( model, '<paragraph>foo [@Lily] bar[]</paragraph>' );
 				model.change( writer => {
-					writer.setAttribute( 'mention', { id: '@John', _uid: 1234 }, doc.selection.getFirstRange() );
+					writer.setAttribute( 'mention', { id: '@Lily', _uid: 1234 }, doc.selection.getFirstRange() );
 				} );
 
 				return waitForDebounce()
@@ -541,6 +594,33 @@ describe( 'MentionUI', () => {
 					} );
 			} );
 
+			it( 'should not show panel when selection moves inside existing mention', () => {
+				setData( model, '<paragraph>foo [@Lily] bar</paragraph>' );
+
+				model.change( writer => {
+					writer.setAttribute( 'mention', { id: '@Lily', _uid: 1234 }, doc.selection.getFirstRange() );
+				} );
+
+				return waitForDebounce()
+					.then( () => {
+						model.change( writer => {
+							writer.setSelection( doc.getRoot().getChild( 0 ), 9 );
+						} );
+					} )
+					.then( waitForDebounce )
+					.then( () => {
+						expect( panelView.isVisible ).to.be.false;
+
+						model.change( writer => {
+							writer.setSelection( doc.getRoot().getChild( 0 ), 8 );
+						} );
+					} )
+					.then( waitForDebounce )
+					.then( () => {
+						expect( panelView.isVisible ).to.be.false;
+					} );
+			} );
+
 			it( 'should show filtered results for matched text', () => {
 				setData( model, '<paragraph>foo []</paragraph>' );
 
@@ -897,6 +977,39 @@ describe( 'MentionUI', () => {
 							expectChildViewsIsOnState( [ true, false, false, false, false ] );
 						} );
 				} );
+
+				it( 'should not cycle with only one item in the list', () => {
+					setData( model, '<paragraph>foo []</paragraph>' );
+
+					const keyDownEvtData = {
+						keyCode: keyCodes.arrowdown,
+						preventDefault: sinon.spy(),
+						stopPropagation: sinon.spy()
+					};
+
+					const keyUpEvtData = {
+						keyCode: keyCodes.arrowdown,
+						preventDefault: sinon.spy(),
+						stopPropagation: sinon.spy()
+					};
+
+					model.change( writer => {
+						writer.insertText( '@T', doc.selection.getFirstPosition() );
+					} );
+
+					return waitForDebounce()
+						.then( () => {
+							expectChildViewsIsOnState( [ true ] );
+
+							fireKeyDownEvent( keyDownEvtData );
+
+							expectChildViewsIsOnState( [ true ] );
+
+							fireKeyDownEvent( keyUpEvtData );
+
+							expectChildViewsIsOnState( [ true ] );
+						} );
+				} );
 			} );
 
 			describe( 'on "execute" keys', () => {
@@ -1446,15 +1559,6 @@ describe( 'MentionUI', () => {
 				mentionUI = editor.plugins.get( MentionUI );
 				panelView = mentionUI.panelView;
 				mentionsView = mentionUI._mentionsView;
-
-				editingView.attachDomRoot( editorElement );
-
-				// Focus the engine.
-				editingView.document.isFocused = true;
-				editingView.getDomRoot().focus();
-
-				// Remove all selection ranges from DOM before testing.
-				window.getSelection().removeAllRanges();
 			} );
 	}