-
Notifications
You must be signed in to change notification settings - Fork 40
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
Comments
Based on #194 I assume this is also a Safari bug?
|
This seems like more of an oversight of the spec. It does seem more consistent to fire |
I suggested the Chrome's behavior half a decade ago. Then reason is, calling |
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. |
Yes, it would be preferable to make the adjustment only in the execCommand spec. |
@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 ( |
@johanneswilm So we should define some InputEvent thing in execCommand, right? I can try that, but first we should decide whether beforeinput or not. |
@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. |
Yes, the current spec also mentions that some dirty thing can be done by |
That sounds like a good idea to me. |
I don't think that |
@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 |
@johanneswilm This must be an issue of wording in UI Events spec. For example, setting |
I'd rather have a consistency that We can make |
It would be, if There also seems to be some interest in I haven't seen a real usecase for generating beforeinput events apart from |
Basically, I agree with that.
From point of view of different position, there is consistency between setting 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
I feel it's odd. |
I don't follow what you're saying here. What consistency are we talking about? What about
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
This is why I suggested an alternative which is to add a new boolean on |
I meant that whether
Indeed, except stack-overflow risk.
I have no idea for handling |
Web apps do not use |
I still don't understand what you're trying to say. It seems more inconsistent for
That's not a security risk.
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
Again, why? What are scenarios in which author scripts have to rect differently in those two cases? |
As far as I know, some developers want to use only simple
In my understanding,
I don't think that |
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 Do you have data showing something else? In that case it would be interesting to look at.
Actually it is. If there is no interest from the JavaScript side in
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 |
Is it true for some simple commands like
I don't think "waste" is good term for |
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
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 |
Completely agree with @johanneswilm. Some basic aspects like "should |
I wonder, if spec allows browsers to dispatch |
Hey @masayuki-nakano I think there is a misunderstanding here. You can implement pretty much whatever you want with |
@johanneswilm I mean that if |
Resolution call 2020-01-10: @johanneswilm will create a PR to the execCommand spec draft about this. |
I agree.
If
Well, I still don't agree with that (FYI: first implementation of |
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
I think such a prefix would break the usecase described by @scottfr above. Also, given that |
@johanneswilm 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 |
@tszynalski I am not asking for anything. I would like for
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. |
@johanneswilm 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. |
@tszynalski Yes, so the editor itself should not use
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 At the last call the solution was that I create a PR on the |
Well, WebKit can cancel
If there are somebody who don't want
I meant that if the caller of
I'm thinking about like this case (it does not matter whether such web apps really exist or not):
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 |
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".
In that case I am OK with your suggestion. Some legacy apps may use I would also be ok with just using an empty string as inputType in these cases.
I don't think I understood this suggestion. How would that work?
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. |
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 |
These seams reasonable to me.
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?) |
@tszynalski Remember this is only for those apps that receive |
@johanneswilm |
@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 @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? |
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 |
Ah, and also could be similar problem, if |
Indeed, only Chrome bans nested |
ok, so that leaves us with the following (it seems):
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. |
This fully addresses the concern I raised. |
I agree. But I wonder, this means that extensions may implement browser feature with
And also, should be defined as the
Yeah, sounds reasonable and make it browser users safer. |
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
The usecase we found here and that we are trying to cover is to emulate already existing native browser behavior using 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 |
….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
….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
….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
….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
….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
I briefly tested
execCommand
andbeforeinput
in https://codepen.io/SaschaNaz/pen/WNepGPa. ForexecCommand("bold")
, Chrome does not dispatchbeforeinput
while Safari does dispatch it withinputType: ""
(whereas both dispatchinput
). As I see nothing aboutbeforeinput
on execCommand spec, which is the expected behavior here?The text was updated successfully, but these errors were encountered: