Merge pull request #233 from ckeditor/t/227

Fix: Contextual toolbar should re–position correctly on window scroll. Closes #227.

Author: oskarwrobel

src/panel/balloon/balloonpanelview.js

@@ -251,19 +251,27 @@ export default class BalloonPanelView extends View {
 		this.attachTo( options );
 
 		const limiter = options.limiter || defaultLimiterElement;
-		let target = null;
+		let targetElement = null;
 
 		// We need to take HTMLElement related to the target if it is possible.
 		if ( isElement( options.target ) ) {
-			target = options.target;
+			targetElement = options.target;
 		} else if ( isRange( options.target ) ) {
-			target = options.target.commonAncestorContainer;
+			targetElement = options.target.commonAncestorContainer;
 		}
 
 		// Then we need to listen on scroll event of eny element in the document.
 		this.listenTo( global.document, 'scroll', ( evt, domEvt ) => {
-			// We need to update position if scrolled element contains related to the balloon elements.
-			if ( ( target && domEvt.target.contains( target ) ) || domEvt.target.contains( limiter ) ) {
+			const scrollTarget = domEvt.target;
+
+			// The position needs to be updated if the positioning target is within the scrolled element.
+			const isWithinScrollTarget = targetElement && scrollTarget.contains( targetElement );
+
+			// The position needs to be updated if the positioning limiter is within the scrolled element.
+			const isLimiterWithinScrollTarget = scrollTarget.contains( limiter );
+
+			// The positioning target can be a Rect, object etc.. There's no way to optimize the listener then.
+			if ( isWithinScrollTarget || isLimiterWithinScrollTarget || !targetElement ) {
 				this.attachTo( options );
 			}
 		}, { useCapture: true } );

src/toolbar/contextual/contextualtoolbar.js

@@ -217,16 +217,20 @@ export default class ContextualToolbar extends Plugin {
 		// Get direction of the selection.
 		const isBackward = editingView.selection.isBackward;
 
-		// getBoundingClientRect() makes no sense when the selection spans across number
-		// of lines of text. Using getClientRects() allows us to browse micro–ranges
-		// that would normally make up the bounding client rect.
-		const rangeRects = editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ).getClientRects();
-
-		// Select the proper range rect depending on the direction of the selection.
-		const rangeRect = isBackward ? rangeRects.item( 0 ) : rangeRects.item( rangeRects.length - 1 );
-
 		return {
-			target: rangeRect,
+			// Because the target for BalloonPanelView is a Rect (not DOMRange), it's geometry will stay fixed
+			// as the window scrolls. To let the BalloonPanelView follow such Rect, is must be continuously
+			// computed and hence, the target is defined as a function instead of a static value.
+			// https://github.com/ckeditor/ckeditor5-ui/issues/195
+			target: () => {
+				// getBoundingClientRect() makes no sense when the selection spans across number
+				// of lines of text. Using getClientRects() allows us to browse micro–ranges
+				// that would normally make up the bounding client rect.
+				const rangeRects = editingView.domConverter.viewRangeToDom( editingView.selection.getFirstRange() ).getClientRects();
+
+				// Select the proper range rect depending on the direction of the selection.
+				return isBackward ? rangeRects.item( 0 ) : rangeRects.item( rangeRects.length - 1 );
+			},
 			positions: isBackward ?
 				[ defaultPositions.northWestArrowSouth, defaultPositions.southWestArrowNorth ] :
 				[ defaultPositions.southEastArrowNorth, defaultPositions.northEastArrowSouth ]

tests/panel/balloon/balloonpanelview.js

@@ -592,7 +592,7 @@ describe( 'BalloonPanelView', () => {
 				sinon.assert.calledTwice( attachToSpy );
 			} );
 
-			it( 'should work for rect as a target', () => {
+			it( 'should work for a Rect as a target', () => {
 				// Just check if this normally works without errors.
 				const rect = {};
 
@@ -604,6 +604,17 @@ describe( 'BalloonPanelView', () => {
 
 				sinon.assert.calledTwice( attachToSpy );
 			} );
+
+			// https://github.com/ckeditor/ckeditor5-ui/issues/227
+			it( 'should react to #scroll from anywhere when the target is not an HTMLElement or Range', () => {
+				const rect = {};
+
+				view.pin( { target: rect } );
+				sinon.assert.calledOnce( attachToSpy );
+
+				notRelatedElement.dispatchEvent( new Event( 'scroll' ) );
+				sinon.assert.calledTwice( attachToSpy );
+			} );
 		} );
 
 		describe( 'unpin()', () => {

tests/toolbar/contextual/contextualtoolbar.js

@@ -140,7 +140,7 @@ describe( 'ContextualToolbar', () => {
 			editor.editing.view.isFocused = true;
 		} );
 
-		it( 'should return promise', () => {
+		it( 'should return a promise', () => {
 			setData( editor.document, '<paragraph>b[a]r</paragraph>' );
 
 			const returned = contextualToolbar._showPanel();
@@ -160,7 +160,7 @@ describe( 'ContextualToolbar', () => {
 					view: contextualToolbar.toolbarView,
 					balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container',
 					position: {
-						target: forwardSelectionRect,
+						target: sinon.match( value => value() == backwardSelectionRect ) ,
 						positions: [ defaultPositions.southEastArrowNorth, defaultPositions.northEastArrowSouth ]
 					}
 				} );
@@ -178,7 +178,7 @@ describe( 'ContextualToolbar', () => {
 						view: contextualToolbar.toolbarView,
 						balloonClassName: 'ck-toolbar-container ck-editor-toolbar-container',
 						position: {
-							target: backwardSelectionRect,
+							target: sinon.match( value => value() == forwardSelectionRect ),
 							positions: [ defaultPositions.northWestArrowSouth, defaultPositions.southWestArrowNorth ]
 						}
 					} );