-
Notifications
You must be signed in to change notification settings - Fork 40
[IME][Safari] Fix IME composistion broken issue in empty paragraph in Safari. #1783
Conversation
Hi, thanks for the updated solution. We will try to check this as soon as possible but it can take a few days due to the holiday season and upcoming release. The trick with pausing rendering in some cases recurs on various occasions so I'd ping @pjasiun to look at this briefly. (About pausing the renderer during text composing) Do you see any problems with that? Collaboration maybe? |
I mark this PR as a sub-PR for: ckeditor/ckeditor5-typing#218 |
I see it only touches the selection, so it could be fine. But since renderer is very tricky this change needs to be tested deeply. |
OK so please take another round of testing of this @Mgsy with both PRs (this & ckeditor/ckeditor5-typing#218) |
Actually, we already did approach it in the past. It's crystal clear that we must not render the selection during composition. There's even a PR which, from its title, was meant to try the same (stop selection rendering): #861. But when I checked the code it actually went much deeper and tried to disable all rendering (which we cannot do). Most likely that's why we never merged it. So, I agree with @pjasiun – stoping rendering selection should be fine, but it requires some more testing. However, I'd actually prefer if we tested first this change alone to see what it fixes/breaks. Then, when we have a better understanding about it, we can see if we need more changes. @farthinker, did you work on these changes (#1783 and ckeditor/ckeditor5-typing#218) separately? I mean – do you know perhaps if the current PR is fixing itself some issues? It's much simpler than the other one and it makes it much easier to close it faster. The one in ckeditor5-typing may take more time because I can see it touches a lot more code. |
@Reinmar I think ckeditor/ckeditor5-typing#218 should be closed in favor of this PR. I've tried to fix the issue in ckeditor5-typing, but it seems impossible. |
fbb00ca
to
82e29ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this PR and it fixes the problem. Also, I didn't find any regression.
@f1ames Sorry for the delay with the review but the topic is not an easy one. So we had to be sure that it will be OK. We'll be merging this soon. @Reinmar I've check the tests and I don't see that we could do any better tests for composition. I'll add some small stylistic changes to the comments. But It's OK for me so I'll be merging this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good 👍.
Fix: The renderer shouold not update DOM selection when document has active composition. Closes #1782. Closes ckeditor/ckeditor5#1333.
See https://github.com/ckeditor/ckeditor5-engine/issues/1782, ckeditor/ckeditor5-typing#218 and ckeditor/ckeditor5#1333 for details about the issue.
This fix make sure DOM selection untouched while composing. I've tested it with several IMEs, and it worked. But I need some help to confirm it won't break something else. @Reinmar @jodator @Mgsy