Merge pull request #386 from ckeditor/t/385

Fix: The balloon toolbar should be attached correctly in case of a multi-range selection. Closes #385.

Author: oleq

src/toolbar/balloon/balloontoolbar.js

@@ -202,6 +202,7 @@ export default class BalloonToolbar extends Plugin {
 		const editor = this.editor;
 		const view = editor.editing.view;
 		const viewDocument = view.document;
+		const viewSelection = viewDocument.selection;
 
 		// Get direction of the selection.
 		const isBackward = viewDocument.selection.isBackward;
@@ -212,7 +213,7 @@ export default class BalloonToolbar extends Plugin {
 			// computed and hence, the target is defined as a function instead of a static value.
 			// https://github.com/ckeditor/ckeditor5-ui/issues/195
 			target: () => {
-				const range = viewDocument.selection.getFirstRange();
+				const range = isBackward ? viewSelection.getFirstRange() : viewSelection.getLastRange();
 				const rangeRects = Rect.getDomRangeRects( view.domConverter.viewRangeToDom( range ) );
 
 				// Select the proper range rect depending on the direction of the selection.

tests/manual/tickets/385/1.html

@@ -0,0 +1,13 @@
+<div id="editor">
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+	<figure class="image">
+		<img src="sample.jpg" alt="Sample image">
+	</figure>
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+	<p>This is a line of text.</p>
+</div>

tests/manual/tickets/385/1.js

@@ -0,0 +1,23 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals window, document, console:false */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
+import BalloonToolbar from '../../../../src/toolbar/balloon/balloontoolbar';
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ ArticlePluginSet, BalloonToolbar ],
+		toolbar: [ 'bold', 'italic', 'link', 'undo', 'redo' ],
+		balloonToolbar: [ 'bold', 'italic', 'link' ]
+	} )
+	.then( editor => {
+		window.editor = editor;
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

tests/manual/tickets/385/1.md

@@ -0,0 +1,13 @@
+## Incorrect BalloonToolbar position in a case of multi-range selection [#385](https://github.com/ckeditor/ckeditor5-ui/issues/385)
+
+### Use Firefox to this test, check the ticket for the explanation.
+
+1. Make a forward selection that starts in the text before the image and ends in the text after the image
+   ```
+   <p>Line of text</p>
+   <p>Line o{f text</p>
+   <figure>...</figure>
+   <p>Line o}f text</p>
+   <p>Line of text</p>
+   ```
+2. Check if the balloon toolbar is attached to the end of the selection

tests/manual/tickets/385/sample.jpg

@@ -14,7 +14,8 @@ import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic';
 import Underline from '@ckeditor/ckeditor5-basic-styles/src/underline';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 
-import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js';
+import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { stringify as viewStringify } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
 /* global document, setTimeout, window */
 
@@ -199,6 +200,22 @@ describe( 'BalloonToolbar', () => {
 			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( forwardSelectionRect );
 		} );
 
+		// https://github.com/ckeditor/ckeditor5-ui/issues/385
+		it( 'should attach the #_balloon to the last range in a case of multi-range forward selection', () => {
+			setData( model, '<paragraph>b[ar]</paragraph><paragraph>[bi]z</paragraph>' );
+
+			balloonToolbar.show();
+
+			// Because attaching and pinning BalloonPanelView is stubbed for test
+			// we need to manually call function that counting rect.
+			const targetRect = balloonAddSpy.firstCall.args[ 0 ].position.target();
+
+			const targetViewRange = editingView.domConverter.viewRangeToDom.lastCall.args[ 0 ];
+
+			expect( viewStringify( targetViewRange.root, targetViewRange ) ).to.equal( '<div><p>bar</p><p>{bi}z</p></div>' );
+			expect( targetRect ).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.
@@ -242,6 +259,22 @@ describe( 'BalloonToolbar', () => {
 			expect( balloonAddSpy.firstCall.args[ 0 ].position.target() ).to.deep.equal( backwardSelectionRect );
 		} );
 
+		// https://github.com/ckeditor/ckeditor5-ui/issues/385
+		it( 'should attach the #_balloon to the first range in a case of multi-range backward selection', () => {
+			setData( model, '<paragraph>b[ar]</paragraph><paragraph>[bi]z</paragraph>', { lastRangeBackward: true } );
+
+			balloonToolbar.show();
+
+			// Because attaching and pinning BalloonPanelView is stubbed for test
+			// we need to manually call function that counting rect.
+			const targetRect = balloonAddSpy.firstCall.args[ 0 ].position.target();
+
+			const targetViewRange = editingView.domConverter.viewRangeToDom.lastCall.args[ 0 ];
+
+			expect( viewStringify( targetViewRange.root, targetViewRange ) ).to.equal( '<div><p>b{ar}</p><p>biz</p></div>' );
+			expect( targetRect ).to.deep.equal( backwardSelectionRect );
+		} );
+
 		it( 'should update balloon position on view#change event while balloon is added to the #_balloon', () => {
 			setData( model, '<paragraph>b[a]r</paragraph>' );