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

Spike: Research scope of work of the migration to the beforeInput event. #7728

Closed
jodator opened this issue Jul 28, 2020 · 4 comments
Closed
Assignees
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@jodator
Copy link
Contributor

jodator commented Jul 28, 2020

Provide a description of the task

Research scope of work of the migration to the beforeInput event.

@jodator jodator added the type:task This issue reports a chore (non-production change) and other types of "todos". label Jul 28, 2020
@jodator jodator added this to the iteration 35 milestone Jul 28, 2020
@oleq
Copy link
Member

oleq commented Aug 11, 2020

Research summary

The PoC can be found in: https://github.com/ckeditor/ckeditor5/compare/i/7462-before-input?expand=1.

The full list of beforeInput event types: https://www.w3.org/TR/input-events-2/

General remarks

  • 👎As of today, beforeInput is not supported in Firefox only but IIRC it is implemented internally but not enabled yet as it waits for some blocker issues to be fixed.
  • 🤔We'll need some feature detection for the beforeInput feature. This, for instance, has been researched by React on GitHub. The feature is fairly new and IMO we shouldn't rely on isBeforeInputSupported = !env.isGecko check because in some environments the migration rate to the new browser versions could be low (or even blocked, which often happens in big corps).
  • 🤔Knowing the above, we'll need to branch the code:
    • in Input and Delete we'll need to conditionally enable mutation-based helpers like injectUnsafeKeystrokesHandling( editor ); / injectTypingMutationsHandling( editor );
    • and beforeInput–based helpers that will hopefully spring up as a follow-up of this research.

Formatting 👍

I managed to enable the support for various text formatting using the native text editing toolbar in iOS (bold, italic, underline).

👍It's fairly simple; it comes down to listening to beforeInput with the type of formatBold (formatItalic, formatUnderline...) and executing related editor commands.

Notes and concerns

  • I placed these beforeInput listeners in respective *Editing plugins for maximum granularity. But they could as well land together with other listeners in ckeditor5-typing, though. Executing commands via editor.execute( ... ) does not require dependencies so this is decision has no consequences.
  • 🤔There are plenty of other format* event types that I didn't check, for instance formatJustifyCenter or formatRemove, etc. (see the list in the first section).
    • I couldn't use them on an iPhone (mobile iOS offers only B/I/U) so I didn't test them out.
    • OTOH, they did land in the spec and some devices/OSs/input methods may support them now or in the future.
    • Screen readers and dictation are one of the first things that popped into my head and just for the sake of future compatibility I think we should integrate as many as possible because this comes down to a single editor.execute() and a test (very cheap) even though there's no physical way to see this in action.
  • 🤔Theres are some formatting types like formatFontName or formatBackColor that clearly comes with some data along with them.
    • I didn't test them because I didn't know how but in theory we could handle them.
    • They would probably use some values pre-defined by the OS and we would need to normalize them to the feature configuration of the editor (FontFamily, FontBackgroundColor). Some approximated mapping between font families and color would need to be created.

Enter 👍

I successfully moved the Enter and ShiftEnter implementation to the beforeInput event.

  • 👍For enter, I used the insertParagraph event type.
  • 👍For shift enter, I used the insertLineBreak event type.

Notes and concerns

  • 🤔This should probably be done in the EnterObserver but in the PoC i modified Enter and ShiftEnter implementations instead by adding beforeInput listeners.
  • 🤔Again, just like with the formatting event types, it's up to us to decide whether this should be handled in ckeditor5-enter or in ckeditor5-typing.

Delete 👍

I rewrote the Delete plugin using the beforeInput event and successfully integrated different kinds of deletion depending on the event type.

