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

Commit

Permalink
Merge 01fa7f9 into 8c39bdb
Browse files Browse the repository at this point in the history
  • Loading branch information
@oskarwrobel
oskarwrobel committed Apr 23, 2019
2 parents 8c39bdb + 01fa7f9 commit 0e59627
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 41 deletions.
15 changes: 15 additions & 0 deletions src/model/document.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,12 +307,19 @@ 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.refresh();

if ( this.differ.hasDataChanges() ) {
this.fire( 'change:data', writer.batch );
} else {
this.fire( 'change', writer.batch );
}

// Theoretically, it is not necessary to refresh selection after change event because
// post-fixers are the last who should change the model, but just in case...
this.selection.refresh();

this.differ.reset();
}

Expand Down Expand Up @@ -391,6 +398,14 @@ 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.
//
// It might be good to refresh the selection after each operation but at the moment it leads
// to losing attributes for composition or and spell checking
// https://github.com/ckeditor/ckeditor5-typing/issues/188
this.selection.refresh();

wasFixed = callback( writer );

if ( wasFixed ) {
Expand Down
69 changes: 33 additions & 36 deletions src/model/documentselection.js
View file
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.
*/
refresh() {
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
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

0 comments on commit 0e59627

Please sign in to comment.