Przeglądaj źródła

Fix: The two step caret movement should not engage when approaching unrelated attributes inside the main attribute.

Aleksander Nowodzinski 7 lat temu
rodzic
commit
9429a42838

+ 31 - 28
packages/ckeditor5-engine/src/model/documentselection.js

@@ -18,6 +18,7 @@ import TextProxy from './textproxy';
 import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import log from '@ckeditor/ckeditor5-utils/src/log';
+import uid from '@ckeditor/ckeditor5-utils/src/uid';
 
 const storePrefix = 'selection:';
 
@@ -406,12 +407,10 @@ export default class DocumentSelection {
 	 *
 	 * @see module:engine/model/writer~Writer#overrideSelectionGravity
 	 * @protected
-	 * @param {Boolean} [customRestore=false] When `true` then gravity won't be restored until
-	 * {@link ~DocumentSelection#_restoreGravity} will be called directly. When `false` then gravity is restored
-	 * after selection is moved by user.
+	 * @returns {String} The unique id which allows restoring the gravity.
 	 */
-	_overrideGravity( customRestore ) {
-		this._selection.overrideGravity( customRestore );
+	_overrideGravity() {
+		return this._selection.overrideGravity();
 	}
 
 	/**
@@ -421,9 +420,10 @@ export default class DocumentSelection {
 	 *
 	 * @see module:engine/model/writer~Writer#restoreSelectionGravity
 	 * @protected
+	 * @param {String} uid The unique id returned by {@link #_overrideGravity}.
 	 */
-	_restoreGravity() {
-		this._selection.restoreGravity();
+	_restoreGravity( uid ) {
+		this._selection.restoreGravity( uid );
 	}
 
 	/**
@@ -530,12 +530,13 @@ class LiveSelection extends Selection {
 		// @member {Array} module:engine/model/liveselection~LiveSelection#_hasChangedRange
 		this._hasChangedRange = false;
 
-		// Each overriding gravity increase the counter and each restoring decrease it.
-		// Gravity is overridden when counter is greater than 0. This is to prevent conflicts when
-		// gravity is overridden by more than one feature at the same time.
+		// Each overriding gravity adds an UID to the set and each removal removes it.
+		// Gravity is overridden when there's at least one UID in the set.
+		// Gravity is restored when the set is empty.
+		// This is to prevent conflicts when gravity is overridden by more than one feature at the same time.
 		// @private
-		// @type {Number}
-		this._overriddenGravityCounter = 0;
+		// @type {Set}
+		this._overriddenGravityRegister = new Set();
 
 		// Add events that will ensure selection correctness.
 		this.on( 'change:range', () => {
@@ -612,7 +613,7 @@ class LiveSelection extends Selection {
 	// @protected
 	// @type {Boolean}
 	get isGravityOverridden() {
-		return this._overriddenGravityCounter > 0;
+		return !!this._overriddenGravityRegister.size;
 	}
 
 	// Unbinds all events previously bound by live selection.
@@ -666,28 +667,30 @@ class LiveSelection extends Selection {
 		}
 	}
 
-	overrideGravity( customRestore ) {
-		this._overriddenGravityCounter++;
+	overrideGravity() {
+		const overrideUid = uid();
 
-		if ( this._overriddenGravityCounter == 1 ) {
-			if ( !customRestore ) {
-				this.on( 'change:range', ( evt, data ) => {
-					if ( data.directChange ) {
-						this.restoreGravity();
-						evt.off();
-					}
-				} );
-			}
+		// Remember that another overriding has been requested. It will need to be removed
+		// before the gravity is to be restored.
+		this._overriddenGravityRegister.add( overrideUid );
 
-			this._updateAttributes();
+		if ( this._overriddenGravityRegister.size === 1 ) {
+			this._refreshAttributes();
 		}
+
+		return overrideUid;
 	}
 
-	restoreGravity() {
-		this._overriddenGravityCounter--;
+	restoreGravity( uid ) {
+		if ( !this._overriddenGravityRegister.has( uid ) ) {
+			throw 'Restoring gravity for unknown id ' + uid;
+		}
+
+		this._overriddenGravityRegister.delete( uid );
 
+		// Restore gravity only when all overriding have been restored.
 		if ( !this.isGravityOverridden ) {
-			this._updateAttributes();
+			this._refreshAttributes();
 		}
 	}
 

+ 7 - 15
packages/ckeditor5-engine/src/model/writer.js

@@ -1112,29 +1112,21 @@ export default class Writer {
 	 * * Default gravity: selection will have the `bold` and `linkHref` attributes.
 	 * * Overridden gravity: selection will have `bold` attribute.
 	 *
-	 * By default the selection's gravity is automatically restored just after a direct selection change (when user
-	 * moved the caret) but you can pass `customRestore = true` in which case you will have to call
-	 * {@link ~Writer#restoreSelectionGravity} manually.
-	 *
-	 * When the selection's gravity is overridden more than once without being restored in the meantime then it needs
-	 * to be restored the same number of times. This is to prevent conflicts when
-	 * more than one feature want to independently override and restore the selection's gravity.
-	 *
-	 * @param {Boolean} [customRestore=false] When `true` then gravity won't be restored until
-	 * {@link ~Writer#restoreSelectionGravity} will be called directly. When `false` then gravity is restored
-	 * after selection is moved by user.
+	 * @returns {String} The unique id which allows restoring the gravity.
 	 */
-	overrideSelectionGravity( customRestore ) {
-		this.model.document.selection._overrideGravity( customRestore );
+	overrideSelectionGravity() {
+		return this.model.document.selection._overrideGravity();
 	}
 
 	/**
 	 * Restores {@link ~Writer#overrideSelectionGravity} gravity to default.
 	 *
 	 * Note that the gravity remains overridden as long as will not be restored the same number of times as it was overridden.
+	 *
+	 * @param {String} uid The unique id returned by {@link #overrideSelectionGravity}.
 	 */
-	restoreSelectionGravity() {
-		this.model.document.selection._restoreGravity();
+	restoreSelectionGravity( uid ) {
+		this.model.document.selection._restoreGravity( uid );
 	}
 
 	/**

+ 246 - 51
packages/ckeditor5-engine/src/utils/bindtwostepcarettoattribute.js

@@ -38,6 +38,8 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
  */
 export default function bindTwoStepCaretToAttribute( view, model, emitter, attribute ) {
 	const modelSelection = model.document.selection;
+	let overrideUid;
+	let skipNextChangeRange = false;
 
 	// Listen to keyboard events and handle cursor before the move.
 	emitter.listenTo( view.document, 'keydown', ( evt, data ) => {
@@ -54,7 +56,7 @@ export default function bindTwoStepCaretToAttribute( view, model, emitter, attri
 			return;
 		}
 
-		// When user tries to expand selection or jump over the whole word or to the beginning/end then
+		// When user tries to expand the selection or jump over the whole word or to the beginning/end then
 		// two-steps movement is not necessary.
 		if ( data.shiftKey || data.altKey || data.ctrlKey ) {
 			return;
@@ -62,80 +64,273 @@ export default function bindTwoStepCaretToAttribute( view, model, emitter, attri
 
 		const position = modelSelection.getFirstPosition();
 
-		// Moving right.
 		if ( arrowRightPressed ) {
-			// If gravity is already overridden then do nothing.
-			// It means that we already enter `foo<a>{}bar</a>biz` or left `foo<a>bar</a>{}biz` text with attribute
-			// and gravity will be restored just after caret movement.
-			if ( modelSelection.isGravityOverridden ) {
+			handleArrowRightPress( position, data );
+		} else {
+			handleArrowLeftPress( position, data );
+		}
+	} );
+
+	emitter.listenTo( modelSelection, 'change:range', ( evt, data ) => {
+		if ( skipNextChangeRange ) {
+			skipNextChangeRange = false;
+
+			return;
+		}
+
+		// Skip automatic restore when the gravity is not overridden — simply, there's nothing to restore
+		// at this moment.
+		if ( !overrideUid ) {
+			return;
+		}
+
+		// Skip automatic restore when the change is indirect AND the selection is at the attribute boundary.
+		// It means that e.g. if the change was external (collaboration) and the user had their
+		// selection around the link, its gravity should remain intact in this change:range event.
+		if ( !data.directChange && isAtBoundary( modelSelection.getFirstPosition(), attribute ) ) {
+			return;
+		}
+
+		restoreGravity( model );
+	} );
+
+	function handleArrowRightPress( position, data ) {
+		// DON'T ENGAGE 2-SCM if gravity is already overridden. It means that we just entered
+		//
+		// 		<paragraph>foo{}<$text attribute>bar</$text>baz</paragraph>
+		//
+		// or left the attribute
+		//
+		// 		<paragraph>foo<$text attribute>bar</$text>{}baz</paragraph>
+		//
+		// and the gravity will be restored automatically.
+		if ( overrideUid ) {
+			return;
+		}
+
+		// DON'T ENGAGE 2-SCM when the selection is at the beginning of an attribute AND already has it:
+		// * when the selection was initially set there using the mouse,
+		// * when the editor has just started
+		//
+		//		<paragraph><$text attribute>{}bar</$text>baz</paragraph>
+		//
+		if ( position.isAtStart && modelSelection.hasAttribute( attribute ) ) {
+			return;
+		}
+
+		// ENGAGE 2-SCM when about to leave one attribute value and enter another:
+		//
+		// 		<paragraph><$text attribute="1">foo{}</$text><$text attribute="2">bar</$text></paragraph>
+		//
+		// but DON'T when already in between of them (no attribute selection):
+		//
+		// 		<paragraph><$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text></paragraph>
+		//
+		if ( isBetweenDifferentValues( position, attribute ) && modelSelection.hasAttribute( attribute ) ) {
+			preventCaretMovement( data );
+			removeSelectionAttribute( model );
+		} else {
+			// ENGAGE 2-SCM when entering an attribute:
+			//
+			// 		<paragraph>foo{}<$text attribute>bar</$text>baz</paragraph>
+			//
+			if ( isAtStartBoundary( position, attribute ) ) {
+				preventCaretMovement( data );
+				overrideGravity( model );
+
 				return;
 			}
 
-			// If caret sticks to the bound of Text with attribute it means that we are going to
-			// enter `foo{}<a>bar</a>biz` or leave `foo<a>bar{}</a>biz` the text with attribute.
-			if ( isAtAttributeBoundary( position.nodeAfter, position.nodeBefore, attribute ) ) {
-				// So we need to prevent caret from being moved.
-				data.preventDefault();
-				// And override default selection gravity.
-				model.change( writer => writer.overrideSelectionGravity() );
+			// ENGAGE 2-SCM when leaving an attribute:
+			//
+			//		<paragraph>foo<$text attribute>bar{}</$text>baz</paragraph>
+			//
+			if ( isAtEndBoundary( position, attribute ) && modelSelection.hasAttribute( attribute ) ) {
+				preventCaretMovement( data );
+				overrideGravity( model );
 			}
+		}
+	}
 
-		// Moving left.
+	function handleArrowLeftPress( position, data ) {
+		// When the gravity is already overridden...
+		if ( overrideUid ) {
+			// ENGAGE 2-SCM & REMOVE SELECTION ATTRIBUTE
+			// when about to leave one attribute value and enter another:
+			//
+			// 		<paragraph><$text attribute="1">foo</$text><$text attribute="2">{}bar</$text></paragraph>
+			//
+			// but DON'T when already in between of them (no attribute selection):
+			//
+			// 		<paragraph><$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text></paragraph>
+			//
+			if ( isBetweenDifferentValues( position, attribute ) && modelSelection.hasAttribute( attribute ) ) {
+				preventCaretMovement( data );
+				restoreGravity( model );
+				removeSelectionAttribute( model );
+			}
+
+			// ENGAGE 2-SCM when at any boundary of the attribute:
+			//
+			// 		<paragraph>foo<$text attribute>bar</$text>{}baz</paragraph>
+			// 		<paragraph>foo<$text attribute>{}bar</$text>baz</paragraph>
+			//
+			else {
+				preventCaretMovement( data );
+				restoreGravity( model );
+
+				// REMOVE SELECTION ATRIBUTE at the beginning of the block.
+				// It's like restoring gravity but towards a non-existent content when
+				// the gravity is overridden:
+				//
+				// 		<paragraph><$text attribute>{}bar</$text></paragraph>
+				//
+				// becomes:
+				//
+				// 		<paragraph>{}<$text attribute>bar</$text></paragraph>
+				//
+				if ( position.isAtStart ) {
+					removeSelectionAttribute( model );
+				}
+			}
 		} else {
-			// If caret sticks to the bound of Text with attribute and gravity is already overridden it means that
-			// we are going to enter `foo<a>bar</a>{}biz` or leave `foo<a>{}bar</a>biz` text with attribute.
-			if ( modelSelection.isGravityOverridden && isAtAttributeBoundary( position.nodeBefore, position.nodeAfter, attribute ) ) {
-				// So we need to prevent cater from being moved.
-				data.preventDefault();
-				// And restore the gravity.
-				model.change( writer => writer.restoreSelectionGravity() );
+			// ENGAGE 2-SCM when between two different attribute values but selection has no attribute:
+			//
+			// 		<paragraph><$text attribute="1">foo</$text>{}<$text attribute="2">bar</$text></paragraph>
+			//
+			if ( isBetweenDifferentValues( position, attribute ) && !modelSelection.hasAttribute( attribute ) ) {
+				preventCaretMovement( data );
+				setSelectionAttributeFromTheNodeBefore( model, position );
 
 				return;
 			}
 
-			// If we are here we need to check if caret is a one character before the text with attribute bound
-			// `foo<a>bar</a>b{}iz` or `foo<a>b{}ar</a>biz`.
-			const nextPosition = position.getShiftedBy( -1 );
+			// DON'T ENGAGE 2-SCM if gravity is already overridden. It means that we have already entered
+			//
+			// 		<paragraph><$text attribute>bar{}</$text></paragraph>
+			//
+			if ( position.isAtEnd && isAtBoundary( position, attribute ) ) {
+				if ( modelSelection.hasAttribute( attribute ) ) {
+					return;
+				} else {
+					preventCaretMovement( data );
+					setSelectionAttributeFromTheNodeBefore( model, position );
+
+					return;
+				}
+			}
+
+			// REMOVE SELECTION ATRIBUTE when restoring gravity towards a non-existent content at the
+			// beginning of the block.
+			//
+			// 		<paragraph>{}<$text attribute>bar</$text></paragraph>
+			//
+			if ( position.isAtStart && isAtBoundary( position, attribute ) ) {
+				if ( modelSelection.hasAttribute( attribute ) ) {
+					removeSelectionAttribute( model );
+
+					return;
+				}
 
-			// When position is the same it means that parent bound has been reached.
-			if ( !nextPosition.isBefore( position ) ) {
 				return;
 			}
 
-			// When caret is going stick to the bound of Text with attribute after movement then we need to override
-			// the gravity before the move. But we need to do it in a custom way otherwise `selection#change:range`
-			// event following the overriding will restore the gravity.
-			if ( isAtAttributeBoundary( nextPosition.nodeBefore, nextPosition.nodeAfter, attribute ) ) {
-				model.change( writer => {
-					let counter = 0;
-
-					// So let's override the gravity.
-					writer.overrideSelectionGravity( true );
-
-					// But skip the following `change:range` event and restore the gravity on the next one.
-					emitter.listenTo( modelSelection, 'change:range', ( evt, data ) => {
-						if ( counter++ && data.directChange ) {
-							writer.restoreSelectionGravity();
-							evt.off();
-						}
-					} );
-				} );
+			// DON'T ENGAGE 2-SCM when about to enter of leave an attribute.
+			// We need to check if the caret is a one position before the attribute boundary:
+			//
+			// 		<paragraph>foo<$text attribute>b{}ar</$text>baz</paragraph>
+			// 		<paragraph>foo<$text attribute>bar</$text>b{}az</paragraph>
+			//
+			if ( isAtBoundary( position.getShiftedBy( -1 ), attribute ) ) {
+				// Skip the automatic gravity restore upon the next selection#change:range event.
+				// If not skipped, it would automatically restore the gravity, which should remain
+				// overridden.
+				skipNextRangeChange();
+				overrideGravity( model );
 			}
 		}
-	} );
+	}
+
+	function overrideGravity( model ) {
+		overrideUid = model.change( writer => writer.overrideSelectionGravity() );
+	}
+
+	function restoreGravity( model ) {
+		model.change( writer => {
+			writer.restoreSelectionGravity( overrideUid );
+			overrideUid = null;
+		} );
+	}
+
+	function preventCaretMovement( data ) {
+		data.preventDefault();
+	}
+
+	function removeSelectionAttribute( model ) {
+		model.change( writer => {
+			writer.removeSelectionAttribute( attribute );
+		} );
+	}
+
+	function setSelectionAttributeFromTheNodeBefore( model, position ) {
+		model.change( writer => {
+			writer.setSelectionAttribute( attribute, position.nodeBefore.getAttribute( attribute ) );
+		} );
+	}
+
+	function skipNextRangeChange() {
+		skipNextChangeRange = true;
+	}
 }
 
-// @param {module:engine/model/node~Node} nextNode Node before the position.
-// @param {module:engine/model/node~Node} prevNode Node after the position.
+// @param {module:engine/model/position~Position} position
 // @param {String} attribute Attribute name.
-// @returns {Boolean} `true` when position between the nodes sticks to the bound of text with given attribute.
-function isAtAttributeBoundary( nextNode, prevNode, attribute ) {
+function isAtStartBoundary( position, attribute ) {
+	const prevNode = position.nodeBefore;
+	const nextNode = position.nodeAfter;
 	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
 	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;
 
-	if ( isAttrInNext && isAttrInPrev && nextNode.getAttributeKeys( attribute ) !== prevNode.getAttribute( attribute ) ) {
+	if ( ( !isAttrInPrev && isAttrInNext ) || isBetweenDifferentValues( position, attribute ) ) {
 		return true;
 	}
 
-	return isAttrInNext && !isAttrInPrev || !isAttrInNext && isAttrInPrev;
+	return false;
+}
+
+// @param {module:engine/model/position~Position} position
+// @param {String} attribute Attribute name.
+function isAtEndBoundary( position, attribute ) {
+	const prevNode = position.nodeBefore;
+	const nextNode = position.nodeAfter;
+	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
+	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;
+
+	if ( ( isAttrInPrev && !isAttrInNext ) || isBetweenDifferentValues( position, attribute ) ) {
+		return true;
+	}
+
+	return false;
+}
+
+// @param {module:engine/model/position~Position} position
+// @param {String} attribute Attribute name.
+function isBetweenDifferentValues( position, attribute ) {
+	const prevNode = position.nodeBefore;
+	const nextNode = position.nodeAfter;
+	const isAttrInNext = nextNode ? nextNode.hasAttribute( attribute ) : false;
+	const isAttrInPrev = prevNode ? prevNode.hasAttribute( attribute ) : false;
+
+	if ( !isAttrInPrev || !isAttrInNext ) {
+		return;
+	}
+
+	return nextNode.getAttribute( attribute ) !== prevNode.getAttribute( attribute );
+}
+
+// @param {module:engine/model/position~Position} position
+// @param {String} attribute Attribute name.
+// @returns {Boolean} `true` when position between the nodes sticks to the bound of text with given attribute.
+function isAtBoundary( position, attribute ) {
+	return isAtStartBoundary( position, attribute ) || isAtEndBoundary( position, attribute );
 }

+ 256 - 38
packages/ckeditor5-engine/tests/utils/bindtwostepcarettoattribute.js

@@ -10,7 +10,6 @@ import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin';
 import bindTwoStepCaretToAttribute from '../../src/utils/bindtwostepcarettoattribute';
 import { upcastElementToAttribute } from '../../src/conversion/upcast-converters';
 import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
-
 import { setData } from '../../src/dev-utils/model';
 
 describe( 'bindTwoStepCaretToAttribute()', () => {
@@ -44,6 +43,26 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 	} );
 
 	describe( 'moving right', () => {
+		it( 'should do nothing for unrelated attribute (at the beginning)', () => {
+			setData( model, '[]<$text c="true">foo</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+			] );
+		} );
+
+		it( 'should do nothing for unrelated attribute (at the end)', () => {
+			setData( model, '<$text c="true">foo[]</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+			] );
+		} );
+
 		it( 'should "enter" the text with attribute in two steps', () => {
 			setData( model, '<$text c="true">foo[]</$text><$text a="true" b="true">bar</$text>' );
 
@@ -73,35 +92,94 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 			] );
 		} );
 
-		it( 'should do nothing for not bound attribute (at the beginning)', () => {
-			setData( model, '[]<$text c="true">foo</$text>' );
+		it( 'should use two-steps movement when between nodes with the same attribute but different value', () => {
+			setData( model, '<$text a="1">bar[]</$text><$text a="2">foo</$text>' );
 
 			testTwoStepCaretMovement( [
-				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
 				'→',
-				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				// <$text a="1">bar</$text>[]<$text a="2">foo</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 },
+				'→',
+				// <$text a="1">bar</$text><$text a="2">[]foo</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 2 },
+				'→',
+				// <$text a="1">bar</$text><$text a="2">f[]oo</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2 }
 			] );
 		} );
 
-		it( 'should do nothing for not bound attribute (at the end)', () => {
-			setData( model, '<$text c="true">foo[]</$text>' );
+		// https://github.com/ckeditor/ckeditor5/issues/937
+		it( 'should not require two-steps between unrelated attributes inside the initial attribute', () => {
+			setData( model, '<$text a="1">fo[]o</$text><$text a="1" b="2">bar</$text><$text a="1">baz</$text>' );
 
 			testTwoStepCaretMovement( [
-				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
 				'→',
-				{ selectionAttributes: [ 'c' ], isGravityOverridden: false, preventDefault: 0 },
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 }
 			] );
 		} );
 
-		it( 'should require two-steps movement when caret goes between text node with the same attribute but different value', () => {
-			setData( model, '<$text a="1">bar[]</$text><$text a="2">foo</$text>' );
+		it( 'should handle passing through the only character in the block', () => {
+			setData( model, '[]<$text a="1">x</$text>' );
 
 			testTwoStepCaretMovement( [
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
 				'→',
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 },
+				'→',
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 }
+			] );
+		} );
+
+		it( 'should handle passing through the only character in the block (no attribute in the initial selection)', () => {
+			setData( model, '[]<$text a="1">x</$text>' );
+
+			model.change( writer => writer.removeSelectionAttribute( 'a' ) );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'→',
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 1 },
 				'→',
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1 },
+				'→',
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 2 },
+				'→',
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 2 }
+			] );
+		} );
+
+		it( 'should handle passing through the only-child with an attribute (multiple characters)', () => {
+			setData( model, '[]<$text a="1">xyz</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// <$text a="1">x{}yz</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// <$text a="1">xy{}z</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// <$text a="1">xyz{}</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// <$text a="1">xyz</$text>{}
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 },
+				'→',
+				// <$text a="1">xyz</$text>{}
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 }
 			] );
 		} );
 	} );
@@ -139,7 +217,7 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 			] );
 		} );
 
-		it( 'should do nothing for not bound attribute (at the beginning)', () => {
+		it( 'should do nothing for unrelated attribute (at the beginning)', () => {
 			setData( model, '<$text c="true">[]foo</$text>' );
 
 			testTwoStepCaretMovement( [
@@ -151,7 +229,7 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 			] );
 		} );
 
-		it( 'should do nothing for not bound attribute (at the end)', () => {
+		it( 'should do nothing for unrelated attribute (at the end)', () => {
 			setData( model, '<$text c="true">foo</$text>[]' );
 
 			testTwoStepCaretMovement( [
@@ -179,11 +257,143 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 			testTwoStepCaretMovement( [
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
 				'←',
+				// <$text a="2">foo</$text><$text a="1">[]bar</$text>
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 0 },
 				'←',
+				// <$text a="2">foo</$text>[]<$text a="1">bar</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 },
+				'←',
+				// <$text a="2">foo[]</$text><$text a="1">bar</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2 },
+				'←',
+				// <$text a="2">fo[]o</$text><$text a="1">bar</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2 }
+			] );
+		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/937
+		it( 'should not require two-steps between unrelated attributes inside the initial attribute', () => {
+			setData( model, '<$text a="1">foo</$text><$text a="1" b="2">bar</$text><$text a="1">b[]az</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				{ selectionAttributes: [ 'a', 'b' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 }
+			] );
+		} );
+
+		it( 'should handle passing through the only-child with an attribute (single character)', () => {
+			setData( model, '<$text a="1">x</$text>[]' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// <$text a="1">{}x</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// {}<$text a="1">x</$text> (because it's a first-child)
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// {}<$text a="1">x</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 }
+			] );
+		} );
+
+		it( 'should handle passing through the only character in the block (no attribute in the initial selection)', () => {
+			setData( model, '<$text a="1">x</$text>[]' );
+
+			model.change( writer => writer.removeSelectionAttribute( 'a' ) );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1 },
+				'←',
 				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1 },
 				'←',
-				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1 }
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 },
+				'←',
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 }
+			] );
+		} );
+
+		it( 'should handle passing through the only-child with an attribute (multiple characters)', () => {
+			setData( model, '<$text a="1">xyz</$text>[]' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// <$text a="1">xy{}z</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// <$text a="1">x{}yz</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// <$text a="1">{}xyz</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 0 },
+				'←',
+				// {}<$text a="1">xyz</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 },
+				'←',
+				// {}<$text a="1">xyz</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 }
+			] );
+		} );
+	} );
+
+	describe( 'moving and typing around the attribute', () => {
+		it( 'should handle typing after the attribute', () => {
+			setData( model, '<$text a="1">x[]</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'y',
+				// <$text a="1">xy[]</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// <$text a="1">xy</$text>[]
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 },
+				'z',
+				// <$text a="1">xy</$text>z[]
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 1 },
+				'←',
+				// <$text a="1">xy</$text>[]z
+				{ selectionAttributes: [], isGravityOverridden: true, preventDefault: 1 },
+				'←',
+				// <$text a="1">xy[]</$text>z
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2 },
+				'w',
+				// <$text a="1">xyw[]</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 2 },
+			] );
+		} );
+
+		it( 'should handle typing before the attribute', () => {
+			setData( model, '<$text a="1">[]x</$text>' );
+
+			testTwoStepCaretMovement( [
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 0 },
+				'←',
+				// []<$text a="1">x</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'z',
+				// z[]<$text a="1">x</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'x',
+				// zx[]<$text a="1">x</$text>
+				{ selectionAttributes: [], isGravityOverridden: false, preventDefault: 0 },
+				'→',
+				// zx<$text a="1">[]x</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: true, preventDefault: 1 },
+				'a',
+				// zx<$text a="1">a[]x</$text>
+				{ selectionAttributes: [ 'a' ], isGravityOverridden: false, preventDefault: 1 },
 			] );
 		} );
 	} );
