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

Make targetRanges an attribute which doesn't response to DOM mutations #40

Open
chong-z opened this issue Oct 12, 2016 · 27 comments
Open

Comments

@chong-z
Copy link
Contributor

chong-z commented Oct 12, 2016

Proposal

Define InputEvent as an one-time-use only event, where if event handlers wants to mess up with DOM, they should call preventDefault() and dispatch new 'beforeinput' to describe the change.

For example if spellcheckers wants to modify DOM, they should:

  1. First call preventDefault() and dispatch a new 'beforeinput', describing the change;
  2. Then modify DOM only if it's not cancelled.

In this way we can have StaticRange as an attribute.

Example Code: #40 (comment)


Original Post:

Example 1:

A Sample Text.

When user does the following:

  1. Double-click word 'Sample' to select;
  2. Press delete key.

UA may choose to delete one additional adjacent space (Chrome) or only the selection (FireFox).

In this case:

  1. User's intention is the selection;
  2. UA's default action should include the additional space (if available).

I think getTargetRanges() should follow User's intention and just return the selection, thus JS doesn't have to guess whether it's smart delete or not.

Example 2:

The current status of StaticRange is:
UA keeps a Range instance during event dispatching to track DOM mutation, and getTargetRanges() returns array of dictionary StaticRange at any point.

This example is about tracking selection/caret.

A Sample| Text.

(| is caret)

editor.addEventListener('beforeinput', () => {
  selectFirstCharacter(); // e.g. Select character 'A'.
});

Assuming there are multiple event listeners, and the above JS will run before any other.
When user does the following:

  1. Place caret after 'Sample';
  2. Press backspace key.

For the follow up listeners, should getTargetRanges() return

  1. User's intention aka. 'e';
  2. UA's default actionaka. 'A'?

I think we should still follow User's intention to return 'e' and don't track selection/caret.

@johanneswilm
Copy link
Contributor

Assuming there are multiple event listeners

I still haven't seen a realworld usecase for multiple event listeners on this. The one case that was mentioned at the F2F: having an external spell checker manipulate the DOM without the editor app knowing about its existence, is not a realworld example, as that would break the existing editors anyway. Many of them provide a plugin system, and there are thousands of plugins for some of them, but they all require to register with the editor app and make changes to the DOM within the restraints provided by the given app.

That is not to say that there aren't plugins that have tried to work on the DOM without being registered. And in some cases they may be lucky and a particular editing action may work even though it's not going through the functions provided by the app. But this is pure coincidence and not something anyone should rely on.

Unless there is a real usecase, I would say you should spend as little time as possible on thinking about multiple event listeners. We could maybe agree on the simplest possible solution. For example: once the DOM has been manipulated by JS, the targetranges are empty?

UA may choose to delete one additional adjacent space (Chrome) or only the selection (FireFox).

Ok, but in this case, is it not just a discrepancy between what the US should be doing if it followed platform conventions and what it actually does? If editors that listed to the beforeinput event behave like Firefox, shouldn't that also be the case for editing apps that don't use the beforeinput event?

@rniwa
Copy link

rniwa commented Oct 12, 2016

The one case that was mentioned at the F2F: having an external spell checker manipulate the DOM without the editor app knowing about its existence, is not a real world example, as that would break the existing editors anyway. Many of them provide a plugin system, and there are thousands of plugins for some of them, but they all require to register with the editor app and make changes to the DOM within the restraints provided by the given app.

That is a valid use case if the use case is about allowing arbitrary spellcheckers to be used with any editor without having to use their plugin system.

That is not to say that there aren't plugins that have tried to work on the DOM without being registered. And in some cases they may be lucky and a particular editing action may work even though it's not going through the functions provided by the app. But this is pure coincidence and not something anyone should rely on.

The fact it doesn't work today doesn't mean it's not a valid use case.

@johanneswilm
Copy link
Contributor

That is a valid use case if the use case is about allowing arbitrary spellcheckers to be used with any editor without having to use their plugin system.

Ok, but you are aware that the changes by such a spell checker may end up not being saved, right? So the user sees one thing on their screen, but after hitting the "save" button and reloading the page, the changes may not be there. Or again, it may, if the user is lucky. In the worst case, this external spell checker breaks the editor in a way that an invalid document is saved. It's definitely not a good idea to try to create a spellchecker like that, which I assume is part of a browser extension and auto initiates itself whenever it sees a contenteditable element?

@rniwa
Copy link

rniwa commented Oct 12, 2016

Ok, but you are aware that the changes by such a spell checker may end up not being saved, right? So the user sees one thing on their screen, but after hitting the "save" button and reloading the page, the changes may not be there.

