-
Notifications
You must be signed in to change notification settings - Fork 17
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
Eliminate insertFromComposition, deleteByComposition, and deleteCompositionText #122
Eliminate insertFromComposition, deleteByComposition, and deleteCompositionText #122
Conversation
…deleteCompositionText
Hey, I don't think it would be a good idea to entirely remove them, because:
I do understand that Chromium has some issues with implementing these features. The Chromium implementation is not able to solve the issues that arise from not implementing this part in any other way. However, the beforeinput/input event is still a plus for other uses. Therefore, I would propose to make these events optional instead of removing them entirely. |
Hi @johanneswilm, Sorry for the slow reply. The challenge as I see it isn’t in implementing the events, but in supporting a relocatable selection when composition starts. The goal of these events (per our previous conversations) is to enable an author to avoid having composition modify the portion of their DOM being used as the view for some editor-specific document model. The author is meant to accomplish that goal by relocating selection and focus to some other part of the DOM where the author would like composition to occur, and then, as composition progresses, the author can incorporate the result back into their document model so they can render it themselves as part of their view – presumably back to the same place where selection was when composition first started. Let me know if you think I've stated the goal correctly and understood the way in which you expect the author to use these events. My mental model of how to enable this scenario looks like this: Step 1: During the compositionstart event, the author should move selection to a new editable element (call it the compositionElement) and remember where the selection was before the move (represent this location as a Range called originalCompositionRange). Step 2: For each compositionupdate event, the author should process the data field into their editor-specific document model and render it back into originalCompositionRange. The author can repeatedly do this for every compositionupdate event, each time replacing the contents of the originalCompositionRange with new text. Note that since composition is taking place in the compositionElement and not inside the originalCompositionRange, the author is free to produce whatever markup they want inside the originalCompositionRange, e.g. the author might surround every character with a span. Step 3: During the compositionend event, the author should move selection back to the end of originalCompositionRange and perform other necessary cleanup (like hiding the compositionElement). Note one challenge with step 1 is that it isn’t clear to me how much additional context the author should copy from the original editable element into the compositionElement. On MacOS, the Japanese IME has a reconversion feature that expands the active composition range to the preceding phrase when the user presses left arrow. In order for that feature to work, the phrase to be included in the expansion must be in the compositionElement. Maybe the answer is to advise the author to copy a full paragraph of text? Another challenge with step 1 is that the selection should indicate the text to be reconverted. This happens naturally when the text to reconvert is selected by the user. Reconversion can also be triggered, however, when the selection is collapsed. In this case the IME determines the range to reconvert. Firefox appears to handle this case by setting the selection equal to the reconversion range prior to dispatching the compositionstart event. Some sample code showing the approach can be found here. It almost works in Firefox on Windows - the first character typed in the composition is dropped as a result of moving the selection to the compositionElement, but it’s otherwise functional. The first character issue I believe is unrelated to whether these events are kept or removed. It also almost works in Safari, but to facilitate reconversion and workaround selection not being equal to the reconversion range, I produced another variant here which derives the reconversion range from the first insertCompositionText beforeinput event. This works for Safari on MacOS for all cases except when the user wants to expand the reconversion range using the left arrow. For that scenario to work I further modified the code to this final variant which also handles insertFromComposition and deleteCompositionText by performing the same basic steps: replace the contents of originalCompositionRange with the data from the event. Besides Firefox on Windows and Safari on MacOS, other browsers have more significant problems with all variants.
In the final variant I mention above for Safari there are also side effects on the state of the IME where it is taken out of phrase mode when hitting the left arrow. Based on my observations from these experiments, my intuition is that there’s a long tail of issues to sort through both for the spec and implementations as we discover the sensitivities of various IMEs for different languages and different platforms. When I consider that the EditContext accomplishes the same goal but doesn’t require a complex protocol to deal with focus changes during composition start and can also work for other scenarios like an editor that renders to a canvas – I'm reluctant to invest more in making a relocatable selection work. Could we instead declare that relocating selection during compositionstart is unsupported and remove these superflous events? |
Hey @BoCupp-Microsoft, thanks for experimenting. These events were created after discussions involving tens of subject experts over an extended period of time. Moving the caret somewhere else is just one of multiple possible uses. Another is to leave the composition in its original place but to use the events at the end of the composition to do some editor-related cleanup of the dom. For example, an editor may want to add spans to mark who wrote a particular section of the document or to mark at which time it was written. Spans could also be used for a word-style tracked changes feature. |
For this scenario what prevents authors from using compositionend?
That's fine. I'll stop objecting. Do you want to prepare that pull request and I'll abandon this one? |
In practice, compositionend, at times in combination with a timeout, is what is used in a lot fo editors today as it is one fo few events that is universally available. However, the event does not give enough information to make it clear where in the dom which changes were done. So with the event, one has to trigger something else - for example a dom diffing mechanism. That's what we attempted to get away from by giving the required information to the web dev so cleanup could be doing with diffing.
OK, I can do that. |
@masayuki-nakano, @johanneswilm, @whsieh In this PR I was proposing we eliminate three input events that aren't implemented by Chromium browsers or Firefox. I think the events are unnecessary, but the events are currently implemented in Safari. Johannes asked if we could mark the events optional instead of eliminating them. I said above that I wouldn't object. @whsieh and @masayuki-nakano could you let @johanneswilm and I know if you're happy with that outcome? If so, I think we can cancel the meeting we were planning for this Friday to discuss how to move forward with this spec. If you think we should discuss other options we can continue discussion here and in our ad-hoc meeting of the Web Editing WG this Friday. Thanks for your thoughts! |
Tagging related issue: #115 |
I think that On the other hand, about So basically, I'd be happy if they'd be dropped from the draft because of the backward compatibility. |
The current proposal is that they are not dropped but made optional. Are you ok with that @masayuki-nakano? |
Well, I don't like the unclear state, but it's fine to me (If Chromium would support them, I'd be not happy, but probably we'll follow it). On the other hand, I don't know whether such unclear state makes web developers confused or not. I guess they'd be confused... Probably, the spec should explain the reason in a note. |
Ok, will add a note. |
I'd like to better understand this comment. Why would cancellation only during commit be useful? The DOM is modified after every |
The merit of Note that canceling "composing characters" may make serious a11y issue. E.g., cannot see characters which are required to convert, and composing character should not be filtered because there are some cases that composing characters are not acceptable but final string is acceptable, e.g., converting from CJK names to their emails. Therefore, |
Agree. What I don't agree with is that cancellation of just the final committed text is important. If the author can't stop composition from modifying their DOM for the first N compositionupdate events, then being able to cancel N+1 doesn't seem to matter. The author still needs to deal with cleaning up their DOM and can process the data of the last compositionupdate by either pulling the final composition string from the DOM or from the event's data property. I can't see how being able to cancel just the last event helps. |
Indeed, if web apps so sensitive for DOM mutations, you're right. But I guess some apps may not be so sensitive, but may want to inserting the string by themselves. I assume that canceling the final |
The Web Editing Working Group just discussed The full IRC log of that discussion<Travis> topic: Eliminate insertFromComposition, deleteByComposition, and deleteCompositionText<Travis> github: https://github.com//pull/122 <tilgovi> johanneswilm: There were years of discussions for input events. In the end, it was implemented almost at the same time in WebKit and Chromium. WebKit implemented it the way we concluded, but Blink figured out there were parts that were difficult to implement. They couldn't implement it all. <tilgovi> ... What we decided was to divide the specification into two levels so we could get them to implement part of it. Level 1 would contain the things that the Chromium team could implement and Level 2 would contain the proposal of what we wanted, and what WebKit implemented. <tilgovi> ... Someone figured out that actually Chromium start implementing things from Level 2 anyway. It turned out that a lot of it was possible. Now Chromium is shipping most of Level 2 anyway, with very few exceptions related to IME. <tilgovi> ... Now that they're almost the same, maybe we can scrap Level 1 and just have the Level 2. <tilgovi> ... Bo did some investigation about what's possible and what's not, and suggested let's remove some input events from Safari, and we came to an agreement to mark them as optional in the spec. We merged Level 1 and Level 2 and marked three input events as optional. <Travis> q? <tilgovi> ... Then, Firefox has implemented it in the same way as Chromium. The representative from Firefox said we need to have a note while these are marked as optional, explaining some informative background. That's the agreement we currently have that everyone seems okay with to merge Level 1 and Level 2. <BoCupp> q+ <tilgovi> ... This way, all browsers will be compliant. <Travis> ack BoCupp <tilgovi> BoCupp: There's a lot of differences between browsers for input events. In this one aspect, can we agree on the set of events that should exist? I think we're there with the exception of these three events. Firefox and Chrome do not dispatch these beforeinput events and Safari currently does. The PR is just about can we say that these beforeinput events MAY be dispatched. <tilgovi> johanneswilm: Currently, they are required. What we need consensus on is to mark them optional. You originally proposed to eliminate them? <tilgovi> BoCupp: That's right. But I am okay with the "MAY" keyword in there. <tilgovi> annevk: Why are the events problematic? <tilgovi> BoCupp: The sequence of events removes content from the DOM and then re-inserts it. On the Chromium side that's an unnecessary mutation. It's extra churn in the DOM. To have a special dance around the beginning and end of composition more than to mark that with events does not seem to have particular value. <BoCupp> q+ <snianu> q? <tilgovi> My laptop just shut off without apparent reason, so you're without scribe until I boot back up. <Travis> ack BoCupp <Travis> Note: about 5 minutes left in the meeting. <Travis> tilgovi: thanks! <Travis> scribeNick: Travis <Travis> whsieh: I think there is still value in seeing text being committed at the end of the composition. <Travis> .. note you can move around the "phrase" in some IMEs? <Travis> BoCupp: Yes, but the range is the entire string--not sub-sets. <Travis> .. there's an initial composition range, and as you type things are changing... but the whole string is being replaced in the range. <Travis> johanneswilm: What happens when you hit a buffer limit? <Travis> BoCupp: What if you need to change the active composition range? (Not just for max limits). <Travis> .. the composition ends (and re-starts). <Travis> .. so any one of these ranges are sufficient. <Travis> .. in Firefox, the selection is updated immediately, so you can derive it from the selection (don't need the composition events!) <BoCupp> q? <Travis> .. in closing: I'm OK with saying the events are optional. <Travis> .. masayuki (based on his posts), thinks its ok if these are optional. <Travis> johanneswilm: I think he also wants there to be a note explaining why. <Travis> annevk: Yes, masayuki does represent Firefox on this matter. <Travis> .. Want to ask whsieh (or someone else at Apple) to take a look before a decision is made. <Travis> whsieh: bo, when you want to remove the evnets, would you replace that with a regular insertText? <Travis> BoCupp: There's two beforeinput events that come at the end... one at the front. <Travis> .. I would not dispatch the one before composition starts. <Travis> .. each sequence of compositionupdate would be paired with an insertText... <Travis> .. then the compositionend would be the end. <Travis> .. also proposing removing the event that fires (only during reconversion): deleteByComposition <Travis> BoCupp: would love to have whsieh take a look and see if it could be removed... <Travis> whsieh: we've shipped this for many years--could have compat risk to remove (in Apple ecosystem). <Travis> BoCupp: likewise could be compat issues to add them :-) (in Firefox/Chromium) <Travis> annevk: See value in taking another look. <Travis> whsieh: possible to switch behavior, but we try to avoid it if possible. <Travis> johanneswilm: This was meant, in the end, to help JS editors. <Travis> snianu: EditContext has a lot more flexibility for this (it doesn't update the DOM) <Travis> BoCupp: would like to direct folks attention to an update to EditContext that is out for review. <Travis> https://w3c.github.io/editing/docs/EditContext/index.html |
@BoCupp-Microsoft I don't see us having decided on any action at the meeting. Is that also what you recall from the meeting? What do you propose we do at this stage given that the positions are the ones they are? As far as I can tell, currently it says that the events are required. AFAICT, there does not seem to be consensus for removing them, but there seems to be potential consensus for marking them optional. Is that also your understanding? We cancelled the meeting last week, but if there are still things to clarify, maybe we should need another special meeting? |
The Web Editing Working Group just discussed The full IRC log of that discussion<Travis> topic: pull requests on input-events (they are basically the same thing)<Travis> github: https://github.com//pull/122 <Travis> johanneswilm: want to know if we have more information here... <Travis> .. looking to see if we can mark these as not required in the spec? <Travis> whsieh: to be clear, both alternatives are OK: remove them, or mark them as deprecated in the spec. <Travis> .. may be a precident for similar things marked deprecated but supported by webkit? extendSelection? <Travis> johanneswilm: what is desired? <Travis> BoCupp: If whsieh is fine with removing it (from both spec/webkit), that would be my preference. <Travis> action: land this pull request (that removes these events from the spec); expectation that they may get removed from webkit in the future. |
I think we (WebKit) would be okay with removing support for these three input event types, to align with Gecko and Blink. |
@whsieh OK, can we get confirmation on that that it is definitely OK so that we can go ahead and merge this. |
As someone who develops a browser based structured document editor I am confused and disheartened seeing that We went through the same testing @BoCupp-Microsoft described where we tried to change the selection in response to the compositionstart event, but this (unsurprisingly) turned out to be mired with issues, some of which BoCupp described[1]. We settled on a solution where we allow the composition events to uncontrollably pollute our DOM, after which we rerender the entire section of the document when the composition finishes using the In conclusion: Maybe we're missing something (we well might be, mobile device support is low on our list of priorities, so most of the investigations have been done by a single person: me), but we really feel like these 3 events are crucial additions to the Input Events API. Even if EditContext will provide a solution that we would like to use one day, that shouldn't be a reason to handicap this spec indefinitely. [1] We also investigated an approach where we would consistently keep the selection 'outside' the actually editable document, but this turned out to result in too much unpredictable behavior with different IME solutions (although we're aware that Google Docs was able to get this to work well). |
Cannot retrieve the range to be recomposed with |
@BoCupp-Microsoft Do you have an answer to the above concern? |
@masayuki-nakano Right now out of the subject matter a bit, but the main problem I am aware of is that IME's really do not like it when anybody 'messes' with their composition area. Even if Chrome would correctly provide Still though, this results in a situation where I think a lot of implementations are going to be incorrect, as To be fair, it's all doable and when I wrote the last comment I somehow hadn't noticed that |
@DavidMulder0 Your last paragraph makes it sound like you see less of a problem with this change now than previously. Is that correctly understood, or do you still view removing these three types permanently from the spec as problematic? |
The first beforeinput insertCompositionText should have the range to be reconverted marked before the text is updated. That makes reconversion the same as every other case from the author's perspective: the range of insertCompositionText is indicating what will be replaced. I would also be OK with doing as @masayuki-nakano suggests and making the selection match that range before dispatching compositionstart. We don't currently do that in Chromium, and I'm not sure its necessary given we'll report the same range in the following insertCompositionText event, but I'd be willing to make a change for interoperability. |
@DavidMulder0 could you confirm if insertCompositionText would meet your needs as I've described it being used above? |
The Web Editing Working Group just discussed The full IRC log of that discussion<Travis> Topic: Input events (PR 122)<Travis> github: https://github.com//pull/122 <Travis> johanneswilm: Was going to merge this PR, but wanted to get additional confirmation from the group... <Travis> .. wanted to get implementer confirmation that we aren't removing something important. <Travis> BoCupp: yes, I will respond <Travis> johanneswilm: seems the commenter is actually using these events in their editor, and can't see an alternative. <Travis> BoCupp: assuming, this meant how to get the range for what is going to be converted. The IME decides what those extents are. The first CompositionUpdate is no different from any other, and the range is the same each time... <Travis> .. also masayuki suggested that you could use selection... but this is not implemented in chromium. <Travis> .. by the time the compositionstart fired, firefox will expand the range to encompass the part that will be reconverted. <Travis> .. I think this is unique to firefox. I'm OK with this being sanctioned as good behavior--however, I think it might be unnecessary because insertCompositionText gives you the same thing. <Travis> .. I've been pushing that insertCompositionText be the same (dependable) each time. Hoping that makes it easier vs using the other events. <Travis> .. I will post on the issue regarding this. <Travis> .. Might spin off a separate issue. <Travis> johanneswilm: user claims that there may be a way around it. Bo, please continue discussing to get clarity. If your proposed solution... <Travis> .. to determine the range to be reconverted, one can use getTargetRanges of the first insertCompositionText event... <Travis> .. be filed as a new issue. |
Yes. If native (i.e., OS's) IME API in all platforms works smarter, browsers can do better. However, for example, IME on Windodws handling with TSF may stop working until restarting browser if the composing string is unexpected.
What should happen when IME gets unexpected result depends on IME in any platforms. I.e., out of scope of browsers. Therefore,
For solving such non-smart API, I suggested Basically, while IME has composition string browser nor web app should not do anything which may surprise IME, e.g., sanitizing the composing string, moving to different position. When the composition gets committed, browser and/or web app should put the result into its undo stack. Then, IME features except undoing last commit feature should work. Therefore, I think that the last
IME for TSF on Windows do same thing. First, setting selection to replace, then, insert (replace) text. Therefore I'm surprised at why Blink does not do this on Windows. And IIRC, reconversion on macOS is also setting selection first. So, doing it is reasonable behavior to me... |
@DavidMulder0 @BoCupp-Microsoft @masayuki-nakano Do you feel that we have reached a conclusion here and that this PR can be merged as is, or do we need to modify it? |
The Web Editing Working Group just discussed The full IRC log of that discussion<Travis> Topic: Input events<Travis> github: https://github.com//pull/122 <Travis> johanneswilm: Is there agreement on what to do next? <Travis> .. please follow up BoCupp ! |
The Web Editing Working Group just discussed The full IRC log of that discussion<Travis> Topic: Anything futher on insertFrom... deleteBy.. etc.?<Travis> github: https://github.com//pull/122 <Travis> BoCupp: Are we still exploring other options? <Travis> scribe: Travis <Travis> johanneswilm: Actually... we're on 122 w3c/input-events <Travis> .. is there consensus on this? <Travis> BoCupp: Last status... on 12/10 we were asking DavidMulder... <Travis> johanneswilm: So, you and masayuki are in agreement? <Travis> BoCupp: Yes, I think so. <Travis> johanneswilm: BoCupp could you please confirm that this is ready to merge? <Travis> BoCupp: looked like masayuki was answering david's question... I think earlier in the thread it looked like masayuki would be disappointed if chromium did add these... <Travis> .. will double check, and post to the issue. |
Apologies, I was thinking this was ready to merge because @masayuki-nakano and I were in agreement and I didn't see any objection from @DavidMulder0. When I read back through this thread though I see this comment from @masayuki-nakano that I didn't address:
With this PR (specifically with the removal of The code would look like this: const ce = document.querySelector("[contenteditable]")
let lastRange = null
ce.addEventListener("beforeinput", e => {
if (e.inputType != "insertCompositionText") {
return
}
const ranges = e.getTargetRanges()
if (ranges.length) {
lastRange = ranges[0]
}
})
ce.addEventListener("compositionend", e => {
if (lastRange) {
// StaticRange doesn't have a deleteContents, so convert to live Range
const r = new Range()
r.setStart(lastRange.startContainer, lastRange.startOffset)
r.setEnd(lastRange.endContainer, lastRange.endOffset)
r.deleteContents()
}
}) There's a demo here that logs events so you can clearly see what's happening. I tested it in FireFox with the in-box Japanese IME on Mac and Windows. @masayuki-nakano, let me know if you agree to having the author handle the scenario you raised as I have shown above. If so, then I don't think we need any additional behavior to remove the composed text on the final |
@BoCupp-Microsoft Sorry for the delay to reply, I got lost because of too busy days due to a house moving. And thank you for pinging me, @johanneswilm.
From point of view of editor app developers, I agree with you, however, according to some bug reports to Firefox, some (non-editor apps') web developers want to use
So as far as I've tested, the demo breaks undo/redo behavior at least on both Firefox and Chrome. Therefore, I think that final composition being cancelable is reasonable to me, and it's consistent with the other text insertion On the other hand, this PR is a reasonable size, and including only one purpose, "removing the unnecessary |
Thank you for the feedback @masayuki-nakano. I'll open a separate issue to discuss cancellation for the final composition per this comment and merge this PR:
|
Created this issue to discuss whether the last |
This PR eliminates three inputTypes from beforeinput/input events: insertFromComposition, deleteByComposition, and deleteCompositionText.
It also clarifies the order of the insertCompositionText beforeinput and input events with respect to composition events. The proposed behavior is that beforeinput and input events for insertCompositionText occur only after compositionupdate events.
Additionally, a note stating that multiple compositionend events may occur for one compositionstart event was removed.
The result is that the composition sequence is:
Preview | Diff
Preview | Diff