[SuperEditor][AttributedText] Fix placeholder breaking attribution behavior (Resolves #2565)

[angelosilvestre]

[SuperEditor][AttributedText] Fix placeholder breaking attribution behavior (Resolves #2565)

Inserting an empty text with a placeholder, with an attribution around it, causes the editor to crash when trying to type.

════════ Exception caught by services library ══════════════════════════════════
The following _Exception was thrown during method call TextInputClient.updateEditingStateWithDeltas:
Exception: Another AttributedSpans can only be appended after the final marker in this AttributedSpans. Final marker: [SpanMarker] - attribution: [NamedAttribution]: bold, offset: 1, type: SpanMarkerType.end

When the exception was thrown, this was the stack:
#0      AttributedSpans.addAt (package:attributed_text/src/attributed_spans.dart:714:7)
#1      AttributedText.copyAndAppend (package:attributed_text/src/attributed_text.dart:510:21)
#2      AttributedText.insertString (package:attributed_text/src/attributed_text.dart:560:22)
#3      InsertTextCommand.execute (package:super_editor/src/default_editor/text.dart:2007:27)
#4      _DocumentEditorCommandExecutor.executeCommand (package:super_editor/src/core/editor.dart:701:15)
#5      Editor._executeCommand (package:super_editor/src/core/editor.dart:304:22)
#6      Editor.execute (package:super_editor/src/core/editor.dart:266:30)
#7      TextDeltasDocumentEditor._insertPlainText (package:super_editor/src/default_editor/document_ime/document_delta_editing.dart:331:12)
#8      TextDeltasDocumentEditor.insert (package:super_editor/src/default_editor/document_ime/document_delta_editing.dart:283:23)
#9      TextDeltasDocumentEditor._applyInsertion (package:super_editor/src/default_editor/document_ime/document_delta_editing.dart:180:5)
#10     TextDeltasDocumentEditor.applyDeltas (package:super_editor/src/default_editor/document_ime/document_delta_editing.dart:81:9)
#11     DocumentImeInputClient.updateEditingValueWithDeltas (package:super_editor/src/default_editor/document_ime/document_ime_communication.dart:232:30)
#12     TextInput._handleTextInputInvocation (package:flutter/src/services/text_input.dart:2005:63)
#13     TextInput._loudlyHandleTextInputInvocation (package:flutter/src/services/text_input.dart:1874:20)
#14     MethodChannel._handleAsMethodCall (package:flutter/src/services/platform_channel.dart:611:55)
#15     MethodChannel.setMethodCallHandler.<anonymous closure> (package:flutter/src/services/platform_channel.dart:601:55)
#16     _DefaultBinaryMessenger.setMessageHandler.<anonymous closure> (package:flutter/src/services/binding.dart:650:35)
#17     _invoke2 (dart:ui/hooks.dart:348:13)
#18     _ChannelCallbackRecord.invoke (dart:ui/channel_buffers.dart:45:5)
#19     _Channel.push (dart:ui/channel_buffers.dart:136:31)
#20     ChannelBuffers.push (dart:ui/channel_buffers.dart:344:17)
#21     PlatformDispatcher._dispatchPlatformMessage (dart:ui/platform_dispatcher.dart:786:22)
#22     _dispatchPlatformMessage (dart:ui/hooks.dart:262:31)

call: MethodCall(TextInputClient.updateEditingStateWithDeltas, [1, {deltas: [{selectionBase: 4, oldText: . , selectionAffinity: TextAffinity.downstream, deltaEnd: 3, deltaText: f, deltaStart: 3, composingExtent: -1, selectionIsDirectional: false, selectionExtent: 4, composingBase: -1}]}])
════════════════════════════════════════════════════════════════════════════════

The issue lives in AttributedText.copyText:

AttributedText copyText(int startOffset, [int? endOffset]) {
    _log.fine('start: $startOffset, end: $endOffset');

    final placeholdersBeforeStartOffset = placeholders.entries.where((entry) => entry.key < startOffset);
    final textStartCopyOffset = startOffset - placeholdersBeforeStartOffset.length;

    final placeholdersAfterStartBeforeEndOffset = placeholders.entries.where(
      (entry) => startOffset <= entry.key && entry.key < (endOffset ?? length),
    );
    final textEndCopyOffset =
        (endOffset ?? length) - placeholdersBeforeStartOffset.length - placeholdersAfterStartBeforeEndOffset.length;

    // Note: -1 because copyText() uses an exclusive `start` and `end` but
    // _copyAttributionRegion() uses an inclusive `start` and `end`.
    final startCopyOffset = startOffset < _text.length ? startOffset : _text.length - 1; <---- this line

    // more code...
}

We are using the text without placeholders to determine the starting offset to copy the spans, but the span marker offsets are based on the text with the placeholders.

The current implementation causes the startCopyOffset to be -1. This causes an issue in AttributedSpans.copyAttributionRegion, in the following code:

// Directly copy every marker that appears within the cut
// region.
_markers //
    .where((marker) => startOffset <= marker.offset && marker.offset <= endOffset!) //
    .forEach((marker) {
  _log.fine('copying "${marker.attribution}" at ${marker.offset} from original AttributionSpans to copy region.');
  cutAttributions.add(marker.copyWith(
    offset: marker.offset - startOffset, <---- this line
  ));
});

The expression marker.offset - startOffset evaluates to 0 - (-1) = 1. The span marker should be at offset 0, and we are changing it to offset 1. This is the cause of the exception.

This PR fixes this issue by computing the start and end offset of the span copy to be based on the text with placeholders.

[angelosilvestre]

The golden test suites are automatically failing due to this error:

This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v3. Learn more: https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/

I already updated the version in #2562, but I can update it here if needed.

[matthew-carroll]

It's not clear to me why we're creating new start and end offsets. Why aren't the existing start and end offsets correct? Also what does "spans" mean?

[angelosilvestre]

It's not clear to me why we're creating new start and end offsets. Why aren't the existing start and end offsets correct?

Because of the comment above, we need to adjust the offsets to make them inclusive. But I do agree that is not clear why the startCopyOffset could be greater than the text length.

Also what does "spans" mean?

Maybe it would be better to call this attributionStartCopyOffset, because this offset is passed to AttributedSpans.copyAttributionRegion to copy the attributions in the range.

[matthew-carroll]

Because of the comment above, we need to adjust the offsets to make them inclusive

Looking closer, was the existing variable just renamed? If so, I'm not sure that it needed a new name. Let me know if I'm missing something.

Could this PR be reduced to simply adding final textWithPlaceholders = toPlainText(); and using that instead of _text?

[angelosilvestre]

Yeah, I was trying to make the variable name more clear, but I reverted the variable rename.

[matthew-carroll]

LGTM