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

Should execCommand dispatch beforeinput or not? #200

Open
saschanaz opened this issue Aug 25, 2019 · 66 comments
Open

Should execCommand dispatch beforeinput or not? #200

saschanaz opened this issue Aug 25, 2019 · 66 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Aug 25, 2019

I briefly tested execCommand and beforeinput in https://codepen.io/SaschaNaz/pen/WNepGPa. For execCommand("bold"), Chrome does not dispatch beforeinput while Safari does dispatch it with inputType: "" (whereas both dispatch input). As I see nothing about beforeinput on execCommand spec, which is the expected behavior here?

@saschanaz
Copy link
Member Author

Based on #194 I assume this is also a Safari bug?

It tests whether execCommand only emit an input event and no beforeinput event. It fails if it emits a beforeinput event.

@rniwa
Copy link
Contributor

rniwa commented Aug 27, 2019

This seems like more of an oversight of the spec. It does seem more consistent to fire beforeinput in this case.

@masayuki-nakano
Copy link
Collaborator

I suggested the Chrome's behavior half a decade ago. Then reason is, calling execCommand() from beforeinput event handler creates really difficult issues from point of view of both compatibility and security. So, currently, Firefox tries to follow the Chrome's behavior.

@masayuki-nakano
Copy link
Collaborator

It's declared by Input Events: https://w3c.github.io/input-events/#event-definitions

@saschanaz
Copy link
Member Author

saschanaz commented Aug 28, 2019

It's declared by Input Events: https://w3c.github.io/input-events/#event-definitions

Yes, but wasn't sure whether that covers execCommand or not. If it does, maybe we can just remove the event part in execCommand spec without modification to use InputEvent there.

@johanneswilm
Copy link
Contributor

Yes, it would be preferable to make the adjustment only in the execCommand spec.

@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 2, 2019

@saschanaz Sorry, I had to reread your comment. I would prefer for everything execCommand-related to stay in the execCommand spec so that execCommand is being contained there. So I would suggest to fix the section in the execCommand spec and not move this to the input events spec, also not implicitly. The input events does not mention execCommand on purpose. execCommand is not used by any of the current editors except for a small number of cases (document.execCommand("copy") - others?) since Firefox removed the other two cases for which it was useful. It should therefore be completely contained in the execCommand spec and not flow out into other specifications. There is already a section in the execCommand spec on how it should relate to the beforeinput/input events. That section should be updated to be in line with the input events spec.

@saschanaz
Copy link
Member Author

@johanneswilm So we should define some InputEvent thing in execCommand, right? I can try that, but first we should decide whether beforeinput or not.

@johanneswilm
Copy link
Contributor

@saschanaz As for whether or not beforeinput in connection with execCommand or not: given that execCommand isn't really in use, I don't care much either way. But I can see that @masayuki-nakano has a point that if it is enabled it can easily lead to issues if a user then adds executes execCommand to the beforeinput event handler.

@saschanaz
Copy link
Member Author

Yes, the current spec also mentions that some dirty thing can be done by beforeinput. I suggest we spec the common part first and defer any beforeinput thing.

@johanneswilm
Copy link
Contributor

That sounds like a good idea to me.

@masayuki-nakano
Copy link
Collaborator

I don't think that execCommand spec needs to define the beforeinput event behavior since the most important point of whether beforeinput should be dispatched or not is that it caused by user input or JS.

@johanneswilm
Copy link
Contributor

@masayuki-nakano But apparently the wording in the input events spec isn't clear enough, and so it wouldn't hurt to be more specific this in the execCommand spec. If beforeinput is to behave differently in the case of execCommand as opposed to direct DOM manipulation by JavaScript, then that should be specified in the execCommand spec.

@masayuki-nakano
Copy link
Collaborator

@johanneswilm This must be an issue of wording in UI Events spec. For example, setting HTMLInputElement.value, calling HTMLInputElement.setRangeText() and same for HTMLTextArea shouldn't cause beforeinput too.

@rniwa
Copy link
Contributor

rniwa commented Sep 2, 2019

if it is enabled it can easily lead to issues if a user then adds executes execCommand to the beforeinput event handler.

I'd rather have a consistency that beforeinput event is fired whenever input event is fired since execCommand does fire input event. We have this kind of mismatch for selectstart event and selectionchange event and it's quite a mess. Every inconsistency like this is an extra cognitive pressure every new Web developer has to endure, and we should avoid adding such a thing as much as possible.

We can make event.isTrusted to false or add a new boolean to InputEvent to make it obvious if there is a use case in which beforeinput fired for execCommand needs to be disambiguated against ones fired for user initiated editing operations.

@johanneswilm
Copy link
Contributor

Every inconsistency like this is an extra cognitive pressure every new Web developer has to endure

It would be, if execCommand would be useful for web developers. The thing is - it's not. When you start writing a JavaScript editor, you may waste a week or so on trying to do it with execCommand until you find out that it's not actually possible to create anything production-quality with it and you start from scratch. That's why it's so important that we have the disclaimer there telling everyone to stay far away from it.

There also seems to be some interest in execCommand from some browser developers. I don't entirely understand the reason for that, and that reason may maybe have something to do why they want it to behave one way or the other? Either way, given that this is a technology not really in use and not really production ready despite having been shipped for more than a decade, I think we should contain it to the one spec draft with that big disclaimer, because otherwise it really gets confusing for web developers if they read about something they can do with execCommand one place without the disclaimer and only once they find the execCommand spec do they get to understand that they shouldn't actually use execCommand.

I haven't seen a real usecase for generating beforeinput events apart from execCommand in JavaScript either, except maybe to provide an event in some case where the browser isn't emitting it due to a bug. And then that should probably be fixed in the browser instead. Unless we find such a usecase, I don't think we should add anything about an untrusted version of the event behaving differently to the Input Events spec.

@masayuki-nakano
Copy link
Collaborator

if it is enabled it can easily lead to issues if a user then adds executes execCommand to the beforeinput event handler.

I'd rather have a consistency that beforeinput event is fired whenever input event is fired since execCommand does fire input event.

Basically, I agree with that.

Every inconsistency like this is an extra cognitive pressure every new Web developer has to endure, and we should avoid adding such a thing as much as possible.

From point of view of different position, there is consistency between setting .value and execCommand() if browsers don't dispatch beforeinput for every changes from JS.

We've already been struggling with mutation event listeners for avoiding security issues so that I don't want new path to nesting edit actions.

What Safari does for the case to call execCommand from beforeinput event listener?

We can make event.isTrusted to false or add a new boolean to InputEvent to make it obvious if there is a use case in which beforeinput fired for execCommand needs to be disambiguated against ones fired for user initiated editing operations.

I feel it's odd. Event.isTrusted should be set to true if it's dispatched by browser and even if it's caused by JS API.

@rniwa
Copy link
Contributor

rniwa commented Sep 3, 2019

From point of view of different position, there is consistency between setting .value and execCommand() if browsers don't dispatch beforeinput for every changes from JS.

I don't follow what you're saying here. What consistency are we talking about? What about .value and execCommand()?

We've already been struggling with mutation event listeners for avoiding security issues so that I don't want new path to nesting edit actions.

This isn't really anything to do with historical source of security bugs in editing, which stems from firing events in the middle of editing commands. Since beforeinput event gets dispatched before the browser engine starts making DOM mutations for the editing command, in terms of reentrancy problem, it's akin to executing arbitrary scripts before entering editing command, which scripts can already before invoking document.execCommand.

I feel it's odd. Event.isTrusted should be set to true if it's dispatched by browser and even if it's caused by JS API.

This is why I suggested an alternative which is to add a new boolean on InputEvent itself. However, is this even a use case that comes up? What are concrete use scenarios in which author scripts need to behave differently when beforeinput event is triggered by execCommand?

@masayuki-nakano
Copy link
Collaborator

From point of view of different position, there is consistency between setting .value and execCommand() if browsers don't dispatch beforeinput for every changes from JS.

I don't follow what you're saying here. What consistency are we talking about? What about .value and execCommand()?

I meant that whether beforeinput event is dispatched or not when web apps modifies editable contents with API. I meant that if beforeinput event is fired only while calling some API, it's inconsistent between API.

We've already been struggling with mutation event listeners for avoiding security issues so that I don't want new path to nesting edit actions.

This isn't really anything to do with historical source of security bugs in editing, which stems from firing events in the middle of editing commands. Since beforeinput event gets dispatched before the browser engine starts making DOM mutations for the editing command, in terms of reentrancy problem, it's akin to executing arbitrary scripts before entering editing command, which scripts can already before invoking document.execCommand.

Indeed, except stack-overflow risk.

I feel it's odd. Event.isTrusted should be set to true if it's dispatched by browser and even if it's caused by JS API.

This is why I suggested an alternative which is to add a new boolean on InputEvent itself. However, is this even a use case that comes up? What are concrete use scenarios in which author scripts need to behave differently when beforeinput event is triggered by execCommand?

I have no idea for handling beforeinput caused by execCommand. That means that web apps may want to distinguish whether each beforeinput is fired by execCommand or not. If so, web apps need to set a wide scope flag to check it in event listeners. Therefore, I think that it's better not to dispatchbeforeinput event at execCommand.

@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 3, 2019

That means that web apps may want to distinguish whether each beforeinput is fired by execCommand or not.

Web apps do not use execCommand for editing and it's not an option to use it either except for small 5-line demos. Unless we can find such an interest (which we have failed to do for the past 5 years) from any web app, I don't see why we should consider this case.

@rniwa
Copy link
Contributor

rniwa commented Sep 3, 2019

I meant that whether beforeinput event is dispatched or not when web apps modifies editable contents with API. I meant that if beforeinput event is fired only while calling some API, it's inconsistent between API.

I still don't understand what you're trying to say. It seems more inconsistent for beforeinput to not fire in cases where input event would otherwise fire.

Indeed, except stack-overflow risk.

