Fix: Prevented from losing selection attributes between operations - IME. #1730
Conversation
|
Let's be extremely cautious here. I assigned also @scofalik who worked the most on selection attributes. |
| @@ -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. | |||
There was a problem hiding this comment.
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.
src/model/documentselection.js
Outdated
| /** | ||
| * Refreshes selection attributes and markers according to the current position in the model. | ||
| */ | ||
| refreshAttributes() { |
There was a problem hiding this comment.
This should be named refresh().
There was a problem hiding this comment.
Or... different. It doesn't look good when refreshAttributes refreshes more than attributes. Or simply let's not wrap it in a function.
There was a problem hiding this comment.
refresh() sounds ok. I was considering it.
src/model/document.js
Outdated
| @@ -313,6 +313,9 @@ export default class Document { | |||
| this.fire( 'change', writer.batch ); | |||
| } | |||
|
|
|||
| // Refresh selection attributes according to the final position in the model after the change. | |||
There was a problem hiding this comment.
As I understand, the selection attributes are now not refreshed on change at all. Does it mean that the selection might have wrong attributes after last post-fixer changes it?
There was a problem hiding this comment.
Maybe this should be added as a change event callback. First because of that reason, second because I think it is better if there's less selection-related code directly in Document class. We already have a lot of listeners in DocumentSelection. WDYT?
There was a problem hiding this comment.
I moved it to Document class to not split the refreshes between two classes.
src/model/document.js
Outdated
| @@ -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. | |||
| this.selection.refreshAttributes(); | |||
There was a problem hiding this comment.
I don't like hard-coding it like this here but I can't find a better idea :(.
src/model/document.js
Outdated
| @@ -307,15 +307,15 @@ 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(); | |||
There was a problem hiding this comment.
Shouldn't it be both before and after change event?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Or I missed something?
Suggested merge commit message (convention)
Fix: Prevented from losing selection attributes between operations - IME. Closes https://github.com/ckeditor/ckeditor5-typing/issues/188.
Additional information
Related ckeditor/ckeditor5-typing#189.