Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional undo step created when editor is blurred while widget focused #3198

Closed
f1ames opened this issue Jun 19, 2019 · 6 comments · Fixed by #3248
Closed

Additional undo step created when editor is blurred while widget focused #3198

f1ames opened this issue Jun 19, 2019 · 6 comments · Fixed by #3248
Assignees
Labels
plugin:embed The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Jun 19, 2019

Type of report

Bug

Provide detailed reproduction steps (if any)

  1. Go to https://codepen.io/f1ames/pen/dBOOrY.
  2. Embed some video (links provided).
  3. Double click embedded video and update its URL via dialog.
  4. Focus embedded widget.
  5. Blur editor by clicking somewhere outside.

Expected result

There is single undo step for video embedding.

Actual result

There are two undo steps for the video.

See #3198 (comment) below for detailed explanation.

Other details

  • Browser: Tested on Chrome
  • OS: macOS
  • CKEditor version: 4.11.4 / 4.10.0
  • Installed CKEditor plugins: standard all preset
@f1ames f1ames added type:bug A bug. status:confirmed An issue confirmed by the development team. plugin:embed The plugin which probably causes the issue. target:minor Any docs related issue that can be merged into a master or major branch. labels Jun 19, 2019
@f1ames
Copy link
Contributor Author

f1ames commented Jun 19, 2019

This issue is a little more fancy. In the steps above I blurred the editor to copy 2nd link and that was the moment when additional undo step was created.

It happens only when widget is focused and editor is blurred. The visible drag handler (has some additional styles) is included in a snapshot created on blur and as this snapshot is different than the previous one it creates additional undo step. Can be tested on https://codepen.io/f1ames/pen/JQbNZq. See (errors in the console are irrelevant for this issue):

embed issue cause

@f1ames f1ames changed the title Additional undo step created when embedded URL is updated Additional undo step created when editor is blurred while widget focused Jun 19, 2019
@wimleers
Copy link
Contributor

wimleers commented Jun 20, 2019

FYI: this was originally reported for a Drupal CKEditor plugin that comes with a Widget: https://www.drupal.org/project/entity_embed/issues/3060245. Thanks @f1ames for investigating this! 🙏

@wimleers
Copy link
Contributor

Why is this not a problem for image2, which also uses a CKEditor Widget? At least on https://ckeditor.com/ckeditor-4/demo/ I cannot reproduce it. Both trigger a dialog. So what's the difference?

@f1ames
Copy link
Contributor Author

f1ames commented Jun 21, 2019

@wimleers it's connected with blurring editor and not dialog itself. It can be reproduced also on demo with image2 plugin (blurring editor adds undo step):

issue undo focus

My assumption is that if dialog somehow blurs the editor (but I think the default dialog logic doesn't do that) the additional undo step will be added.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 21, 2019

Another update to my previous #3198 (comment) - the additional snapshot is created when editor is focused after blurring.

What happens is that when editor is focused (e.g. text clicked somewhere), for a very short time, widget regains fake selection (which triggers drag handler to show/blink) and then fake selection is removed due to selection change. This short widget focus (not 100% sure if it's the focus itself) triggers undo snapshot creation which contains visible drag handler. I confirmed it by locking/unlocking undo manager on widget focus/blur events - the snapshot is not created then.

@f1ames f1ames added this to the 4.13.0 milestone Jul 4, 2019
@engineering-this engineering-this self-assigned this Jul 5, 2019
@engineering-this
Copy link
Contributor

Screenshot 2019-07-05 at 13 17 40

Here is a diff of two snapshots that produces same data.

UndoManager doesn't support any filtering of snapshots. We could fix the issue by introducing new UndoManager.filter class that would allow registering rules to be applied on saved snapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:embed The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. target:minor Any docs related issue that can be merged into a master or major branch. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants