i/6053: The gravity of the selection should be overridden if a link was pasted for better typing experience #260
Conversation
| // Before paste: | ||
| // | ||
| // ↱ | ||
| // []<$text linkHref="foo">link</$text> |
There was a problem hiding this comment.
Edit: This is how link works in general. It is OK.
What should happen in a case below:
Go inside link.Paste bolded text.Type.
What happens is that you type with but bold wasn't pasted):
|
@oleq I've found something like this:
Expected: You should type with link. Actual: Only the first character is typed with link. Subsequent are plain text. Additional info: if you change selection to other place in the editor (other link, typing) and go back to the same position as in tested scenario the type with link works as expected. Looks like minor issue to me if not something easy to spot. Edit: After step 1:
After step 2 (gravity is overridden):
After moving selection after other link (gravity is not overridden):
|
|
|
||
| // Filter out the following case where a link is a pasted in the middle (or before) another link | ||
| // (different URLs, so they will not merge). In this (let's say weird) case, we can leave the gravity | ||
| // as it is because it would stick to the first link anyway. |
There was a problem hiding this comment.
I understood this as "it would stick to the first link ('foo') anyway" but the behaviour is that it sticks to the pasted link (also as shown on the sample below). I'd rephrase that comment to remove ambiguity.
| } | ||
|
|
||
| // Skip automatic restore when the change is indirect. | ||
| // It means that e.g. if the change was external (collaboration) and the user had their |
| }, { priority: 'low' } ); | ||
|
|
||
| // Restore the selection gravity automatically as soon as the selection range changes. | ||
| selection.on( 'change:range', ( evt, data ) => { |
There was a problem hiding this comment.
This doesn't work in a case from a comment. The gravity change does not trigger this fixer (the ranges doesn't change) so you can't type with link.
| model: 'bold', | ||
| view: 'b' | ||
| } ); | ||
|
|
||
| model = editor.model; | ||
| view = editor.editing.view; | ||
| } ); | ||
| } ); | ||
|
|
||
| afterEach( () => { | ||
| editor.destroy(); |
There was a problem hiding this comment.
Would be safer to wait for a promise to resolve.
I'm using async/await in test, but can be done with removing an element and then returning the editor.destroy() promise.
| model: 'bold', | ||
| view: 'b' | ||
| } ); | ||
|
|
||
| model = editor.model; | ||
| view = editor.editing.view; | ||
| } ); | ||
| } ); | ||
|
|
||
| afterEach( () => { | ||
| editor.destroy(); |
There was a problem hiding this comment.
Would be safer to wait for a promise to resolve.
I'm using async/await in test, but can be done with removing an element and then returning the editor.destroy() promise.
| @@ -73,8 +87,8 @@ describe( 'LinkEditing', () => { | |||
| } ); | |||
|
|
|||
| it( 'should be bound to th `linkHref` attribute (RTL)', () => { | |||
| return VirtualTestEditor | |||
| .create( { | |||
| return ClassicTestEditor | |||
There was a problem hiding this comment.
Might be a follow-up but AFAICS the main beforeEach() editor was not destroyed and this looks like we could have two editors there.
There was a problem hiding this comment.
Adding element in tests and using ClassicTestEditor might create some memory leaks if multiple editors are created without removing previous one (from global beforeEach() ).
| expect( model.document.selection.isGravityOverridden ).to.be.false; | ||
| } ); | ||
|
|
||
| it( 'should restore gravity on the next change:range', () => { |
There was a problem hiding this comment.
Looks like the test description is too technical ;) and will need to change. I'd go with the expected behaviour like when user move selection, or something similar.
| sinon.assert.notCalled( spy ); | ||
| } ); | ||
|
|
||
| it( 'should not restore the gravity if the change:range was indirect (e.g. comming from the collaboration)', () => { |
There was a problem hiding this comment.
| it( 'should not restore the gravity if the change:range was indirect (e.g. comming from the collaboration)', () => { | |
| it( 'should not restore the gravity if the selection change was indirect (e.g. comming from the collaboration)', () => { |
| @@ -625,9 +824,20 @@ describe( 'LinkEditing', () => { | |||
| } ); | |||
|
|
|||
| describe( 'upcast converter', () => { | |||
| let element, editor; | |||
|
|
|||
| beforeEach( () => { | |||
There was a problem hiding this comment.
Another nested beforeEach might be probably safe as it handles different editors. I don't like that there are two editors on the same page in this tests.
Again, this does not break now, but we need at least a follow-up for this whole file.
|
FYI we went with a better approach #261. |
Suggested merge commit message (convention)
Fix: The gravity of the selection should be overridden if a link was pasted for better typing experience. Closes ckeditor/ckeditor5#6053.