That's not a security risk.

I feel it's odd. Event.isTrusted should be set to true if it's dispatched by browser and even if it's caused by JS API.

This is why I suggested an alternative which is to add a new boolean on InputEvent itself. However, is this even a use case that comes up? What are concrete use scenarios in which author scripts need to behave differently when beforeinput event is triggered by execCommand?

I have no idea for handling beforeinput caused by execCommand. That means that web apps may want to distinguish whether each beforeinput is fired by execCommand or not.

But why? What are scenarios in which one has to distinguish the two?

If someone is writing a library which needs to react to whenever an editing operation gets triggered in a contenteditable region (e.g. in order to update corresponding data model), then it needs to listen to such an event regardless of whether it happened due to direct user inputs or execCommand. There is no need to distinguish the two.

If so, web apps need to set a wide scope flag to check it in event listeners. Therefore, I think that it's better not to dispatchbeforeinput event at execCommand.

Again, why? What are scenarios in which author scripts have to rect differently in those two cases?

@masayuki-nakano
Copy link
Collaborator

That means that web apps may want to distinguish whether each beforeinput is fired by execCommand or not.

Web apps do not use execCommand for editing and it's not an option to use it either except for small 5-line demos.

As far as I know, some developers want to use only simple execCommand for making modification undo-able with browser built-in undo stack. Anyway, not point of this issue.

I meant that whether beforeinput event is dispatched or not when web apps modifies editable contents with API. I meant that if beforeinput event is fired only while calling some API, it's inconsistent between API.

I still don't understand what you're trying to say. It seems more inconsistent for beforeinput to not fire in cases where input event would otherwise fire.

In my understanding, beforeinput is suggested for that web apps can cancel or overwrite the behavior of user operation before browsers modify the DOM tree. If somebody want to "correct" characters and make it undo-able without their own transaction manager, they may want to do:

editor.addEventListener("beforeinput", (event) => {
  if (event.inputType === "insertText") {
    let modifiedData = modifySomething(event.data);
    if (modifiedData !== event.data) {
      document.execCommand("insertText", false, modifiedData);
      event.preventDefault();
    }
  }
});

If someone is writing a library which needs to react to whenever an editing operation gets triggered in a contenteditable region (e.g. in order to update corresponding data model), then it needs to listen to such an event regardless of whether it happened due to direct user inputs or execCommand. There is no need to distinguish the two.

I don't think that beforeinput even is a good solution for that purpose because it cannot catch DOM tree modification by DOM API. Isn't mutation observer designed for such purpose? I think that events defined by UI Events are useful only for listening user actions.

@johanneswilm
Copy link
Contributor

johanneswilm commented Sep 4, 2019

As far as I know, some developers want to use only simple execCommand for making modification undo-able with browser built-in undo stack.

The empirical data we have collected so far shows something else.

It's impossible to use it for what you mention as long as the editor supports at least one operation that requires JavaScript dom manipulation. Even if you create an editor that only does those things that are covered by execCommand, it will not work because only using browser built-in operations means that it will produce different HTML in different browsers. I made an extensive survey about practices, and we arrived at this result [1].

The execCommand calls that are in use would be copy and probably still enableObjectResizing and enableInlineTableEditing because Firefox only recently removed the need to execute these two. It's also possible that there are intranet solutions which require all users to use the same browser that use execCommand somewhat more extensively.

Do you have data showing something else? In that case it would be interesting to look at.

Anyway, not point of this issue.

Actually it is. If there is no interest from the JavaScript side in execCommand - why are we (again) wasting all this time on it? I can understand from the persepctive of a browser developer that this seems like something that should be fixed, just as newbie web developers will likely try to write their first 5 line text editor with it. It's once you have to write an editor that is not just for demoing purposes, that either has to run in multiple browsers or do at least one operation not supported by the browser, or that you have to get around at least one bug in the browsers that you notice that this isn't an option.

I don't think that beforeinput even is a good solution for that purpose because it cannot catch DOM tree modification by DOM API. Isn't mutation observer designed for such purpose? I think that events defined by UI Events are useful only for listening user actions.

I think about ten years ago it was probably normal to have say 10 or 20 small independent JavaScript programs run on a page that didn't interact much with oneanother and where it would be possible for one program to modify the DOM and for another to only notice later. This is not common practice today. The different libraries used on a webpage are all connected by means of a common main program and also generally bundled into a common bundle. The editing library will get a DOM element that it will have "full control" over and so there should not be these arbitrary changes to the DOM coming from unknown sources.

Because we don't have beforeinput events level 2 implemented everywhere and so not everything is covered by it, the common practice in JavaScript editors indeed seems to be to use mutation observers to listen for changes within a certain range, but then not to use the information that comes with the event and instead just diff the dom. It's used in combination with a timeout so that multiple mutations that directly follow oneanother do not trigger a large amount of diffs.

[1] #150

@masayuki-nakano
Copy link
Collaborator

