Merge pull request #309 from ckeditor/t/308

Fix: `ContextualToolbar` should not be positioned to a zero–width DOM rect when invoked for a multi-line forward selection. Closes #308.
ckeditor · Sep 21, 2017 · 00b701b · 00b701b
2 parents 2bea29c + 938379e
commit 00b701b
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 9 deletions.
diff --git a/src/toolbar/contextual/contextualtoolbar.js b/src/toolbar/contextual/contextualtoolbar.js
@@ -214,7 +214,18 @@ export default class ContextualToolbar extends Plugin {
 				const rangeRects = Rect.getDomRangeRects( editingView.domConverter.viewRangeToDom( range ) );
 
 				// Select the proper range rect depending on the direction of the selection.
-				return rangeRects[ isBackward ? 0 : rangeRects.length - 1 ];
+				if ( isBackward ) {
+					return rangeRects[ 0 ];
+				} else {
+					// Ditch the zero-width "orphan" rect in the next line for the forward selection if there's
+					// another one preceding it. It is not rendered as a selection by the web browser anyway.
+					// https://github.com/ckeditor/ckeditor5-ui/issues/308
+					if ( rangeRects.length > 1 && rangeRects[ rangeRects.length - 1 ].width === 0 ) {
+						rangeRects.pop();
+					}
+
+					return rangeRects[ rangeRects.length - 1 ];
+				}
 			},
 			positions: getBalloonPositions( isBackward )
 		};

diff --git a/tests/toolbar/contextual/contextualtoolbar.js b/tests/toolbar/contextual/contextualtoolbar.js
@@ -115,10 +115,10 @@ describe( 'ContextualToolbar', () => {
 	} );
 
 	describe( 'show()', () => {
-		let balloonAddSpy, forwardSelectionRect, backwardSelectionRect;
+		let balloonAddSpy, backwardSelectionRect, forwardSelectionRect;
 
 		beforeEach( () => {
-			forwardSelectionRect = {
+			backwardSelectionRect = {
 				top: 100,
 				height: 10,
 				bottom: 110,
@@ -127,7 +127,7 @@ describe( 'ContextualToolbar', () => {
 				right: 250
 			};
 
-			backwardSelectionRect = {
+			forwardSelectionRect = {
 				top: 200,
 				height: 10,
 				bottom: 210,
@@ -136,7 +136,10 @@ describe( 'ContextualToolbar', () => {
 				right: 250
 			};
 
-			stubSelectionRect( forwardSelectionRect, backwardSelectionRect );
+			stubSelectionRects( [
+				backwardSelectionRect,
+				forwardSelectionRect
+			] );
 
 			balloonAddSpy = sandbox.spy( balloon, 'add' );
 			editor.editing.view.isFocused = true;
@@ -165,7 +168,24 @@ describe( 'ContextualToolbar', () => {
 				}
 			} );
 
-			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( backwardSelectionRect );
+			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( forwardSelectionRect );
+		} );
+
+		// https://github.com/ckeditor/ckeditor5-ui/issues/308
+		it( 'should ignore the zero-width orphan rect if there another one preceding it for the forward selection', () => {
+			// Restore previous stubSelectionRects() call.
+			editor.editing.view.domConverter.viewRangeToDom.restore();
+
+			// Simulate an "orphan" rect preceded by a "correct" one.
+			stubSelectionRects( [
+				forwardSelectionRect,
+				{ width: 0 }
+			] );
+
+			setData( editor.document, '<paragraph>b[a]r</paragraph>' );
+
+			contextualToolbar.show();
+			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( forwardSelectionRect );
 		} );
 
 		it( 'should add #toolbarView to the #_balloon and attach the #_balloon to the selection for the backward selection', () => {
@@ -191,7 +211,7 @@ describe( 'ContextualToolbar', () => {
 				}
 			} );
 
-			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( forwardSelectionRect );
+			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( backwardSelectionRect );
 		} );
 
 		it( 'should update balloon position on ViewDocument#render event while balloon is added to the #_balloon', () => {
@@ -451,7 +471,7 @@ describe( 'ContextualToolbar', () => {
 		} );
 	} );
 
-	function stubSelectionRect( forwardSelectionRect, backwardSelectionRect ) {
+	function stubSelectionRects( rects ) {
 		const editingView = editor.editing.view;
 		const originalViewRangeToDom = editingView.domConverter.viewRangeToDom;
 
@@ -460,7 +480,7 @@ describe( 'ContextualToolbar', () => {
 			const domRange = originalViewRangeToDom.apply( editingView.domConverter, args );
 
 			sandbox.stub( domRange, 'getClientRects' )
-				.returns( [ forwardSelectionRect, backwardSelectionRect ] );
+				.returns( rects );
 
 			return domRange;
 		} );