Again, it doesn't really matter whether it works today or not. The question is if there is a scenario in which someone would want to create such a thing. Use cases shape API development, not the other way around. If we only focused on things that already work, we never improve the Web.

For example, if the problem is that editors don't respond to changes made by spellcheckers, one solution here is for these spellcheckers to start dispatching beforeinput and input events instead of directly manipulating DOM, the same way browser would.

@johanneswilm
Copy link
Contributor

I came across this kind of a spell checker a while ago. At first I was delighted that it would work with the editor I was working on. Then I realized it was breaking everything. Googling for how it works with other editors that use contenteditable, I only find bug reports:

http://daviseford.com/node/72

tinymce/tinymce#2577

http://stackoverflow.com/questions/37444906/how-to-stop-extensions-add-ons-like-grammarly-on-contenteditable-editors

I don't think this is something that should be encouraged even further. If we really wanted to add this kind of functionality, it would have to be in a way where this third party spell checker can declare input intentions itself, so that the proper beforeinput events, etc. are called, which the main app then can respond to.

@rniwa
Copy link

rniwa commented Oct 12, 2016

If we really wanted to add this kind of functionality, it would have to be in a way where this third party spell checker can declare input intentions itself, so that the proper beforeinput events, etc. are called, which the main app then can respond to.

That's exactly what we need to figure out.

@johanneswilm
Copy link
Contributor

For example, if the problem is that editors don't respond to changes made by the spellchecker, the correct response here is for these spellcheckers to be dispatching beforeinput and input events instead of directly manipulating DOM, the same way browser would.

Agreed on this point. This is how it should be handled, if we want to support these kinds of plugins.

But even then, in those cases where a plugin manipulates the dom due to some user intent, the main app most likely shouldn't try to act on that intent as well. So the second event listener (the main editor app) should get an empty target range or not get the event at all, or alike. The plugin would then dispatch a new user intent which the main app could listen to.

I am afraid that all this may be a lot more complex and wonder whether it would be ok to wait with supporting such plugins in a version 2?

@rniwa
Copy link

rniwa commented Oct 12, 2016

But even then, in those cases where a plugin manipulates the dom due to some user intent, the main app most likely shouldn't try to act on that intent as well. So the second event listener (the main editor app) should get an empty target range or not get the event at all, or alike. The plugin would then dispatch a new user intent which the main app could listen to.

I think this line of thought is directly relevant in this issue.

I am afraid that all this may be a lot more complex and wonder whether it would be ok to wait with supporting such plugins in a version 2?

I don't think we can avoid this complexity until version 2 because it affects how StaticRange is formed in each event dispatch, and changing its behavior later would not be backwards compatible.

@johanneswilm
Copy link
Contributor

I don't think we can avoid this complexity until version 2 because it affects how StaticRange is formed in each event dispatch, and changing its behavior later would not be backwards compatible.

Ok, so is there anything in the way for us saying that if one event listener manipulates the DOM, the response to all further getTargetRanges is an empty array?

That should be compatible with adding support for user initiated beforeinput events some time in the future.

@rniwa
Copy link

rniwa commented Oct 12, 2016

Ok, so is there anything in the way for us saying that if one event listener manipulates the DOM, the response to all further getTargetRanges is an empty array?

Any DOM mutation anywhere? I don't think that makes sense. Some event listeners might be, for example, updating a toolbar UI somewhere. Detecting which part of DOM has been mutated is quite expensive so I don't think we want to spec that either.

@chong-z
Copy link
Contributor Author

chong-z commented Oct 12, 2016

Can we define InputEvent as an one-time-use only event?

For example if spellcheckers wants to modify DOM, they should:

  1. First call preventDefault() and dispatch a new 'beforeinput', describing the change;
  2. Then modify DOM only if it's not cancelled.

In this way we can have StaticRange as an attribute.

@johanneswilm
Copy link
Contributor

Can we define InputEvent as an one-time-use only event?

That sounds like a good solution to me.

I would then still think it's best to leave the ability to declare new beforeinput events within JavaScript for version 2 for reasons of simplicity, although there is a another direct usecase there that goes beyond plugins:

If a particular editor decides that it wants to wants to change the mapping between user input and intentions. For example one editor decides that hitting a button in the UI should "delete word backward". The problem is -- it's not really easy to figure out what the targetrange of deleting a word backward would be using only JavaScript. If the JS could declare an intention to delete word backward, it could then take the target range from the resulting beforeinput event.

@rniwa
Copy link

rniwa commented Oct 12, 2016

