Handling attributes, ranges and their changes in (Live)Selection. #634
Conversation
…s only one change:range event.
…+ other slight fixes.
…they were specified.
…ed and with additional parameters.
Change log: moved callbacks updating LiveSelection attributes to LiveSelection, fixed events to fire only when range or attribute changed, removed enqueueChanges blocks, fixed attribute overwriting by _updateAttributes, other minor fixes.
| * Returns a default range for this selection. The default range is a collapsed range that starts and ends | ||
| * at the beginning of this selection's document's {@link engine.model.Document#_getDefaultRoot default root}. | ||
| * | ||
| * @protected |
| @@ -313,7 +306,7 @@ export default class Document { | |||
| * @protected | |||
| * @returns {engine.model.RootElement} The default root for this document. | |||
| */ | |||
| _getDefaultRoot() { | |||
| getDefaultRoot() { | |||
| * Keeps mapping of attribute name to priority with which the attribute got modified (added/changed/removed) | ||
| * last time. Possible values of priority are: `'low'` and `'normal'`. | ||
| * | ||
| * Priorities are used by internal `LiveSelection` mechanisms. All attributes set using `LiveSelection` |
There was a problem hiding this comment.
You meant "The 'low' priority is used by..."?
There was a problem hiding this comment.
In fact, what I meant by this comment was to ask for an explanation when low is being used.
There was a problem hiding this comment.
low is being used when attributes are changed indirectly, that is through _updateAttributes.
Really, if you think is is overengineered I'll try to close this "logic" (that _updateAttributes clears only attributes set by itself) in _updateAttributes. IMHO it is over-engineered, but we may resolve this issue in ... spearate issue.
| this.on( 'change:range', ( evt, data ) => { | ||
| if ( data.directChange ) { | ||
| // Reset attributes on selection (clear attributes and priorities). | ||
| //this._resetAttributes(); |
There was a problem hiding this comment.
Actually, it seems to be a commented method call. Didn't you want to introduce this method?
There was a problem hiding this comment.
In a separate comment, which I removed after a while, I wrote that _updateAttributes( true/false ) sounds like two separate methods, cause it isn't clear what it does with that param. It's okayish now, cause it's a priv stuff, but worth giving a short thought.
There was a problem hiding this comment.
Te commented code is a mistake. Sorry for that.
As far as _updateAttributes is concerned, this issue may be resolved by simplifying LiveSelection internal logic and getting rid of priorities. I thought priorities, as a general solution, will be nice, but they got too clunky and even though I have those priorities, I still have to write some special code (like in _updateAttributes).
| @@ -178,6 +178,11 @@ export class SchemaItem { | |||
|
|
|||
| // Now we have to check every item name from the path to check. | |||
| for ( let checkName of checkPath ) { | |||
| // Don't check items that are not registered in schema. | |||
| if ( !this._schema.hasItem( checkName ) ) { | |||
|
I found one regression: https://github.com/ckeditor/ckeditor5-engine/issues/637. |
|
The rest is fine – 100% CC, manual tests pass, etc. #637 needs to be changed in other piece of code, so it's independent. |
|
The other tickets which will be closed or needs to be checked: |
|
I pushed manual test for #603 and checked that it's fully fixed by this PR. Cool! |
|
I've pushed test for Does this mean that this PR is ready to be closed? |
|
Another ticket which seemed to be fixed: https://github.com/ckeditor/ckeditor5-engine/issues/590. |
Fixes ckeditor/ckeditor5#3597. See the issue for the list of changes.
Followup issue: ckeditor/ckeditor5#3847
Related PR: ckeditor/ckeditor5-core#31