T/430: The BalloonToolbar should not blink when the editor receives focus
#432
Conversation
… event aggregating multiple trigger events.
…its visibility has been settled.
| * it sets the {@link #_isVisibilityTogglePending} flag `true` to notify other pieces of code | ||
| * that debouncing is in progress. | ||
| */ | ||
| this._fireToggleVisibilityDebounced = ( eventName, data ) => { |
There was a problem hiding this comment.
This could be a regular class method instead of an anonymous function assigned to the class property.
| * v v | v | | ||
| * |------------------x------x----------------x-----------------------x----------------x----> [time] | ||
| * |------|----------------| |----------------| | ||
| * <200ms 200ms 200ms |
There was a problem hiding this comment.
Not sure if we need this chart here. It basically shows how debounce is working.
| this.listenTo( this, '_selectionChangeDebounced', () => { | ||
| if ( this.editor.editing.view.document.isFocused ) { | ||
| this._fireToggleVisibilityDebounced( '_selectionChangeDebounced' ); |
There was a problem hiding this comment.
Do you need to define the event name ('_selectionChangeDebounced') in such case? Maybe the parameter should be optional?
There was a problem hiding this comment.
Also, it is not perfectly clear to me why we need to debounce selection change since we already listen on change:range and have debouncing in toggle visibility. So what is missing in the documentation is the answer to 2 question:
- what is the difference between '_selectionChangeDebounced' and 'change:range'?
- why do we need debounce to be used 2 time?
| // Don't reposition the toolbar when awaiting visibility toggle. It may cause unnecessary | ||
| // movement just before it disappears as a result of the toggle. | ||
| if ( this._isVisibilityTogglePending ) { | ||
| this.once( '_toggleVisibilityDebounced', () => { |
There was a problem hiding this comment.
show() method could reposition when ballon is already shown.
This listener won't be necessary then.
| // Hide the toolbar when the selection is changed by a direct change or has changed to collapsed. | ||
| if ( eventName == 'change:range' ) { | ||
| if ( data.directChange || selection.isCollapsed ) { | ||
| this.hide(); |
There was a problem hiding this comment.
What if there are direct selection change and focus change withing 10ms? Since only the last event name is used, there will be no information that direct selection change happen and the panel will not be hidden?
This is a bigger problem, and the reason I am not sure about the eventName parameter. What, in general, should be used as eventName if multiple events happen? If we will use only the first or the last one we will lose some information which might be important. We could use an array, but then guess which of these events should be used might be also very tricky. This is why I think that it would be much better if the event will not say what was the reason for the change. The listener should decide what to do based on the editor state (check if the selection is collapsed and if the editor is focused).
|
@oleq do you want to keep this PR alive? Or could we close it and just keep the branch? I presume you're not planning to get back to this anytime soon. |
|
I can still reproduce it in Letters so the issue is valid. It's unlikely though we're going to schedule some time to work on it anytime soon. On the third hand, I'd hate to lose the code BC I saw there's a lot of rocket science involved (research, ASCII art :P) and this could be a nice starting point in the future. |
Suggested merge commit message (convention)
Type: The
BalloonToolbarshould not blink when the editor receives focus. Closes ckeditor/ckeditor5#5480.Additional information
Requires a small correction in the image tests https://github.com/ckeditor/ckeditor5-image/tree/t/ckeditor5-ui/430.