It's impossible to use it for what you mention as long as the editor supports at least one operation that requires JavaScript dom manipulation. Even if you create an editor that only does those things that are covered by execCommand, it will not work because only using browser built-in operations means that it will produce different HTML in different browsers.

Is it true for some simple commands like insertText, insertLineBreak, insertParagraph and delete with simple DOM tree? (If so, I'd like to fix such incompatibility issues as far as possible after finishing implementingbeforeinput event on Firefox.)

If there is no interest from the JavaScript side in execCommand - why are we (again) wasting all this time on it?

I don't think "waste" is good term for execCommand. Both backward and cross browser compatibility of (unfortunately) existing API are important if they can be fixed by browser side.

@johanneswilm
Copy link
Contributor

As a developer I can understand that it feels frustrating if there is something that isn't quite clean and where it feels that if only I invested a little more time I could get this to be much smoother and simpler.

However, I think it's misguided in the case of execCommand. We have an actual need of improvements in the area of editing, and they are not in any way related to execCommand. Yet that is what 80-90% of the focus here seems to be on. I cannot stress enough that efforts would be much better invested in just about anything other than execCommand.

execCommand cannot work unless you implement every single feature an editor needs because if just one feature relies on dom tree manipulation then the global history will be screwed. Imagine you have spent say 5 years creating an execCommand-based editor (lets just say that this would actually be possible), and then you get a feature request for one more thing that happens not to be covered by execCommand, or a bug report about something where execCommand outputs an incorrect DOM structure. The only option you now have is to throw away all your code and reimplement it all again using direct dom manipulation. That's one of the many reasons editors are not actually built on top of execCommand.

I don't think "waste" is good term for execCommand. Both backward and cross browser compatibility of (unfortunately) existing API are important if they can be fixed by browser side.

Again, I get it from the view point of a developer. It's like stepping into a room and on the table there is a puzzle that looks like it's 50% done and you are being told to just throw it out rather than to finish it. If we had an ton of people to work on all this and everything from input events to Clipboard API, contenteditable-disabled, content menus, IME issues, etc. was spec'ed and implemented everywhere - sure no problem that some people try to go play with the puzzle. But as long as that is not the case, it just ends up being a gigantic distraction to constantly go back to execCommand.

@Reinmar
Copy link

Reinmar commented Sep 6, 2019

Completely agree with @johanneswilm. Some basic aspects like "should execCommand fire beforeInput" are definitely worth clarifying, but as a JS developer I have zero interest in actually fixing anything in any of the commands. We don't use this API and AFAIK no other advanced editor does. There are many other issues, though, that we'd be much more interested to work on. We talk about those, but I feel that execCommand just attracts too much attention and shifts the focus away from important things.

@masayuki-nakano
Copy link
Collaborator

I wonder, if spec allows browsers to dispatch beforeinput event for execCommand, please declare all inputType values of each command and declare whether each of them cancelable or not. And I need definitions how to handle nested execCommands. For example, both beforeinput event listener and input event listeners can set value of <input> and <textarea> or modify the DOM in contenteditable. Especially if beforeinput event is not canceled but the values are modified, it requires that how the original default action should be handled. E.g., where is the good place to insert the original edit action's data/dataTransfer.

@johanneswilm
Copy link
Contributor

Hey @masayuki-nakano I think there is a misunderstanding here. You can implement pretty much whatever you want with execCommand as it's a non-specced and largely unused feature. That also means that we should not add anything execCommand-specific to the input events spec.
Again, I advice strongly against wasting too much time on execCommand as it's not something the JavaScript developers working with editors have requested because they don't actually work with execCommand.

@masayuki-nakano
Copy link
Collaborator

masayuki-nakano commented Nov 19, 2019

@johanneswilm I mean that if beforeinput should be fired for any execCommand commands and make it cancelable, corresponding inputType value should be there. Currently, I plan to make any beforeinput events whose inputType value is not declared in the spec non-cancelable on Firefox (e.g., resizing elements with Firefox built-in resizer causes beforeinput event, but not cancelable since there is no corresponding inputType value).

@johanneswilm
Copy link
Contributor

Resolution call 2020-01-10: @johanneswilm will create a PR to the execCommand spec draft about this.

@masayuki-nakano
Copy link
Collaborator

masayuki-nakano commented Jan 15, 2020

* Add the `beforeinput` event to the `execCommand` spec draft. It's the closest thing we have to a real spec of `execCommand` so that's where that should live.

I agree.

* Add an empty string as an `inputType` for `execCommand`s that cannot be mapped to an existing `inputType` from the `Input Events` spec. Specify this in the `execCommand` draft spec.

If inputType value is empty string, I think that the beforeinput event should not be cancelable since it does not make to sense preventing default of unknown events. Note that Firefox will dispatch non-cancelable beforeinput event for undefined edit operation with empty inputType.

* On the question on whether or not to add a `beforeinput` event only to `execCommand` if they come from a browser plugin or not, I do not have a strong opinion, but it would seem that for consistency it would be simpler to just add it every time.

Well, I still don't agree with that execCommand should cause beforeinput event (if the caller is web apps). However, if Chromium developers agree with that, Mozilla will follow that. But once browsers start to dispatch beforeinput for execCommand, there should be an API to distinguish whether coming beforeinput event is caused by execCommand or appending prefix or postfix to existing inputType values might be better. E.g., "execCommand-insertText". Then, I think that it won't break existing web apps which check inputType value of beforeinput.

(FYI: first implementation of beforeinput event on Firefox was landed yesterday into Nightly channel, you can test it with turning dom.input_events.beforeinput.enabled to true in about:confing.)

@johanneswilm
Copy link
Contributor

johanneswilm commented Jan 15, 2020

If inputType value is empty string, I think that the beforeinput event should not be cancelable since it does not make to sense unknown events. Note that Firefox will dispatch non-cancelable beforeinput event for undefined edit operation with empty inputType.

I guess this would be like level 1 of the spec. Would not that be weird in webkit though, as they have implemented level 2 which means that all of the beforeinput events can be cancelled? Is the beforeinput that comes with execCommand in webkit currently cancelable? If it is, I propose we just don't specify what it is. That way you can do what seems to make most sense in Firefox. I don't have a strong opinion though.

But once browsers start to dispatch beforeinput for execCommand, there should be an API to distinguish whether coming beforeinput event is caused by execCommand or appending prefix or postfix to existing inputType values might be better. E.g., "execCommand-insertText". Then, I think that it won't break existing web apps which check inputType value of beforeinput.

I think such a prefix would break the usecase described by @scottfr above. Also, given that beforeinput has not been present on Firefox so far, I think those editors that rely on it will likely additionally rely on some alternative methods anyway and know that they will update the app once Firefox has it. Could you describe a case in which an app would break because of this?

@tszynalski
Copy link

@johanneswilm
What you seem to be asking for is a way for a JS app to generate input events which are indistinguishable from real input events issued by the user. I'm not sure that should be a design goal at all, given that, for example, JS cannot generate keyboard events that are indistinguishable from the real thing.

Generally speaking, I believe you should err on the side of giving developers more information. The reason is simple: If your app doesn't need some piece of information, you can always ignore it. But if you need that piece of information and it's not there, there's nothing you can do. Above, I have described an example where it's important to distinguish between user-issued and app-issued beforeinput events. You can bet there are many more cases like that.

@johanneswilm
Copy link
Contributor

johanneswilm commented Jan 15, 2020

What you seem to be asking for is a way for a JS app to generate input events which are indistinguishable from real input events issued by the user.

@tszynalski I am not asking for anything. I would like for execCommand to just disappear altogether. I am here trying to find a solution for those of you who do still use execCommand for something. How about the suggestion made above by @rniwa to use isTrusted or alternatively another property to make this differentiation in those cases where you really need it? The prefix solution suggested by @masayuki-nakano has that shortcoming that it will then only work for those editors that had execCommand in mind when they built their editor. In many cases, that will not happen, because the editors themselves don't use execCommand and so it's only if the user combines the editor with a special browser plugin that does use execCommand that this will be relevant.

Above, I have described an example where it's important to distinguish between user-issued and app-issued beforeinput events. You can bet there are many more cases like that.

I am not really sure I agree with the need for the distinction in that example: what if the user has a physical keyboard with both a dash and an endash - then you'd surely also want to turn that feature of yours off as well for the regular dash, right? But yes, I can theoretically see that there might be cases where this it makes a difference. I am not sure is important enough to add an entirely new property.

@tszynalski
Copy link

@johanneswilm
I think the isTrusted solution would work for me, as would a dedicated property.

If the user has a keyboard with a physical endash key, then the user may or may not want to auto-convert dashes. If the physical endash is hard to access (e.g. some AltGr combo), it may still be more comfortable to just have your editor do it. Some users may not know the right combination as well (regular users are often shockingly unaware of modifier shortcuts). Anyway, I'm not sure why you would bring up that argument, seeing as it applies only to a small percentage of users.

@johanneswilm
Copy link
Contributor

@tszynalski Yes, so the editor itself should not use execCommand at all, and if it does, it would not be the kind of editor that uses beforeinput so that should not matter. The usecase of @scottfr though is that you have something external - likely a browser plugin that adds some functionality by pasting things in - and it currently only has execCommand for that. That seems it could also work for a solution similar to your own editor solution for users who lack just a few keys on their keyboard that they need for one specific language [1] if instead of doing the input in a special editor, you'd add those keys by means of a browser plugin so they work as sort of an additional soft keyboard.

Anyway, I'm not sure why you would bring up that argument, seeing as it applies only to a small percentage of users.

The point is it is a very special case and some users may actually not want the behavior you describe. Or they want it even if using their physical keyboard. So you'd need an on/off setting somewhere anyway. And so given that it is such a special case, it's not clear that it is worth the effort to add this to the spec and then program it for all the browsers.

Another approach we could take (that I'd also be OK with) is to do as @masayuki-nakano suggested and prefix all the beforeinput events related to execCommand or even remove them entirely. Then execCommand would not be useful for @scottfr and then we'd need to add a new way for JavaScript to execute all the actions corresponding to all the different input types and fire the chain fo events.

At the last call the solution was that I create a PR on the execCommand spec for the next call (in February). Given that I don't have a strong opinion on the exact solution other than that this needs to be resolved in a way that works for users, I am also happy to discuss alternative PRs that others present at next month's call.

[1] https://www.typeit.org/

@masayuki-nakano
Copy link
Collaborator

If inputType value is empty string, I think that the beforeinput event should not be cancelable since it does not make sense preventing default of unknown events. Note that Firefox will dispatch non-cancelable beforeinput event for undefined edit operation with empty inputType.

I guess this would be like level 1 of the spec. Would not that be weird in webkit though, as they have implemented level 2 which means that all of the beforeinput events can be cancelled? Is the beforeinput that comes with execCommand in webkit currently cancelable?

Well, WebKit can cancel beforeinput event which is caused by execCommand("bold"). That's what I concern.

If it is, I propose we just don't specify what it is. That way you can do what seems to make most sense in Firefox. I don't have a strong opinion though.

If there are somebody who don't want beforeinput events coming from execCommand called by their web apps, they must use a flag like isDoingExecCommand. I'd like to avoid such design.

But once browsers start to dispatch beforeinput for execCommand, there should be an API to distinguish whether coming beforeinput event is caused by execCommand or appending prefix or postfix to existing inputType values might be better. E.g., "execCommand-insertText". Then, I think that it won't break existing web apps which check inputType value of beforeinput.

I think such a prefix would break the usecase described by @scottfr above.

I meant that if the caller of execCommand is browser itself or part of browser (i.e., addons), such prefix shouldn't be added because it looks like a user's intention from point of view of web apps. So, I meant that when execCommand is called by web contents, adding prefix or setting new attribute value of InputEvent.

Also, given that beforeinput has not been present on Firefox so far, I think those editors that rely on it will likely additionally rely on some alternative methods anyway and know that they will update the app once Firefox has it. Could you describe a case in which an app would break because of this?

I'm thinking about like this case (it does not matter whether such web apps really exist or not):

editor.addEventListener("beforeinput", event => {
  if (event.inputType === "insertText") {
    let filteredData = event.data.replace(/Something/g, "OtherData");
    if (filteredData != event.data) {
      event.preventDefault();
      if (filteredData !== "") {
        document.execCommand("insertText", false, filteredData);
      }
      return;
    }
  }
});

This example does not assume that this event listener won't be called again and if the developer tests only with Chrome, this works as expected. So, it may cause unexpected result due to recursive call or infinite loop.

As a developer of browser, it may cause web-compat issues of recursive calls of a beforeinput event listener. If it does not prevent default but modifies selection and/or editor value, it'll create really complicated case. So, I'd like spec to avoid complicated cases as far as possible. (If beforeinput event would be fired later asynchronously as non-cancelable event, it'd become simpler, though.)

@johanneswilm
Copy link
Contributor

I guess this would be like level 1 of the spec. Would not that be weird in webkit though, as they have implemented level 2 which means that all of the beforeinput events can be cancelled? Is the beforeinput that comes with execCommand in webkit currently cancelable?

Well, WebKit can cancel beforeinput event which is caused by execCommand("bold"). That's what I concern.

Yes, I think that is the case for all beforeinput events in level 2. So JS running on Safari may assume that it can always cancel all beforeinput events. And that's really where we want to go anyway, so I don't think we should move away from that.

However, level 1 does not specify that "beforeinput cannot be cancelled". It just says "beforeinput cancelable: Undefined" so that also those who implement level 2 are compliant with level 1. I suggest we do the same in this case - we set it to "Undefined".

But once browsers start to dispatch beforeinput for execCommand, there should be an API to distinguish whether coming beforeinput event is caused by execCommand or appending prefix or postfix to existing inputType values might be better. E.g., "execCommand-insertText". Then, I think that it won't break existing web apps which check inputType value of beforeinput.

I think such a prefix would break the usecase described by @scottfr above.

I meant that if the caller of execCommand is browser itself or part of browser (i.e., addons), such prefix shouldn't be added because it looks like a user's intention from point of view of web apps. So, I meant that when execCommand is called by web contents, adding prefix

In that case I am OK with your suggestion. Some legacy apps may use execCommand directly in the app, but those apps then don't need to also use beforeinput events, at least not in the non-prefixed version.

I would also be ok with just using an empty string as inputType in these cases.

or setting new attribute value of InputEvent.

I don't think I understood this suggestion. How would that work?

I'm thinking about like this case (it does not matter whether such web apps really exist or not):

Ok, there are other ways of breaking websites and there is a limit as to how much baby sitting one can expect. Also, if we made Chrome be compliant with whatever behavior we specify here, then the developer would already notice the issue when testing in Chrome. That being said, I am totally OK with execCommand being treated differently if it is executed on the website than if it is executed by an extension/addon.

@tszynalski
Copy link

And so given that it is such a special case, it's not clear that it is worth the effort to add this to the spec and then program it for all the browsers.

Special case? Perhaps, but this is relevant to any app that converts dashes, apostrophes and quotes automatically (a standard feature in editors) while also allowing the user to type "normal" dashes and straight quotes in some other way. In general, any app that filters user input in some way needs to distinguish between original user input and the output of the filter. Otherwise, it will re-apply the filter to data which has already been filtered. Yeah, it's possible to hack around the problem, but the most elegant solution would be to have the InputEvent itself indicate whether it is issued by the user.

@scottfr
Copy link

scottfr commented Jan 16, 2020

I think such a prefix would break the usecase described by @scottfr above.

I meant that if the caller of execCommand is browser itself or part of browser (i.e., addons), such prefix shouldn't be added because it looks like a user's intention from point of view of web apps. So, I meant that when execCommand is called by web contents, adding prefix

In that case I am OK with your suggestion. Some legacy apps may use execCommand directly in the app, but those apps then don't need to also use beforeinput events, at least not in the non-prefixed version.

These seams reasonable to me.

editor.addEventListener("beforeinput", event => {...});

Not to diminish the compatibility concerns, but as an aside, if 'beforeinput' was added to execCommand without any changes to the inputType, I think this specific example would still happen continue to work correctly in Chrome as they bail when they detect recursive execCommand command calls: (https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/editing/commands/document_exec_command.cc;l=75). So I think the first execCommand would execute and then the second would fail. (Maybe this recursion behavior is worth clarifying too or opening an issue on?)

@johanneswilm
Copy link
Contributor

johanneswilm commented Jan 16, 2020

Special case? Perhaps, but this is relevant to any app that converts dashes, apostrophes and quotes automatically (a standard feature in editors) while also allowing the user to type "normal" dashes and straight quotes in some other way.

@tszynalski Remember this is only for those apps that receive execCommand from a third party plugin that they cannot control and additionally want to be aware that the dash comes from execCommand.

@tszynalski
Copy link

@johanneswilm
OK, now I'm lost. Starting from here, I've been talking about JS apps (not extensions) being able to distinguish between user-issued input and app-issued input. I spoke out, because you appeared to suggest that input issued by execCommand() should be indistinguishable from real input provided by the user. But perhaps you were only talking about browser extensions that use execCommand(), not Web apps that use execCommand().

@johanneswilm
Copy link
Contributor

@tszynalski JS is single threaded so of you insist on using execCommand internally in your editor app, in spite of our warnings against it, then you should still have no issue distinguishing a beforeinput event that was caused by execCommand in your own code from one that comes from user input. Just set some property like this.executingCommand = true somewhere in your code before you run the execCommand and then turn it off when you are done. Also, we should absolutely not encourage the usage of execCommand unless there is no alternative.

@masayuki-nakano Given the comment by @scottfr on Chrome preventing recursive execCommand, do you still feel there is a need to differentiate? Do you have another example where this will be relevant?

@masayuki-nakano
Copy link
Collaborator

@johanneswilm @scottfr

Oh, that's really interesting and I'd like Firefox to follow the behavior because I'm still struggling with security bugs caused by nested execCommand calls. So, I don't have another example, but from a point of view of DOM Event stuff of Mozilla, I still feel that it's odd if web apps cannot distinguish whether it comes from execCommand call of themselves or not, though.

@masayuki-nakano
Copy link
Collaborator

Ah, and also could be similar problem, if beforeinput event listener does something with setTimeout(..., 0);.

@masayuki-nakano
Copy link
Collaborator

Indeed, only Chrome bans nested execCommand: testcase
(FYI: Firefox's behavior is tricky. input event is omitted by async event dispatcher, probably.)

@johanneswilm
Copy link
Contributor

johanneswilm commented Jan 24, 2020

ok, so that leaves us with the following (it seems):

  • In case of browser plugins/extensions: We agree that execCommand should cause a beforeinput event and that this beforeinput event should use the regular inputTypes in case there are is one corresponding to the command and otherwise an empty string. This should be added to the execCommand spec by means of a PR1.

  • In case of execCommand triggered within a webapp: We are OK with also triggering a beforeinput event for execCommand commands being executed within a regular web page so this will be added to PR1.

  • In case of execCommand triggered within a webapp: We cannot at this stage agree on how or whether to mark the beforeinput event in some way: by means of the isTrusted attribute, by means of a separate attribute, by means of a prefix to the inputType or by nothing at all. So we will not add anything about this in the spec at this stage, but we will open a new github issue specifically about this issue.

  • We would like something about not allowing nested execCommand as is currently practiced by Chrome to the execCommand spec and we'll add it by means of a PR2.

Is that correctly understood? If it is, I will go ahead with trying to write a draft for PR1 (and maybe PR2 unless someone else wants to do that). If it is not correctly understood, let me know what isn't right and I'll try to adjust.

@scottfr
Copy link

scottfr commented Jan 27, 2020

In case of browser plugins/extensions: We agree that execCommand should cause a beforeinput event and that this beforeinput event should use the regular inputTypes in case there are is one corresponding to the command and otherwise an empty string. This should be added to the execCommand spec by means of a PR1.

This fully addresses the concern I raised.

@masayuki-nakano
Copy link
Collaborator

* **In case of browser plugins/extensions**: We agree that execCommand should cause a beforeinput event and that this beforeinput event should use the regular inputTypes in case there are is one corresponding to the command and otherwise an empty string. This should be added to the `execCommand` spec by means of a PR1.

I agree. But I wonder, this means that extensions may implement browser feature with execCommand and web apps may want to cancel only some specific commands, so, it might be worthwhile to add inputType values for some commands.

* **In case of `execCommand` triggered within a webapp:** We are OK with also triggering a `beforeinput` event for `execCommand` commands being executed within a regular web page so this will be added to PR1.

* **In case of `execCommand` triggered within a webapp:** We cannot at this stage agree on how or whether to mark the beforeinput event in some way: by means of the `isTrusted` attribute, by means of a separate attribute, by means of a prefix to the `inputType` or by nothing at all. So we will not add anything about this in the spec at this stage, but we will open a new github issue specifically about this issue.

And also, should be defined as the beforeinput events caused by execCommand within web apps should be one of the following options:

  • always cancelable?
  • same as inputType definition? (if so, how about for empty inputType cases?)
  • always non-cancelable?
* We would like something about not allowing nested `execCommand` as is currently practiced by Chrome to the `execCommand` spec and we'll add it by means of a PR2.

Yeah, sounds reasonable and make it browser users safer.

@johanneswilm
Copy link
Contributor

johanneswilm commented Feb 10, 2020

But I wonder, this means that extensions may implement browser feature with execCommand and web apps may want to cancel only some specific commands, so, it might be worthwhile to add inputType values for some commands.

As long as there isn't consensus on the answer to this question, it means that we do not have agreement on the point of using the empty string in these cases and the inputType that we write in the spec for those cases should then therefore be undefined, I believe.

And also, should be defined as the beforeinput events caused by execCommand within web apps should be one of the following options:

  • always cancelable?
  • same as inputType definition? (if so, how about for empty inputType cases?)
  • always non-cancelable?

The usecase we found here and that we are trying to cover is to emulate already existing native browser behavior using execCommand. For that reason I would argue that it needs to behave as close as possible to the native browser behavior that it is trying to emulate (also in terms of being cancelable). In the case of an execCommand that does not have a native browser counterpart (so that the inputType is undefined) that is not something we have a usecase for currently, so I don't have an opinion.

Notice that this is not a perfect solution but just a temporary crutch to allow the insertion of content using execCommand paste which should be close enough to the desired results for many external grammar and spell checkers, auto text entering mechanisms, etc. . But the inputType will in most of these cases be semantically wrong so we continue to need a more permanent solution to be developed some time in the future.

@johanneswilm johanneswilm removed the Agenda+ Agenda item to be inserted in the Editing TF meeting queue label Feb 14, 2020
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Mar 30, 2020
….execCommand()` is called by addon r=smaug

Discussion in
* w3c/input-events#91
* w3c/editing#200

Web developers must want to handle `beforeinput` events which are caused by
addons because addon's behavior is exactly same as default action of browser
from point of view of web apps.

Differential Revision: https://phabricator.services.mozilla.com/D68528
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 30, 2020
….execCommand()` is called by addon r=smaug

Discussion in
* w3c/input-events#91
* w3c/editing#200

Web developers must want to handle `beforeinput` events which are caused by
addons because addon's behavior is exactly same as default action of browser
from point of view of web apps.

Differential Revision: https://phabricator.services.mozilla.com/D68528

--HG--
extra : moz-landing-system : lando
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Mar 31, 2020
….execCommand()` is called by addon r=smaug

Discussion in
* w3c/input-events#91
* w3c/editing#200

Web developers must want to handle `beforeinput` events which are caused by
addons because addon's behavior is exactly same as default action of browser
from point of view of web apps.

Differential Revision: https://phabricator.services.mozilla.com/D68528

UltraBlame original commit: 684fdad241529b919670d21d860edc094512946f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Mar 31, 2020
….execCommand()` is called by addon r=smaug

Discussion in
* w3c/input-events#91
* w3c/editing#200

Web developers must want to handle `beforeinput` events which are caused by
addons because addon's behavior is exactly same as default action of browser
from point of view of web apps.

Differential Revision: https://phabricator.services.mozilla.com/D68528

UltraBlame original commit: 684fdad241529b919670d21d860edc094512946f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Mar 31, 2020
….execCommand()` is called by addon r=smaug

Discussion in
* w3c/input-events#91
* w3c/editing#200

Web developers must want to handle `beforeinput` events which are caused by
addons because addon's behavior is exactly same as default action of browser
from point of view of web apps.

Differential Revision: https://phabricator.services.mozilla.com/D68528

UltraBlame original commit: 684fdad241529b919670d21d860edc094512946f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants