ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

[beforeInput] Consider updating text node values instead of replacing entire text nodes

Open Reinmar opened this issue 1 year ago • 3 comments

It was proposed as a part of #12058 but the change may need to be wider (cover all browsers).

  • First question to answer: Why is this needed on non-Android browsers?
  • Assuming, the answer indicates that we should make that change for all browsers, extract that change from https://github.com/ckeditor/ckeditor5/pull/12446/files and propose a complete PR in this ticket.

Reinmar avatar Sep 12 '22 10:09 Reinmar

  • First question to answer: Why is this needed on non-Android browsers?

We figured out that we were triaging many tickets, also non-Android ones, on the Android PoC which includes this change applied to all browsers. By reverting it for non-Android browsers we might actually regress something compared to a state that we know.

This indicates that it may make more sense to keep this change.

Reinmar avatar Sep 12 '22 10:09 Reinmar

The flow of typing a single letter (not IME, desktop):

  1. beforeInput:insertText native event 
    • default prevented - no text will be inserted to DOM by the browser itself
  2. translated to insertText event
  3. translated to insertText command call
  4. text node inserted to model tree
  5. model changes downcasted to view -> DowncastWriter.insert()
    1. Element._insertChild() is called
      1. fires change:children event
      2. handled as Renderer#markToSync( 'children', ... )
    2. DowncastWriter.mergeAttributes() -> mergeTextNodes()
  6. view changes triggered DOM rendering
  7. paragraph (parent of text) is updated since it was marked-to-sync
  8. the old text node is removed
  9. a new text node is inserted 

In the above scenario, the whole text node is replaced.

The flow of typing a single letter (IME, desktop):

  1. compositionStart native event 
    • renderer is disabled
  2. beforeInput:insertCompositionText native event 
    • can't be default prevented - text will be inserted to DOM by the browser itself
  3. MutationObserver detected changes 
    1. Renderer#markToSync( 'text', ... )
    2. Rendering is still disabled, nothing changes
  4. compositionEnd native event
    1. renderer is enabled
      1. updates text node marked by MutationObserver (reverts changes made by the browser)
    2. triggers insertText 
      1. translated to insertText command call
      2. text node inserted to model tree
      3. model changes downcasted to view -> DowncastWriter.insert()
        1. Element._insertChild() is called
          1. fires change:children event
          2. handled as Renderer#markToSync( 'children', ... )
        2. DowncastWriter.mergeAttributes() -> mergeTextNodes()
  5. view changes triggered DOM rendering
  6. paragraph (parent of text) is updated since it was marked-to-sync
  7. the old text node is removed
  8. a new text node is inserted 

In the above scenario:

  • Browser modifies DOM
  • Editor reverts that changes (updates DOM node)
  • Editor replaces the text node with a new one

niegowski avatar Sep 15 '22 17:09 niegowski

Renderer-related problems:

  • It's too complex. The implementation was never reviewed and cleaned up.
  • We perform superfluous DOM changes in the above mentioned workflows.
  • We have no control over which part of rendering are blocked (no way to tune this for IME and RTC-related scenarios).
  • Disabling rendering leads today to problems with mapping – the editor starts crashing. This should be more robust and safer.
  • And moar. Inline filler? Performance?

Decision for now: It does not make sense to dig deeper into this specific problem right now. The DOM<->View space requires a more thorough analysis and potentially a full redesign.

Reinmar avatar Sep 22 '22 09:09 Reinmar