Notes and concerns

  • I tested Safari on Mac, Chrome on Mac and Safari on iOS and I discovered there are differences when it comes to beforeInput types in certain situations. More of them could come out if I tested on Windows and here are some of them that I noticed (and successfully handled):
    • 👍deleteContent: Only in Safari on Mac when pressing Ctrl+K on a selected widget,
    • 👍deleteContentBackward: Deletes single characters backward, both in Safari and Chrome when pressing Ctrl+H,
    • 👍deleteContentForward: Deletes single characters forward, both in Safari and Chrome when pressing Ctrl+D,
    • 👍deleteWordBackward:
      • On Mac (Safari, Chrome) fired when pressing Option+Backspace,
      • On iOS, fired when holding the backspace for some time text is getting deleted word by word.
    • 👍deleteWordForward: On Mac (Safari, Chrome) when pressing Fn+Option+Backspace,
    • 👍🤔deleteHardLineForward: Works only in Chrome on Mac on Ctrl+K ****despite being listed in the official document about Mac keyboard shortcuts. It should delete everything forward up to the end of the block or the closest line break.
      • To see it in action you have to get rid of the Link plugin which uses the same keystroke to open the link insertion UI. It's a valid use-case IMO, though.
      • I had to write a few lines of code to implement this behavior because editor.execute( 'forwardDelete' ) supports only deleting characters or words. Supposedly, this code should land in the implementation of this command.
      • When there's nothing to delete or the selection is not collapsed I copied the behavior of VSCode which is the same as deleteContentForward.
    • 👎deleteHardLineBackward: I couldn't find a way to trigger this event type.
    • 👍deleteSoftLineBackward: In Chrome and Safari on Mac this happens on Cmd+Backspace
      • I didn't implement it though since this is almost the same as deleteHardLineForward.
      • Anyway, there's this deleteSoft* and deleteHard* thing going on in event types. The former should delete up to the closest visual line break and the latter to the actual <br> or the end of the block. To keep things simple (because otherwise this is a true "Rect fiesta") I'd align handling of these to the "hard" way.
    • 👎deleteEntireSoftLine : I couldn't find a way to trigger this event type.
      • But I guess this should be implemented as "clear the entire block for me" (we don't want the "Rect fiesta" at this moment, remember?).
    • 👎deleteByDrag, deleteByCut: I couldn't find a way to trigger this event type. And I'm not sure how to handle them.

Input 👍🤔

I implemented the stable typing integration in just a few lines. There are some ambiguities when it comes to beforeInput event types but it looks good to me.

Notes and concerns

I tested Safari on Mac, Chrome on Mac and Safari on iOS and I discovered there are differences when it comes to beforeInput types in certain situations. More of them could come out if I tested on Windows and here are some of them that I noticed (and successfully handled):

  • 👍insertText: it works fine in both browsers when simply typing, however:
    • Chrome also uses this event when typing accented letters on Mac (Safari uses insertReplacementText instead)
      • ⚠️ When typing accented letters on Chrome, the first event must be preventDefaulted. Otherwise the second one comes with a wrong target range and the insertion fails.
    • Safari uses this event when accepting spell check suggestions (Chrome uses insertReplacementText instead)
  • 👍insertReplacementText: also works fine but it is fired by different browsers in different situations:
    • Fired by Safari on Mac when accepting an accented letter (the second press after the long press).
    • Chrome fires this event when accepting a spell check suggestion.
  • 🤔there are other insert event types like insertOrderedList or insertHorizontalRule that I didn't test and I don't know how to trigger but I think we should handle them (just like formatting types) because they are cheap to implement
  • 🤔there are some insert event types like insertFromPasteAsQuotation or insertTranspose insertFromYank or insertFromDrop that I don't know what to think about,
  • 🤔we should probably intercept insertLink but I don't know how this should work and I don't know how to test it:
    • this event will either pass an URL along with it (sth. like DomEvent = { data: 'foo', href: '<http://ckeditor.com>' } )
    • or will just have data and the data will be the href (sth. like DomEvent = { data: '<http://ckeditor.com>' } )
  • 🤔insertFromPaste... I tested it and IIRC it overlaps with paste from the ClipboardObserver.
    • So we can either ignore it and allow the ClipboardObserver do its job or we can handle it at the beforeInput level.
    • IMO It's not that important because pasting works OK on all devices.
  • 🤔when insertText type is fired on a non-collapsed range or insertReplacementText is fired in general, they come with a target range the tells where the insertion should occur
    • I used this information (sort of tricky when a fake selection is on and there's no way to map the target DOM range from an event to a model range; I managed it, though) but now I wonder if this was necessary because the selection state is maintained by the editor anyway.
    • OTOH maybe there is some way to fire beforeInput event for a specific selection (target range) in the OS going round the selection state of the editor? Like for instance, some software tells the browser to insert "X" at position "Y" before the editor was even focused, so maybe we should use this target range from the event (DomEvent.getTargetRanges()) after all because it is safer and more versatile?
    • ⚠️OTTH now that I think of it, dropping DomEvent.getTargetRanges() may not be possible. Mostly because Chrome uses insertText for both regular typing and accepting accents. The only way to tell them apart is by looking at the target range (collapsed vs. expanded) so if we used the state of the editor only, that wouldn't work at all.
  • 🤔there are 2 sources of data in the native DOM event: evt.data and evt.dataTransfer
    • I noticed that depending on the Browser/event type combination, the data is in one place or another
    • To keep things simple I used domEvent.data || domEvent.dataTransfer && domEvent.dataTransfer.getData( 'text/plain' ) but it could be that for some events (like insertFromPaste) we may want different data types from the DataTransfer (html is available).

Composition/IME 👎

  • 👍There's a insertCompositionText event correctly fired by both Chrome and Safari.
  • 👎I tried to execute editor.execute( 'input' ) upon this event and it triggered the composition UI in Chrome only for some weird combination of range and resultRange in the options
    • 👎but the composition was broken (happened before the caret) and/or crashed the editor and/or produced invalid results,
    • 👎I tried re-using the community fix that blocks selection changes in the renderer when composing https://github.com/ckeditor/ckeditor5-engine/pull/1783/files but to no avail,
      • 🤔I thought this is gonna be a silver bullet. If the renderer does not set the selection when composing, it shouldn't break the composition, right? But it does.
    • 🤔TBH I'm not sure how this whole chain (user input→model change→view change→renderer) works ATM with mutations and how this is supposed to work after the migration to beforeInput,
      • 🤔I'm not sure how to even debug it. The response of the editor to my editor.execute( 'input' ) is "magical" and I don't understand the processes and the flow of information from this point on up to the DOM selection being changed and the composition compromised,
    • 🤔this whole composition feels like a very fragile thing and it's easy to break it (or not even initiate it correctly in the first place)

Other event types 🤔

  • 🤔historyUndo and historyRedo – I didn't check them but they are available in the iOS native toolbar so they should be handled as well.
    • 🤔I'm nearly sure this comes down to formatBold etc. (see the "Formatting" section) so it's gonna be simple and painless.

cc @Reinmar

@Reinmar
Copy link
Member

Reinmar commented Aug 12, 2020

We discussed with @oleq our approach to composition. We were testing Chrome so far which only fires the insertCompositionText. We know that Firefox will and Safari does already fire also insertFromComposition at the composition end, in Chrome we need to use compositionend and use the previously stored value (text inserted from the composition start moment till now).

There are two strategies for updating the model while the composition takes places. Below, I'm posting our notes. The first option would be optimal as we wouldn't need to rely on DOM diffing which is doh.

HOWEVER, one big assumption here is that during composition the renderer is COMPLETELY switched off. This will most likely cause some issues as the DOM will be desynced with the view for a while (bindings). But there's no other option – while the composition takes place, we must not touch the DOM. Period.

1. Replacing the text inserted from the compositionstart with the current beforeinput 'data'
2. Like mutation obsever - read from the DOM


# 1. 

MODEL: <paragraph>foo[]</paragraph>
VIEW: <p>foo[]</p>
DOM: <p>foo[]</p>

## press "a"
beforeInput data:"a"
MODEL: <paragraph>foo[]</paragraph>
VIEW: <p>foo[]</p>
DOM: <p>fooa[]</p>

initialCaretPosition = store the current model selection position in a live position
previousData = "a"

insert "a" at initialCaretPosition

## press "down"
beforeInput data:"b"
MODEL: <paragraph>fooa[]</paragraph>
VIEW: <p>fooa[]</p>
DOM: <p>foob[]</p>

delete text from initialCaretPosition to previousData.length
insert "b" at initialCaretPosition

## compositionend
(after isComposing was set to false)
editor.editing.view.forceRender()

# 2. 

On compositionupdate (assuming that it's fired after the DOM changed) extract the change from the DOM in a similar way to what we do in the mutation handler, and update the model based on that.

@Reinmar
Copy link
Member

Reinmar commented Aug 12, 2020

HOWEVER, one big assumption here is that during composition the renderer is COMPLETELY switched off. This will most likely cause some issues as the DOM will be desynced with the view for a while (bindings). But there's no other option – while the composition takes place, we must not touch the DOM. Period.

The other option – not changing the model during composition is doable as well, however, it may give a lot worse results. For instance:

  • We'll have to switch off all view observers as they may fire events that will not be applicable to the desynced model. E.g. the selection would move around while you're composing a text, but since the model would not contain that text, the DOM selection cannot be mapped to the view (which is outdated as well).
  • Since the model would not be changing we'd not be able to implement e.g. autocompletion.

@Reinmar Reinmar modified the milestones: iteration 35, iteration 36 Aug 24, 2020
@oleq
Copy link
Member

oleq commented Sep 3, 2020

The PoC phase is over. Moving the implementation to the main issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

3 participants