Uploaded image for project: 'XWiki Platform'
  1. XWiki Platform
  2. XWIKI-25071

Concurrent file uploads in the realtime WYSIWYG editor can move the caret or the uploaded file into the other user's paragraph

    XMLWordPrintable

Details

    • Bug
    • Resolution: Unresolved
    • Major
    • None
    • 16.10.0
    • CKEditor, Realtime
    • None
    • Unknown

    Description

      Analysis, text and proposed fixes by Claude Code with Opus 5.5

      Summary

      When two users edit the same page in realtime and one of them receives a remote change while one of their own uploads (drag & drop or paste of a file or image) is in progress, the local user's caret, or the upload widget itself, can end up in the paragraph edited by the other user. Consequences observed:

      • the uploaded file / image is inserted in the other user's paragraph;
      • the upload placeholder is orphaned and stays in the content (cke_widget_uploadfile never goes away);
      • the text typed right after the drop goes to the wrong paragraph, or before the text typed just before the drop.

      The main cause (1. below) is not specific to uploads: it can happen whenever the local user has a widget selected (image, macro, upload) when a remote change that adds content to another paragraph is received.

      Steps to reproduce

      1. Two users edit the same page in realtime; user 1 types "first end" in the first paragraph, user 2 types "second " in a second paragraph.
      2. User 1 drops a file between "first " and "end" and, while it uploads, user 2 drops a file at the end of the second paragraph and types " blue ".
      3. User 1 continues typing / dropping.

      Expected: each user's uploads and text stay where each user put them.
      Actual: sometimes they move to the other paragraph. This is automated by AllIT$NestedRealtimeWYSIWYGEditorIT#dragAndDropFilesAtTheSameTime (failures F4, F5 and F6 of XWIKI-22619); it reproduces much more often locally (about 2 times out of 3 on Chrome) than on CI, because CI is slow enough that each upload usually completes before the next action.

      Analysis

      Locally, with only the test fix from XWIKI-22619, the test fails 10 times out of 15 (Chrome) at waitForUploadsToFinish (F4), waitUntilTextContains("yellow") (F6) or the final assertion (F5) - far worse than on CI, because CI is slow enough that each upload usually settles before the next action.

      All three have the same visible symptom: while an upload is in progress, a remote change moves the local user's caret, or the local user's upload widget itself, into the other user's paragraph. Four independent defects produce it.

      1. CKEditor's hidden selection container skews the DOM diff (main cause)

      When a widget is selected (which is the case right after a file is dropped), CKEditor uses a fake selection and appends a hidden container directly under the editable area:

      <p>first <xwiki-widget-uploadfile .../>end</p><p>second&nbsp;</p><div data-cke-hidden-sel="1" data-cke-temp="1">a widget</div>
      

      This element is not part of the synchronized content, so the local root has one more child than the remote one. DiffDOM's roughlyEqual considers the two paragraphs equal (same child structure, text / widget / text) and findCommonSubsets breaks ties with >=, i.e. prefers the last match. It therefore maps the local first paragraph (which holds the local upload widget) to the remote second paragraph. Patch logged in the browser:

      addElement [0], modifyTextElement [1,0], modifyTextElement [1,2], removeElement [2], removeElement [2]
      

      The paragraph holding the upload widget is reused as the second paragraph, its text is rewritten to "second ..." and the selection correctly follows the reused node, into the wrong paragraph. Without the hidden container the same diff is the expected minimal patch (addElement [1,1], addTextElement [1,2]). Consequences: the uploaded content replaces the wrong placeholder and the original one is orphaned (F4), or the next drop / typed text goes to the other paragraph (F5, F6).

      Fix (ckeditorRealtimeAdapter.js): detach the data-cke-temp children of the editable area while the remote patch is computed and applied, then put them back.

      2. The upload placeholder widget data never matches the upload widget

      The synchronized placeholder <span class="xwiki-widget-placeholder-uploadfile"> is upcast to an upload widget, and CKEditor stores the element classes in the widget data (this.data.classes || this.setData("classes", this.getClasses())). The real upload widget has classes: null. The protected widget value therefore always differs, so every remote change received during an upload contains a modifyAttribute on the local upload widget, which also calls setData on it.

      Fix (xwiki-realtime/plugin.js): remove the placeholder class in upcast.

      3. Path-based selection update: attribute changes invalidate the selection, and more

      Patches._updateRangeBoundary had no default branch, so any attribute, comment or value change returned undefined and silently discarded the whole saved selection (logged as {{Restoring path-based selection: [

      {"collapsed":false}

      ]}}); the text-based fallback then put the caret in the wrong place. Combined with 2. this happened on every remote change during an upload. The same code had further bugs:

      • ancestor checks compared paths as strings ("1/20".startsWith("1/2"));
      • relocateGroup adjusted the index twice - checked exhaustively against DiffDOM's relocation algorithm, the old mapping was wrong in 859 of 1974 cases, the new one in none;
      • removing the node just after a caret offset invalidated the selection.

      Fix (patches.js): handle the other change types as no-ops, compare paths as arrays, compute relocated indexes the way DiffDOM applies them, and only invalidate on removal when the boundary is inside the removed node. A randomized check against the real DiffDOM (a few thousand patches per run, tracking a node through each one) shows no wrong position; the old code needlessly invalidated the selection in about 15% of the cases.

      4. CKEditor's filling character sequence

      On Chrome, CKEditor prefixes the text typed after an inline widget with 7 zero-width spaces and remembers that text node (cke-fillingChar). They are not in the synchronized content, so each remote change removes them with a modifyTextElement before the caret (saved at offset 13 in a 6-character " blue "), which invalidated the selection. Once removed, CKEditor still subtracts 7 from the offset of any range in that node, without checking the characters are still there, and places the caret before the text node when the result is <= 0 - exactly F5 (...blue [[image:...]] green becomes {{...[[image:...]] green blue }}).

      Fix: a text change that only removes characters before the boundary now moves the boundary to the left instead of invalidating it (patches.js), and after a remote patch the adapter makes CKEditor forget the filling character node if the sequence is gone (ckeditorRealtimeAdapter.js).

      Note: placeholder identity is not the fix

      Giving each placeholder a unique data-xwiki-upload-id was tried first and measured (9 and 11 failures out of 15, versus 10 without it): no effect. With distinct identifiers the upload widget still moved, for reason 1 above.

      Validation

      Local Chrome, 15 repetitions per run, all on top of the test fix from XWIKI-22619:

      Variant Failures out of 15
      test fix only (XWIKI-22619) 10
      + 2 and 3 15 - all at the final assertion, " yellow " in the wrong paragraph (reason 1)
      + 1 1
      + 4, text removal only 4 (caret moved by CKEditor, see 4)
      + 4, CKEditor forgets the removed filling characters 0
      all fixes, on Firefox 0

      Remaining known weakness (not fixed)

      DiffDOM's tie-breaking (last match wins among roughly equal siblings) can still map a local paragraph to the wrong remote paragraph whenever sibling counts differ for other reasons, e.g. P("f") -> P("f"), P("second ") is patched as "insert a new first paragraph and rewrite the old one to 'second '". The text-based fallback recovers the caret in that case, but a proper fix belongs in DiffDOM (or in how we call it).

      How to reproduce locally

      Temporarily turn the test into a @RepeatedTest(15) (the environment startup dominates: about 3.5 min per Maven run versus about 20 s per repetition), then:

      LANG=C.UTF-8 xmvn clean install -pl :xwiki-platform-realtime-wysiwyg-test-docker \
        -Dit.test='AllIT$NestedRealtimeWYSIWYGEditorIT#dragAndDropFilesAtTheSameTime' \
        -Dxwiki.test.ui.browser=chrome
      

      Group the failures by the line number of the deepest RealtimeWYSIWYGEditorIT stack frame - several distinct races hide behind this one test. On CI, the RealtimeTestDebugger output (browser console logs plus window.REALTIME_DEBUG) is in the Jenkins console log, not in the archived artifacts.

      Attachments

        Issue Links

          Activity

            People

              MichaelHamann Michael Hamann
              MichaelHamann Michael Hamann
              Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

                Created:
                Updated: