瀏覽代碼

Merge branch 'master' into t/27

# Conflicts:
#	tests/mentionui.js
Maciej Gołaszewski 6 年之前
父節點
當前提交
e3b66f359d

+ 10 - 4
packages/ckeditor5-mention/src/mentionui.js

@@ -277,8 +277,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;
@@ -289,8 +295,8 @@ export default class MentionUI extends Plugin {
 			const matchedTextLength = marker.length + feedText.length;
 
 			// create 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 );
 

+ 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 } ) => {
-			// The indirect changes (ie on typing) are handled in document's change event.
+		editor.model.document.selection.on( 'change:range', ( evt, { directChange } ) => {
+			// The indirect changes (ie when user types or external changes are applied) are handled in 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;
+		}
 	}
 
 	/**

+ 120 - 25
packages/ckeditor5-mention/tests/mentionui.js

@@ -16,6 +16,7 @@ import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventd
 import EventInfo from '@ckeditor/ckeditor5-utils/src/eventinfo';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon';
+import env from '@ckeditor/ckeditor5-utils/src/env';
 
 import MentionUI from '../src/mentionui';
 import MentionEditing from '../src/mentionediting';
@@ -325,28 +326,34 @@ describe( 'MentionUI', () => {
 					writer.insertText( '@', doc.selection.getFirstPosition() );
 				} );
 
-				return waitForDebounce()
-					.then( () => {
-						const arrowDownEvtData = {
-							keyCode: keyCodes.arrowdown,
-							preventDefault: sinon.spy(),
-							stopPropagation: sinon.spy()
-						};
+				const arrowDownEvtData = {
+					keyCode: keyCodes.arrowdown,
+					preventDefault: sinon.spy(),
+					stopPropagation: sinon.spy()
+				};
 
-						const arrowUpEvtData = {
-							keyCode: keyCodes.arrowup,
-							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( () => {
 						// 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...
-						mentionsView.element.style = 'height:100px;padding:0px;margin:0px';
+						const reset = 'padding:0px;margin:0px;border:0 none;line-height: 1em;';
+
+						mentionsView.element.style = `min-height:100px;height:100px;max-height:100px;${ reset };`;
 
 						// ...and each list view item size to 25px...
 						Array.from( mentionsView.items ).forEach( item => {
-							item.children.get( 0 ).element.style = 'height:25px;padding:0px;margin:0px;';
+							const listItemElement = item.children.get( 0 ).element;
+
+							listItemElement.style = `min-height:unset;height:25px;max-height:25px;${ reset };min-width:12em;`;
 						} );
 
 						// ...so after those changes it is safe to assume that:
@@ -355,27 +362,55 @@ describe( 'MentionUI', () => {
 						// - 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 ] );
-						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( arrowDownEvtData );
 
 						expectChildViewsIsOnState( [ false, true, 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 ) {
+							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.within( 140, 160 ); // We want 150, but sometimes we get e.g. 151.
+
+						// 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 +467,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 +484,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,13 +557,40 @@ 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: '@Lily', _uid: 1234 }, doc.selection.getFirstRange() );
+				} );
+
+				return 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 not show panel when selection moves inside existing mention', () => {
+				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()
 					.then( () => {
+						model.change( writer => {
+							writer.setSelection( doc.getRoot().getChild( 0 ), 9 );
+						} );
+					} )
+					.then( waitForDebounce )
+					.then( () => {
 						expect( panelView.isVisible ).to.be.false;
 
 						model.change( writer => {
@@ -897,6 +959,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', () => {