Disable inline filling character removal during composition #1355
Conversation
|
Just to describe in more details changes in this PR so it may be helpful during testing / looking for regressions. There are two main changes in the Preventing inline filler removal during compositionEvery time composition is started inside element which has inline filler (so basically empty inline element), inline filler will not be removed until composition is finished. Before this change the inline filler was simply removed every time first character was inserted inside such elements. Now if character is inserted by composition, the filler will not be removed. That means the modified behaviour is only observable during composition. The one problem I could think of is that renderer is still not perfect when it comes to handling composition and it tent to break it in some cases (especially when composing inside empty elements). Sometimes in such cases the Adjusting the general behaviour when filler is removedIt is already mentioned in the PR description:
When renderer encounters already rendered text node with content like The previous behaviour assumed that filler in such cases should not be touched because it was left there on purpose. However, such situations will not have place as the filler is removed right after something is inserted in the empty node with filler. So if there is somewhere any case like above in which inline filler should not be touched in a content like |
|
I've noticed one regression. Putting the selection in a link inserts the inline filler. The caret becomes invisible, the link is divided and after typing some text, more After removing the link, GIF - Click. It occurs in all browsers. |
ChromeSteps to reproduce
Current resultThe inline filler has been inserted to the GIF |
src/view/renderer.js
Outdated
| @@ -104,6 +104,13 @@ export default class Renderer { | |||
| */ | |||
| this.isFocused = false; | |||
|
|
|||
| /** | |||
| * Indicates if composition takes places inside view document. | |||
There was a problem hiding this comment.
"Indicates whether text composition takes place in the document."
(you can also update isFocused description)
src/view/renderer.js
Outdated
| @@ -182,7 +189,8 @@ export default class Renderer { | |||
| // There was inline filler rendered in the DOM but it's not | |||
| // at the selection position any more, so we can remove it | |||
| // (cause even if it's needed, it must be placed in another location). | |||
| if ( this._inlineFiller && !this._isSelectionInInlineFiller() ) { | |||
| // Filler should not be touched during composition to not break it. | |||
| if ( this._inlineFiller && !this.isComposing && !this._isSelectionInInlineFillerOnlyNode() ) { | |||
There was a problem hiding this comment.
Could you explain the change from isSelectionInInlineFiller() to isSelectionInInlineFillerOnlyNode()?
There was a problem hiding this comment.
Described it in more detail in comment above - #1355 (comment) in Adjusting the general behaviour when filler is removed section.
| @@ -646,6 +646,80 @@ describe( 'Renderer', () => { | |||
| expect( domSelection.getRangeAt( 0 ).collapsed ).to.be.true; | |||
| } ); | |||
|
|
|||
| it( 'should add and remove inline filler in case <p>foo<b>[]bar</b></p>', () => { | |||
There was a problem hiding this comment.
I feel that these two tests are too long. Can't they be split into more concrete parts?
There was a problem hiding this comment.
E.g. there's just one new test for the whole change with not removing the filler during composition while there are at least two things to check – whether the filler is not touched during composition and that it's removed when it can be removed.
I mean – I know there are some pretty long tests in this file already, but your tests are twice as long :D
There was a problem hiding this comment.
The test is super long, I think I tried to omit some initialization code repetition and merged two checks into one test... I will split it and introduce more test to cover other cases if needed.
|
After some bold/italic buttons clicking, typing and moving the caret around (by clicking) I got two fillers:
And once there are two+ fillers, only one of them is skipped on left/right arrow keys. The other require 7 key presses. |
|
Another interesting observation – sometimes, selecting text from right to left causes adding the filler to every text node in that selection :D But I can't repeat it in a stable manner. Also, to be clear, multiple fillers is not something we certainly need to avoid. However, I wonder whether we shouldn't remove the previous filler if the selection needs a new one. So, to never allow to have two fillers at the same time. WDYT, @f1ames? Could we do that? |
|
BTW, should I notice any composition case getting fixed? Or are all of them still broken because we're re-rendering too big part of the tree? |
The second one, so there are not visible improvements due to how rerendering works now. |
@Mgsy for some reason I am not able to reproduce this issue. Are there any specific steps needed? What I noticed looking at the provided gif is that in you case "link selection span" is inside
When I run similar test locally I see that
Also I don't see inline filler on the provided gif, only span marking link selection. Inline filler is shown in a browser dev tools like
Checked on |
|
I was able to reproduce the 2nd issue (only on windows as on macOS |
I think that would be a valid approach. |
Fortunately, after pulling latest changes I can't reproduce it too 🎉 |
|
I redesigned this solution a bit, so now it handles more cases. In the ideal world, handling inline filler should work like:
However, there are other cases:
The proposed solution handles all above situations:
The only issue here is that if composition is not ended properly (last point above), regular typing in empty inline element will not remove filler immediately (because |
|
I've found another case. Steps to reproduce
Current result
GIF
|
|
@Mgsy I see it's Windows only (e.g. on macOS you cannot change styling during composition). As for step 6., it behaves as described but with Shift + Arrow up for me. Step 5. is correct (well, almost - #1409) and behaves the same without composition. While you are on end of paragraph and after
Step 6. is incorrect as filler should be removed in such cases. The thing is, we assumed that we shouldn't remove inline filler during composition and try to not rerender any part of the text node in which composition takes place. Here when switching off bold we need to render it which breaks composition. I think it will be good to report as separate issue (that you can change styling during composition which breaks it - extracted to ckeditor/ckeditor5#976). Still, I will take a look on reported issue with fillers management if it can be improved. |
You're right, it can't be reproduced on macOS. And of course in the last step I meant Shift + Arrow up :) I'll edit the scenario. |
|
I improved The complexity of this PR (the whole solution basically) is mainly caused by some composition edge cases (as mentioned in #1355 (comment)). So apart from simple cases (composition starts and ends in the same element), it handles extending selection during composition (cases mentioned by @Mgsy) and also deals with not properly ended composition (when Broken composition may occur when element in which composition takes place is rerendered, but also during collaborative editing. So even if we are planning to fix as many cases as possible it still may happen that composition will break at some point and renderer should be able to handle such situations without spoiling editing experience. |
src/view/renderer.js
Outdated
| } else if ( this.isComposing ) { | ||
| // When selection has 0 ranges, `isCollapsed` returns false, but here | ||
| // we are only interested in non-collapsed selection (so with at least 1 range). | ||
| if ( !this.selection.isCollapsed && this.selection.rangeCount > 0 ) { |
There was a problem hiding this comment.
In such cases, to make the code more readable is good to split the code into two steps:
// When selection has 0 ranges, `isCollapsed` returns false, but here
// we are only interested in non-collapsed selection (so with at least 1 range).
const isSelectionNotCollapsed = !this.selection.isCollapsed && this.selection.rangeCount > 0;
// Explain what we do if it's not collapsed and someone's composing.
if ( isSelectionNotCollapsed ) {Now, for an explanation how we handle this case you need to dive into the body of this if() while it's a bit too late. When scanning a code you should not need to go that deep.
src/view/renderer.js
Outdated
| // node (using 'shift + up' in Chrome on Windows during composition). In such situations | ||
| // filler should not be moved or deleted (because it is possible to continue composing). | ||
| inlineFillerPosition = this._getExistingInlineFillerPosition(); | ||
| } else if ( !this._isValidCompositionSelection() ) { |
There was a problem hiding this comment.
This function is used only in this place so it could be made more contextual. Like _checkCompositionWasNotEndedCorrectly().
|
We've just talked with @f1ames that this PR doesn't actually change the behaviour much when typing (the filler is maintained if the mapping worked) because of this: ckeditor5-engine/src/view/renderer.js Lines 321 to 325 in 868d79b It's even reported in https://github.com/ckeditor/ckeditor5-engine/issues/1409. This PR will make filler stay longer if the composition takes place. But if the selection left the text node in which the filler was, this PR will also make sure that the filler is removed (because we might've encountered an incorrectly terminated composition). So, to sum up, the only thing which this PR changes is the fact that the filler will be kept if the composition takes place when the selection is non-empty (on master it's enough that the selection is not empty for the filler to be removed). Therefore, we need to reconsider whether merging this PR makes sense. Perhaps it's enough to keep the current behaviour (keeping the filler as long as the selection stays in the same text node) + extending this behaviour to non empty selection + extending |
|
See also https://github.com/ckeditor/ckeditor5-engine/issues/1342#issuecomment-391422791 which extends the above comment. |
|
It turned out that this change isn't needed because the renderer was designed to not remove the inline filler while the selection is in the same text node anyway. That was buggy but since we fixed it in #1424 it works fine. There's not need for any additional blocking. At least we think so now... |
|
Please do not remove the branch. |
Suggested merge commit message (convention)
Fix: Prevent inline filler removal during composition. Closes ckeditor/ckeditor5#4033. Closes ckeditor/ckeditor5#4340.
Additional information
There is still one issue (ckeditor/ckeditor5#4307) which is not resolved by this fix due to native Chrome bug.
This PR adjusted the way how
inlineFilleris handled in non-empty text nodes. Previously there was an assumption that in nodes likeFILLERtext{}, the filler should not be touched. But there were no real cases producing such situations.Blocking inline filler removal during composition is now such case (and the only we know ATM) so it seems reasonable to just adjust this behaviour, so the filler gets removed also in such situations.