For example if spellcheckers wants to modify DOM, they should:

  1. First call preventDefault() and dispatch a new 'beforeinput', describing the change;
  2. Then modify DOM only if it's not cancelled.

In this way we can have StaticRange as an attribute.

Are you suggesting that we don't do anything in response to DOM mutations? That's fine with us.

I would then still think it's best to leave the ability to declare new beforeinput events within JavaScript for version 2 for reasons of simplicity

InputEvent already has a constructor. I don't see what else needs to be defined for authors to dispatch beforeinput events.

@chong-z
Copy link
Contributor Author

chong-z commented Oct 12, 2016

it's not really easy to figure out what the targetrange of deleting a word backward would be using only JavaScript.

We could probably add something like extend('backward', 'word') to Selection API?

Are you suggesting that we don't do anything in response to DOM mutations? That's fine with us.

Yes. I think who (JS handlers) intents to mutate DOM should be responsible for dispatching 'beforeinput' beforehand. (If he believes he is not the final consumer)

Reasons for not doing it in UA:

  1. Tracking DOM change doesn't guarantee target ranges to be useful/valid.
    • e.g. For text Two Word|. Other JS handler might remove the space in the middle on deleteWordBackward. The target ranges would still be Word but it should be considered as invalid.
  2. Too complex as we also have to track selection/caret change as described in Example 2.
  3. Not very useful in the multiple event handlers case, where spellcheckers might want to turn insertParagraph (Enter key) into insertReplacementText (or +insertParagraph).

CCing participants from #38: @smaug---- @garykac @ojanvafai @annevk
Does the solution proposed in #40 (comment) work?

@annevk
Copy link
Member

annevk commented Oct 13, 2016

I don't understand what it means to define it as a one-time-use only event? If you're simply saying that developers are on their own if they make mutations to the tree, I still don't really see how that works with multiple listeners that might not know from each other that they exist.

@rniwa
Copy link

rniwa commented Oct 13, 2016

I still don't really see how that works with multiple listeners that might not know from each other that they exist

I think what @choniong is suggesting is recommending that the author scripts just call preventDefault() whenever it's messing with DOM by convention.

It does raise an interesting question about what such a listener is supposed to when it receives its own beforeinput / input (fired as after it prevented the original one) though.

@johanneswilm
Copy link
Contributor

So preventDefault will mean no other listeners will receive the event?
Right now JS cannot fire it's own beforeinput events, but once we add that in the next version or so, maybe we could add an option to tag it with an identifier, so that the script can avoid receiving its own events?

@rniwa
Copy link

rniwa commented Oct 13, 2016

Right now JS cannot fire it's own beforeinput events

That's just not true. document.body.dispatchEvent(new Event('beforeinput')) words on any browser today, and document.body.dispatchEvent(new InputEvent('beforeinput')) would work on any browser that supports InputEvent.

@johanneswilm
Copy link
Contributor

johanneswilm commented Oct 13, 2016

Ok, let me restate that: We have not added provisions to make this an event that is meant to be triggered by JavaScript. Those used to be there in an earlier draft of the spec, if I remember correctly.

@rniwa How would I set the target range on such an auto-created event, for example to replace an arbitrary part of the DOM that's not connected to the selection?

@annevk
Copy link
Member

annevk commented Oct 13, 2016

@rniwa did you mean stopImmediatePropagation()? preventDefault() doesn't help much with respect to other listeners. (Of course, stopImmediatePropagation() might break other parts of the page if you don't know what the other listeners are doing.)

@johanneswilm
Copy link
Contributor

Of course, stopImmediatePropagation() might break other parts of the page if you don't know what the other listeners are doing.

I wonder under what circumstances and for what usecases one could really say one can safely implement an extra feature without going through the plugin system of the editor in question.

Some other issues:

  1. The spell checker likely wants to draw a span around the word so that it can underline it that will be unexpected for the main editor. It could move these spans to another part of the DOM and then use positioning to put them on top of the word in question, but then it will have to update it with any potential layout change. In order to do this, it needs to analyze the page fairly well to know what is being positioned where and to add its HTML to a part of the DOM where it doesn't ruin other things.

  2. After the word has been underlined, the user needs to be able to interact with the spell checker. So it needs to add a menu and make sure that it doesn't interfere with the UI controls of the editor.

  3. We are only scratching the surface. For example, one of the bug reports of above-mentioned spellcheck plugin that tries to do this, mentions:

    It replaces pictures with base64 images, which can transform a 30kB message to multiple megabytes fairly quickly. [1]

    I don't know why it would do that, but I assume it was programmed with one editor in mind where this made sense. If the editor application receives a beforeinput event saying that the user intents to replace one image with another, it will have to do a number of checks on the transferData that it likely wouldn't do if it didn't know of the existence of this plugin.

One place where a general plugin could maybe make sense would be a JavaScript-based IME that only has minimal UI elements? The main advantage here is that editors have "learned" that they shouldn't do much of anything during an IME composition in order not to cancel it. So as long as the JS IME removes all it's UI elements from the DOM after it's done, this may work in say 90% of JS editors.

[1] http://daviseford.com/node/72

@chong-z
Copy link
Contributor Author

chong-z commented Oct 13, 2016

For example a simple spellchecker can be written like this:

// Spell Checker
// Replacing 'omw' with 'On My Way!' on Enter key.
editor.addEventListener('beforeinput', event => {
    // Only do spellcheck on user action.
    if (!event.isTrusted || event.defaultPrevented)
        return;

    if (event.inputType == 'insertParagraph') {
        // Enter key
        if (getWordBackward() == 'omw') {
            event.preventDefault();
            // Calculate target, ranges, etc.
            const substitutionEvent = new InputEvent('beforeinput', {
                inputType: 'insertReplacementText',
                data: 'On My Way!',
                ranges: [/* StaticRange of 'omw' */],
                cancelable: true,
            });
            let cancelled = !target.dispatchEvent(substitutionEvent);
            if (!cancelled) {
                // Probably no other editors, e.g. UA default contentEditable.
                // Mutating DOM by myself.
            }
            // Create and dispatch 'insertParagraph'.
        }
    }
});

And an editor can be implemented as below:

// Editor
editor.addEventListener('beforeinput', event => {
    if (event.defaultPrevented)
        return;

    event.preventDefault();

    if (event.isTrusted) {
        // User action, handle as normal.
    } else {
        // Probably generated by spellchecker, handle with caution.
    }
});