@@ -269,42 +479,50 @@ describe( 'bindTwoStepCaretToAttribute()', () => {
 	function testTwoStepCaretMovement( scenario ) {
 		for ( const step of scenario ) {
 			if ( typeof step == 'string' ) {
-				let preventDefaultCalled;
+				// An arrow key pressed. Fire the view event and update the model selection.
+				if ( keyMap[ step ] ) {
+					let preventDefaultCalled;
 
-				fireKeyDownEvent( {
-					keyCode: keyCodes[ keyMap[ step ] ],
-					preventDefault: () => {
-						preventDefaultSpy();
+					fireKeyDownEvent( {
+						keyCode: keyCodes[ keyMap[ step ] ],
+						preventDefault: () => {
+							preventDefaultSpy();
 
-						preventDefaultCalled = true;
-					}
-				} );
+							preventDefaultCalled = true;
+						}
+					} );
 
-				if ( !preventDefaultCalled ) {
 					const position = selection.getFirstPosition();
-					let shift;
-
-					if ( step == '→' ) {
-						if ( position.isAtEnd ) {
-							return;
-						}
 
-						shift = 1;
-					} else if ( step == '←' ) {
-						if ( position.isAtStart ) {
-							return;
+					if ( !preventDefaultCalled ) {
+						if ( step == '→' ) {
+							if ( !position.isAtEnd ) {
+								model.change( writer => {
+									writer.setSelection( selection.getFirstPosition().getShiftedBy( 1 ) );
+								} );
+							}
+						} else if ( step == '←' ) {
+							if ( !position.isAtStart ) {
+								model.change( writer => {
+									writer.setSelection( selection.getFirstPosition().getShiftedBy( -1 ) );
+								} );
+							}
 						}
-
-						shift = -1;
 					}
+				}
 
+				// A regular key pressed. Type some text in the model.
+				else {
 					model.change( writer => {
-						writer.setSelection( selection.getFirstPosition().getShiftedBy( shift ) );
+						writer.insertText( step, selection.getAttributes(), selection.getFirstPosition() );
 					} );
 				}
-			} else {
+			}
+
+			// If not a key, then it's an assertion.
+			else {
 				const stepIndex = scenario.indexOf( step );
-				const stepString = `in step #${ stepIndex } (${ JSON.stringify( step ) })`;
+				const stepString = `in step #${ stepIndex } ${ JSON.stringify( step ) }`;
 
 				expect( getSelectionAttributesArray( selection ) ).to.have.members( step.selectionAttributes, '#attributes ' + stepString );
 				expect( selection.isGravityOverridden ).to.equal( step.isGravityOverridden, '#isGravityOverridden ' + stepString );