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

[Safari] Reverse typing effect after focus change, e.g., outdent button causing text to insert backwards. #14702

Closed
david-merritt opened this issue Aug 1, 2023 · 17 comments · Fixed by #16289
Labels
browser:safari domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:core Issue to be handled by the Core team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.

Comments

@david-merritt
Copy link

📝 Instructions

  1. Open Safari
  2. Navigate to: https://ckeditor.com/ckeditor-5/demo/feature-rich/
  3. Remove all text from CKEditor input
  4. Click into the CKEditor input
  5. Click the Outdent button
  6. Start typing

✔️ Expected result

Text should input normally

❌ Actual result

The text is input in reverse

📃 Other details

  • Browser: Safari
  • OS: macOS
  • First affected CKEditor version: Latest

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@david-merritt david-merritt added the type:bug This issue reports a buggy (incorrect) behavior. label Aug 1, 2023
@david-merritt david-merritt changed the title Outdent Button Casing Text to Insert Backwards Outdent Button Causing Text to Insert Backwards Aug 1, 2023
@Witoso
Copy link
Member

Witoso commented Aug 1, 2023

I cannot reproduce it, could you share a video?

Screen.Recording.2023-08-01.at.13.40.48.mov

@Witoso Witoso added browser:safari pending:feedback This issue is blocked by necessary feedback. domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). labels Aug 1, 2023
@urbanspr1nter
Copy link
Contributor

@Witoso I believe this may be another case of "backwards" typing. We've also seen this type of behavior in Mac where no performance issue needs to be present for it to reproduce. This may be one of the cases I called out in the bug: #14569 (comment) where I believed there could be additional cases in this platform (Mac).

Internally, we've had a number of users claim similar things where performance issue has not been present (again, Mac!) but I've yet to actually reproduce it myself.

From my general understanding, the CMD+TAB action back into the editable will actually begin to insert text backwards (sticky cursor) -- however, based on my testing with @niegowski 's change, I was unable to reproduce it further. I personally don't use Mac often so it may just be me not being thorough enough.

Could there be another area where things like indent/outdent could affect selection not being updated?

@Witoso
Copy link
Member

Witoso commented Aug 1, 2023

The demos on the page are not updated yet, so there's a chance it will help with this case. @niegowski WDYT about the indent?

@niegowski
Copy link
Contributor

This case is different. While clicking on the disabled button (in Safari) it gets focused but the selection is still in the editable:

2023-08-01 15 22 01

But since editable is not focused, we do not touch the DOM selection so the selection is not updated.

@Witoso
Copy link
Member

Witoso commented Aug 1, 2023

Oh, it's about the outdent 🤦

@urbanspr1nter
Copy link
Contributor

This case is different. While clicking on the disabled button (in Safari) it gets focused but the selection is still in the editable:

2023-08-01 15 22 01

But since editable is not focused, we do not touch the DOM selection so the selection is not updated.

@niegowski Yeah this is something that I get confused about. 😖 This happens on other scenarios too, but the basic trigger is the same. The text box generally looks focused at the moment the action is taken, but it isn't? This seems to be a Mac-only thing...

@niegowski
Copy link
Contributor

Oh, it's about the outdent 🤦

It's about any disabled button in the toolbar.

If the button is disabled we probably should not focus the button on click (but still be focusable by the keyboard). But I'm not sure how it conforms to accessibility rules. cc @Comandeer?

@Witoso Witoso added the squad:core Issue to be handled by the Core team. label Aug 2, 2023
@Comandeer
Copy link
Member

The focusability of disabled controls is about the discoverability of these controls and it's mainly mentioned in the context of a keyboard focus. I would argue that in the case of a mouse user, the discoverability is better achieved in other ways (e.g. hovering the disabled button). Due to that, I'd say that it's safe to disable the focus on clicking and keep enabled only the keyboard one. We've also done it in CKEditor 4 and it worked well.

@Witoso Witoso removed the pending:feedback This issue is blocked by necessary feedback. label Aug 3, 2023
@Witoso Witoso changed the title Outdent Button Causing Text to Insert Backwards [Safari] Reverse typing effect after focus change, e.g., outdent button causing text to insert backwards. Aug 21, 2023
@Witoso
Copy link
Member

Witoso commented Aug 21, 2023

Another case #14830 of a similar behavior.

I wonder @niegowski what could be our actions here:

  1. Don't focus disabled (workaround for the browser's issue)
  2. Could we report somewhere upstream to Safari? WDYT @Reinmar?
  3. Should we "defocus" editable in Safari (is it even possible?) when focus lands outside the editable?

@dtdesign
Copy link

I would like to suggest taking a look at #14830 because I provided a test case that allows one to reproduce the issue using a button element outside that exists outside of the editor’s realm. The disabled toolbar button is just another case of this and (at least there) you could possibly call event.preventDefault() to avoid the focus shift.

In the issue I have explained that document.activeElement in that situation points to an element somewhere else on the page (including the document.body). Maybe you could suppress typing events while the editor is not focused?

@urbanspr1nter
Copy link
Contributor

urbanspr1nter commented Aug 21, 2023

Another case #14830 of a similar behavior.

I wonder @niegowski what could be our actions here:

  1. Don't focus disabled (workaround for the browser's issue)
  2. Could we report somewhere upstream to Safari? WDYT @Reinmar?
  3. Should we "defocus" editable in Safari (is it even possible?) when focus lands outside the editable?

I've run into similar issues like this, and have found that at times focus is ripped away improperly on Mac due to some forced blur event. We did something similar to (3).

This problem causes the DOM selection to "disappear". The editable is still in some weird focused state. How we were able to get around this temporarily was to register a beforeinput event at low priority, and on some insertion of the text, perform some operations to "refocus" by dispatching the focus again against the editable through manual DOM event.

Note, the issue doesn't seem to be limited to just these button clicks, it seems to be a general problem overall where unexpected focus stealing from the editable where it doesn't have a chance to (for lack of better term) clean up some state causes it to go into a reverse typing mode. (As @niegowski mentioned, this issue presented here is different from the other one that was fixed)

@david-merritt
Copy link
Author

Hi I see issue: #14830 was closed. Does this mean that this issue has resolved as well? If so what release will it be deployed in?

@Witoso
Copy link
Member

Witoso commented Sep 25, 2023

Hi, it was closed to not duplicate discussion on multiple issues. This is the main one for this problem, and the discussion about it.

@slotterbackW
Copy link

Just to add another way to reproduce this issue, I stumbled on this one recently.

While using Safari, in the CKEditor classic demo editor, if I click into the table menu and move my mouse around and then close the table menu, text starts being inserted in reverse order.

safari_focus.mov

@michalbilik
Copy link
Contributor

michalbilik commented Dec 7, 2023

Another scenario for the sticky cursor in Safari:

Steps:

  1. Focus the editor.
  2. Click Upload image from computer.
  3. Click ESC or Cancel.

Results:
Sticky cursor and typing in reverse.

Screen.Recording.2023-12-07.at.09.26.11.mov

Notes:
This was working correctly in CKEditor v35.2.0, and started to be an issue in v36.0.0

macOS 14.0
Safari 17.0.0

@michalbilik michalbilik added the type:regression This issue reports a bug that was not present in the previous releases. label Dec 7, 2023
@aldonace-wu aldonace-wu added the support:2 An issue reported by a commercially licensed client. label Apr 15, 2024
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Apr 22, 2024
niegowski added a commit that referenced this issue Apr 25, 2024
Fix (engine): The reverse typing effect should not happen after the focus change. Closes #14702.
@aldonace-wu aldonace-wu added support:1 An issue reported by a commercially licensed client. and removed support:2 An issue reported by a commercially licensed client. labels May 17, 2024
@niegowski
Copy link
Contributor

The work for this issue and others related is completed in the #16289, and we finalized the round of internal (successful) tests. I encourage everyone to test the PR if you have a chance. It should be merged in the following days, and will be a part of the next release.

niegowski added a commit that referenced this issue Jul 3, 2024
Fix (typing, engine): Predictive text should not get doubled while typing. Closes #16106.

Fix (engine): The reverse typing effect should not happen after the focus change. Closes #14702. Thanks, @urbanspr1nter!

Fix (engine, typing): Typing on Android should avoid modifying DOM while composing. Closes #13994. Closes #14707. Closes #13850. Closes #13693. Closes #14567. Closes: #11569.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jul 3, 2024
@CKEditorBot CKEditorBot added this to the iteration 77 milestone Jul 3, 2024
@david-merritt
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser:safari domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). squad:core Issue to be handled by the Core team. support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior. type:regression This issue reports a bug that was not present in the previous releases.
Projects
None yet
Development

Successfully merging a pull request may close this issue.