Notes:

  1. Assuming there is a way for spellcheckers to register their event listeners before any other (I'm not sure how though)
  2. JS generated 'beforeinput' doesn't have default UA action.

@johanneswilm
Copy link
Contributor

Notes:

And assuming:

  • The spellchecker can do it's underlining without interfering with the DOM in the editor.
  • The spellchecker checker does not create UI elements that interfere with the editor.
  • The spell checker figures out when an element boundary corresponds to a word boundary and doesn't mess with those. The same is also true for browser built-in spell checkers, but at least the JS editor developers can test their editor with the handful of most common browsers and adjust their editors to work in those. And they can turn the browser-based spellchecker off, which many of the common editors do. This plugin is not turn-off-able, is it?
  • The user doesn't have more than one of these plugins installed. If a spell checker, a grammar checker, JS-based IME, an "add funny emojis mid-sentence" plugin and a "try to convert s for Cyrillic alternatives" plugin all try to compete for the beforeinput events, then all but the first event will ignore the other events because they are untrusted, correct?

Again, I am not convinced this is a good way to have people to create plugins (in the long run it may be more fruitful to try to get JS editors to standardize on a plugin system), but if we do it, would it not be possible to tag a beforeinput event with an identifier, so that these plugins can avoid listening to the beforeinput events they cause themselves?

ranges: [/* StaticRange of 'omw' */],

This is functionality we would need to add, right? Currently there isn't a way to specify the target ranges in JS, as far as I can tell.

@chong-z
Copy link
Contributor Author

chong-z commented Oct 13, 2016

The spellchecker can do it's underlining without interfering with the DOM in the editor.

Spellchecker can dispatch 'insertReplacementText' with proper dataTransfer, e.g.

myDataTransfer.setData('text/html', '<span class="underline-cls">Misteka</span>');

The spellchecker checker does not create UI elements that interfere with the editor.

I don't quite get it. Spellcheckers can do whatever they want, but instead of modifying DOM directly, they should dispatch 'beforeinput' and let the editor execute the action.

This plugin is not turn-off-able, is it?

Editors can ignore !isTrusted events, and handle all isTrusted evnets even if it's defaultPrevented.

The user doesn't have more than one of these plugins installed.... then all but the first event will ignore the other events because they are untrusted, correct?

Yes, it doesn't support 'plugin-chain'.

  1. One 'beforeinput' can only be handled by at most one plugin, and
  2. Events posted by plugins should go to end point editor directly.

But of course those plugins can have their own 'plugin' API as well.

would it not be possible to tag a beforeinput event with an identifier, so that these plugins can avoid listening to the beforeinput events they cause themselves?

I believe JS can attach arbitrary attributes to event objects, no?

But then how do you solve the case that 2 plugins may catch events in turn and dispatch new events (with the private tag), which would cause an infinite loop?

This is functionality we would need to add, right? Currently there isn't a way to specify the target ranges in JS, as far as I can tell.

Since StaticRange is an IDL dictionary I believe you can simply do:

let theSpan = ...;
let ranges = [{startContainer: theSpan, startOffset: 4, endContainer: theSpan, endOffset: 7}];

@chong-z chong-z changed the title Should getTargetRanges() describe "user's intention" or "UA's default action"? Make targetRanges an attribute which doesn't response to DOM mutations Oct 13, 2016
@johanneswilm
Copy link
Contributor

johanneswilm commented Oct 13, 2016

The spellchecker can do it's underlining without interfering with the DOM in the editor.

Spellchecker can dispatch 'insertReplacementText' with proper dataTransfer, e.g.

myDataTransfer.setData('text/html', '<span class="underline-cls">Misteka</span>');

The spellchecker checker does not create UI elements that interfere with the editor.

I don't quite get it. Spellcheckers can do whatever they want, but instead of modifying DOM directly, they should dispatch 'beforeinput' and let the editor execute the action.

The way all the richtext editors on the web work is that they strictly limit the type of HTML they permit within the element they control. For most input types they can do so directly. For some inputtypes they don't get an event before, so instead they have to roll back the DOM change afterward and replace it with what they want.

For this reason, editors cannot work with an arbitrary span which they don't understand the meaning of within their element. Placing it there by direct DOM manipulation will break the editor. Creating a beforeinput event to place a span there will in all cases mean that the span is being replaced by some other HTML before it becomes part of the DOM (it will be cleaned, like a paste), so in that case the underlining won't work. Such cleanup will in normally include removal of all classes, IDs and other attributes, and elements will be replaced by the whitelist of accepted elements the particular editor accepts. Some common attributes may survive this, such as href attributes on links, but classes and IDs will usually not. Also, spans will usually not survive the cleanup process, as the editor doesn't know what semantic meaning they are supposed to have. Editors themselves often use spans for various purposes, which is another reason why spans are removed from html that is somehow added to the editing host from the outside.

That's why I can only see one option of placing those underlines: Put the elements to which are used to display underlining will be attached in another part of the DOM. Then track the position of the word and update some CSS code to always have the position of the line correspond with where the word is. But for this really to work reliably, the JS will likely have to parse all the CSS connected with the page and analyze the DOM in detail.

To give you an analogy: Imagine if the Operating system had some code that checked which program is started, and if it's Chrome or another browser, it starts interfering in how the layoutting on the screen is done to put it's own adds on web pages, without letting the browser know about it. This may work in a test and on 10 sites which this was made for, but it will likely break the rest of the internet. The same way you expect for such "interventions" to come in the form of a plugin in your browser, the JS editors will in most cases find it difficult to deal with an unknown piece of software interfere with what they think is "their" editor element.

Now this is only the part about putting lines under words in an editor that doesn't know about the existence of the spell checker. Adding UI elements for the spell checker will be similarly problematic. The part about creating a beforeinput event when once the user has selected to replace a word with the spellchecked version should be more straightforward, and for that part we could likely in most cases do what you outlined above.

@chong-z
Copy link
Contributor Author

chong-z commented Oct 13, 2016

So basically you are suggesting that Multi-listener style plugin system is not so useful.

OK that's probably true, what they can safely do are:

  1. Plain-text substation.
  2. Simple rich text substation with HTML tags like

    , .

  3. Underlining mechanism issues could probably solved by your proposal in the future.

But I guess we should discuss this as a separate issue (e.g. Design a general plugin system).


However my proposal is: Making StaticRange targetRanges an attribute and do not observe DOM mutation (as UA cannot do it well enough, see reasons #40 (comment)).

I cannot see how the plugin issue would block this proposal, or do you prefer the original getTargetRanges()?

@johanneswilm
Copy link
Contributor

OK that's probably true, what they can safely do are

I agree with those. Possibly during IME a little more, because the editor is aware that IMEs do changes to the DOM that it should not try to do anything about before the IME is over.

I cannot see how the plugin issue would block this proposal, or do you prefer the original getTargetRanges()?

I agree with your proposal. Should we maybe add a note to JS authors that they should be aware that a second listener will get an unusable staticrange and that in the cases where there may be multiple listeners and they change the DOM, they should call stopImmediatePropagation() to stop subsequent listeners from getting a useless event?

The thing I would like to leave for later is trying to figure out all the details of this plugins and whether this is the best way to create plugins altogether. However, if adding the static range directly to the event means that it will make it easier to create useful beforeinput events in JS in the future, then that is just positive.

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

4 participants