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

The interaction between composition and updateText/updateSelection should be more clearly defined #111

Open
marijnh opened this issue Nov 15, 2024 · 3 comments

Comments

@marijnh
Copy link

marijnh commented Nov 15, 2024

At the moment, in Chromium's implementation of EditContext, if you push a text/selection update into the edit context while a composition is in progress, further composition updates will continue to happen at the old selection position. See also this issue.

In a situation (which I think is typical of an EditContext-based editor) where the editor library will push changes made to its document via sources other than the edit context into the edit context in order to keep that synced with its own document model, this is going to cause issues.

I would expect updates to the edit context's text/selection model that don't directly impact the text around the composition to move the context's understanding of where the composition happens. Updates that do touch the composed text should probably abort the composition.

The current interface, where updateText and updateSelection are two separate methods, even though the data they interact with is deeply entangled, makes addressing this somewhat awkward, because a state update isn't a single atomic thing, but two imperative calls, with the context being in a bogus state in between them.

I was actually surprised to see that (at least in Chrome's implementation), updateText does not affect selectionStart/selectionEnd—i.e. when the selection is at position 10 and you call updateText(0, 6, ""), it stays at 10 (or is clipped to the end of the document) rather than moving to position 4 along with the text that it points at. Is that an intentional decision, or something that just fell out of the most straightforward implementation? If updateText did adjust the selection (which seems like it will be a welcome behavior in almost every situation), it provides more of an atomic way to push updates into the context, and may make it easier to define composition behavior in response to such an update.

Given all that, my proposal would be:

  • Make updateText affect the selection start and end, moving them by text.length - (rangeEnd - rangeStart) when pos >= rangeEnd, or to rangeStart + text.length when pos > rangeStart && pos < rangeEnd.
  • When a composition is active while updateText is called
    • If the updated range does not overlap with the composed range, move the composition position in the same way
    • If it does overlap, abort the composition
  • When a composition is active while updateSelection is called with a selection that differs from the current selection, the composition should probably be aborted
@dandclark
Copy link
Contributor

I agree that this behavior seems surprising, and your proposed changes are reasonable. I'm not aware of a deliberate decision having been made here, and am assuming that, like you say, the current behavior was just the most straightforward implementation.

If we want to change this behavior the main issue would be that it's a breaking change to the Chromium implementation. Chromium will generally try to avoid changes that might break code in the wild without very strong justification.

In building a justification, it would be good to understand better how hard it is to work around this. For the failure to update the selection position, I assume devs can work around this by just ensuring that after calling updateText they always call updateSelection if needed. But the active composition case looks like it can't really be worked around.

I'm tempted to try to split the difference and only change the behavior in the second and third bullet points: adjusting composition positions if updateText is called during an active composition, or aborting composition if there's an overlap. I assume that the developer impact of this would be smaller, and it's also more measurable; we can add a use counter specifically checking how often updateText and updateSelection are called when there's an active composition.

However, maybe it would be confusing for developers if we change that behavior without also changing the behavior in the first bullet point (updating selection positions when updateText is called).

@marijnh
Copy link
Author

marijnh commented Dec 12, 2024

In building a justification, it would be good to understand better how hard it is to work around this.

It is extremely awkward. If your text model changes through some event that doesn't originate in the edit context during a composition, you cannot touch the edit context to sync it up, because that will cause it to emit unusable events. So you have to somehow accommodate for the fact that your edit context and your actual text model have diverged—and because they can diverge in arbitrary ways, this is very difficult to model. My current workaround consists of some crude, unsound hacks (codemirror/view@9658d5e, codemirror/view@e7d6eeb) because modeling this properly would just be too much added complexity.

If we want to change this behavior the main issue would be that it's a breaking change to the Chromium implementation.

Yes, but only for a case where the current behavior is not very compelling (landing the selection in a more or less arbitrary place after you call updateText). My feeling is that improving this at this relatively early stage is going to be less of a headache than standardizing the existing behavior, and might even accidentally fix code that's currently subtly broken. (But this isn't a deal breaker for me either way, I can work around it by making sure I always follow up with an updateSelection call, if necessary.)

@johanneswilm
Copy link

From meeting minutes (2024-12-12):

RESOLUTION:
we write up changes to the spec, and then put up a spec PR
dandclark to add a non-normative note to the spec noting that changes may be coming to selection offset update behavior

@johanneswilm johanneswilm removed the Agenda+ Queue this item for discussion at the next WG meeting label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants