Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Fix: Prevented from losing selection attributes between operations - IME. #1730

Merged
merged 6 commits into from
Apr 23, 2019
Merged

Fix: Prevented from losing selection attributes between operations - IME. #1730

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
72d6b60
Fix: Prevented from loosing selection attributes between operations -…
oskarwrobel Apr 21, 2019
809f3a0
Be sure selection is up to date on change event.
oskarwrobel Apr 23, 2019
87d79b6
Renamed DocumentSelection#refreshAttributes() to refresh().
oskarwrobel Apr 23, 2019
c7a0c74
Docs.
oskarwrobel Apr 23, 2019
961383f
Refresh selection after change block. Just in case.
oskarwrobel Apr 23, 2019
01fa7f9
Merge branch 'master' into t/ckeditor5-typing/188
oskarwrobel Apr 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/model/document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,9 @@ export default class Document {
if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
this._callPostFixers( writer );

// Refresh selection attributes according to the final position in the model after the change.
this.selection.refreshAttributes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be both before and after change event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, it should be not necessary. Changing the model on change event is not recommended, post fixers are the last who should change the model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or I missed something?


if ( this.differ.hasDataChanges() ) {
this.fire( 'change:data', writer.batch );
} else {
Expand Down Expand Up @@ -391,6 +394,10 @@ export default class Document {

do {
for ( const callback of this._postFixers ) {
// Ensure selection attributes are up to date before each post-fixer.
// https://github.com/ckeditor/ckeditor5-engine/issues/1673.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add here additional comment on why it is done like this and not after every operation so we won't be re-thinking this problem again in a year.

this.selection.refreshAttributes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like hard-coding it like this here but I can't find a better idea :(.


wasFixed = callback( writer );

if ( wasFixed ) {
Expand Down
69 changes: 33 additions & 36 deletions src/model/documentselection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,14 @@ export default class DocumentSelection {
return this._selection.hasAttribute( key );
}

/**
* Refreshes selection attributes and markers according to the current position in the model.
*/
refreshAttributes() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be named refresh().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or... different. It doesn't look good when refreshAttributes refreshes more than attributes. Or simply let's not wrap it in a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refresh() sounds ok. I was considering it.

this._selection._updateMarkers();
this._selection._updateAttributes( false );
}

/**
* Checks whether object is of given type following the convention set by
* {@link module:engine/model/node~Node#is `Node#is()`}.
Expand Down Expand Up @@ -598,6 +606,26 @@ class LiveSelection extends Selection {
// @type {Set}
this._overriddenGravityRegister = new Set();

// Ensure selection is correct after each operation.
this.listenTo( this._model, 'applyOperation', ( evt, args ) => {
const operation = args[ 0 ];

if ( !operation.isDocumentOperation || operation.type == 'marker' || operation.type == 'rename' || operation.type == 'noop' ) {
return;
}

while ( this._fixGraveyardRangesData.length ) {
const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();

this._fixGraveyardSelection( liveRange, sourcePosition );
}

if ( this._hasChangedRange ) {
this._hasChangedRange = false;
this.fire( 'change:range', { directChange: false } );
}
}, { priority: 'lowest' } );

// Ensure selection is correct and up to date after each range change.
this.on( 'change:range', () => {
for ( const range of this.getRanges() ) {
Expand All @@ -615,38 +643,12 @@ class LiveSelection extends Selection {
);
}
}

this._updateMarkers();
this._updateAttributes( false );
} );

// Update markers data stored by the selection after each marker change.
this.listenTo( this._model.markers, 'update', () => this._updateMarkers() );

// Ensure selection is correct and up to date after each operation.
this.listenTo( this._model, 'applyOperation', ( evt, args ) => {
const operation = args[ 0 ];

if ( !operation.isDocumentOperation || operation.type == 'marker' || operation.type == 'rename' || operation.type == 'noop' ) {
return;
}

while ( this._fixGraveyardRangesData.length ) {
const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();

this._fixGraveyardSelection( liveRange, sourcePosition );
}

if ( this._hasChangedRange ) {
this._hasChangedRange = false;
this.fire( 'change:range', { directChange: false } );
}

this._updateMarkers();
this._updateAttributes( false );
}, { priority: 'lowest' } );

// Clear selection attributes from element if no longer empty.
// Ensure selection is up to date after each change block.
this.listenTo( this._document, 'change', ( evt, batch ) => {
clearAttributesStoredInElement( this._model, batch );
} );
Expand Down Expand Up @@ -715,12 +717,12 @@ class LiveSelection extends Selection {

setTo( selectable, optionsOrPlaceOrOffset, options ) {
super.setTo( selectable, optionsOrPlaceOrOffset, options );
this._refreshAttributes();
this._updateAttributes( true );
}

setFocus( itemOrPosition, offset ) {
super.setFocus( itemOrPosition, offset );
this._refreshAttributes();
this._updateAttributes( true );
}

setAttribute( key, value ) {
Expand All @@ -747,7 +749,7 @@ class LiveSelection extends Selection {
this._overriddenGravityRegister.add( overrideUid );

if ( this._overriddenGravityRegister.size === 1 ) {
this._refreshAttributes();
this._updateAttributes( true );
}

return overrideUid;
Expand All @@ -773,15 +775,10 @@ class LiveSelection extends Selection {

// Restore gravity only when all overriding have been restored.
if ( !this.isGravityOverridden ) {
this._refreshAttributes();
this._updateAttributes( true );
}
}

// Removes all attributes from the selection and sets attributes according to the surrounding nodes.
_refreshAttributes() {
this._updateAttributes( true );
}

_popRange() {
this._ranges.pop().detach();
}
Expand Down
49 changes: 44 additions & 5 deletions tests/model/documentselection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,15 +1143,14 @@ describe( 'DocumentSelection', () => {

// https://github.com/ckeditor/ckeditor5-engine/issues/1673
describe( 'refreshing selection data', () => {
it( 'should be up to date when post fixers are called', done => {
it( 'should be up to date before post fixers', () => {
model.schema.extend( '$text', { allowAttributes: 'foo' } );

const p = doc.getRoot().getChild( 0 );

doc.registerPostFixer( () => {
expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker' ] );
done();
} );

model.change( writer => {
Expand All @@ -1169,7 +1168,7 @@ describe( 'DocumentSelection', () => {
} );
} );

it( 'should be up to date when post fixers have changed the data', () => {
it( 'should be up to date between post fixers', () => {
model.schema.extend( '$text', { allowAttributes: 'foo' } );

const p = doc.getRoot().getChild( 0 );
Expand All @@ -1186,6 +1185,11 @@ describe( 'DocumentSelection', () => {
} );
} );

doc.registerPostFixer( () => {
expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
} );

model.change( writer => {
writer.insertText( 'abcdef', { foo: 'bar' }, p );

Expand All @@ -1199,9 +1203,44 @@ describe( 'DocumentSelection', () => {

writer.setSelection( writer.createPositionFromPath( p, [ 3 ] ) );
} );
} );

expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
it( 'should be up to date after post fixers (on `change` event)', done => {
model.schema.extend( '$text', { allowAttributes: 'foo' } );

const p = doc.getRoot().getChild( 0 );

doc.on( 'change', () => {
expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
done();
} );

doc.registerPostFixer( writer => {
writer.setAttribute( 'foo', 'biz', p.getChild( 0 ) );
writer.removeMarker( 'marker-1' );
writer.addMarker( 'marker-2', {
range: writer.createRange(
writer.createPositionFromPath( p, [ 1 ] ),
writer.createPositionFromPath( p, [ 5 ] )
),
usingOperation: false
} );
} );

model.change( writer => {
writer.insertText( 'abcdef', { foo: 'bar' }, p );

writer.addMarker( 'marker-1', {
range: writer.createRange(
writer.createPositionFromPath( p, [ 1 ] ),
writer.createPositionFromPath( p, [ 5 ] )
),
usingOperation: false
} );

writer.setSelection( writer.createPositionFromPath( p, [ 3 ] ) );
} );
} );
} );

Expand Down