-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR

[juj]

Add -s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR setting to opt out from old event handler target lookup rules in HTML5 API.

This option does a couple of changes:

It changes html5.h API event target lookups to utilize document.querySelector() function instead of document.getElementById() function. querySelector() is much more flexible, since it allows one to use arbitrary CSS selectors, and not just DOM IDs. One can query DOM IDs with # prefix, i.e. document.querySelector('#canvas') will find DOM element with ID "canvas".

In old lookup rules registering an event callback to "#canvas" would target whatever was set to Module['canvas']. This option also removes all use of this Module['canvas'] object, i.e. in the new lookup rules, that property is gone.

In old lookup, one could do

emscripten_set_keydown_callback("#window", ...);
emscripten_set_keydown_callback("#document", ...);

These uses are replaced with special tokens

emscripten_set_keydown_callback(EMSCRIPTEN_EVENT_TARGET_WINDOW, ...);
emscripten_set_keydown_callback(EMSCRIPTEN_EVENT_TARGET_DOCUMENT, ...);

Also, in old lookup, one could use 0/null pointer as a special ID to magically match event registration to happen to the DOM element that I had in 2014 thought to be the "proper" one for each event in question. Since then, it turns out that such magic behavior is a bit counterproductive, as depending on how a site is set up, especially with respect to iframes, event bubbling and capturing. Such magic behavior now feels like a false service, but it was easy to ignore since one could always override. Now that the lookup is about to change, it is a good timing to also drop this kind of magic behavior. So in new lookup rules, attempting to register an event to 0/null pointer will be an error. This has the benefit of catching error scenarios when one accidentally would attempt to register an event to a string that happened to be a null pointer - such mistake will now be guarded against.

This does not break behavior for developers by default, but in -s ASSERTIONS=1 enabled builds, they will get a hint to migrate to the new lookup rules.

Other smaller changes are related to code size optimizations - removing findEventTarget from JSEvents object, window.screen -> screen, and outlining some accesses to document and window. These optimize away -4.5% of the minimal WebGL demo code size.

[kripken]

Sounds generally reasonable, but we should be careful here and get feedback on the direction.

My general thoughts are that we should consider going even further here - not add a new option, and go fully to the new approach as a breaking change. That seems less complex, and avoid deferring the change to later, where current users will eventually have to update. Of course if we do this we'd need to be careful and document it, mention it on the mailing list, etc.

In old lookup rules registering an event callback to "#canvas" would target whatever was set to Module['canvas']. This option also removes all use of this Module['canvas'] object, i.e. in the new lookup rules, that property is gone.

Getting #canvas would still work in the new model, in our default shell at least, since we give the id "canvas" to the canvas, is that right? May be worth documenting that specifically. edit: and also perhaps show a special error in ASSERTIONS mode if #canvas is requested but no canvas exists with that id.

[juj]

My general thoughts are that we should consider going even further here - not add a new option, and go fully to the new approach as a breaking change.

I would not want to handle this kind of breakage like this. We cannot reach everyone on emscripten-discuss up front, and people will be angry when things break on them even if there was a heads up email. Especially because many developers use a library like SDL2, and the registration occurs inside that and not their own code. I have already gone through the trouble of authoring this as optional, having to remove that and wait for an "internet echo" of a random sampling of people for a +1 on a breaking change that will affect everyone else as well does not feel good.

This option could be dropped when aligning to something like 1.38.x -> 1.39.0 switch, where the breaking changes are documented, but I'd prefer not to just land this in the tree without an option, since everyone using the HTML5 API would unconditionally break?

[kripken]

My main concern with landing this with an option is that then we must support both options for a while, which is bad in itself, and unless you actually go and do the breaking change later, we end up with it forever. And the problem is that it's a breaking change at some time - why is later better than now?

Clumping breaking changes together in 1.39.* is an option, but I'm not sure it's better that way.

Perhaps we should open a discussion on the mailing list about the breaking changes policy?

[juj]

we must support both options for a while, which is bad in itself

Currently it looks like the two options are self contained in library_html5.js, mostly in those two lookup functions. I am comfortable with the two options, it was not too much work to do.

unless you actually go and do the breaking change later, we end up with it forever.

We have done other breaking changes via deprecation and then remove route, most recently the Pointer_stringify removal that landed yesterday, or defaulting to ERROR_ON_UNDEFINED_SYMBOLS=1 that occurred way back.

the problem is that it's a breaking change at some time - why is later better than now?

The main advantage is to give people time. If we start with this as an option, while starting to warn at runtime, then people can test flipping the option and see how it behaves differently to their working build. We can then later be more aggressive and flip the default over to new behavior (an easy change), requiring people to explicitly revert back to the old option if they still want to keep the old behavior. That way they'll need to acknowledge they are doing something that is going to go away. Then when we finally remove the option at 1.39.0, there is less of a chance of angry people complaining about breakage.

I would be fine with landing this with the option defaulted to the new behavior right now, forcing people already today to explicitly revert to old form, if they still want it. If that might hurry things along faster?

Also, I would be fine with landing this in without an option right now, and forcing everyone else to immediately update their code as well when they upgrade, since all my code is updated already. That would be the simplest path, but it does feel dirty.

What I would not like to do is to have to wait for an indeterminate number of days to accumulate feedback; having to wait for X days for such a conversation to maybe occur is not productive, while the new feature remains unavailable. Therefore I am leaning towards the best option being to make this a build switch, and play it the same route as other deprecations. I do not regard this as being too much complexity to manage - after all I already did do all the work to support both options, so I am fine with carrying the option for a while, given that I am the maintainer of library_html5.js (so I'm not e.g. imposing the complexity to someone else's yard). Making this a switch lets this land now, so that development can go forward, but waiting an unspecified time for feedback could make this PR linger around for weeks. (which is something I feel generally really bummed about)

Perhaps we should open a discussion on the mailing list about the breaking changes policy?

I think our policy has been the standard one for a number of years now? Introduce feature X to be deprecated but still keep it around, and then later remove it once time has passed. (having an option to flip between feature X enabled or not has been a bit of a bonus) This policy of course only applies to "public"/developer facing features of X, and not what we consider to be an internal API or detail.

[kripken]

Fair points. Ok, sounds good to land this as is - add the option, declare it as deprecated, and eventually later we'll flip the option to the new modern way, then eventually remove the old way.

Please add a mention in the changelog about this, mention it on the mailing list, and open an issue about changing the default and removing the old way, and assign yourself to it - hopefully will reduce the risk of us forgetting.

[juj]

For more details on the mailing list, see https://groups.google.com/d/msg/emscripten-discuss/xScZ_LRIByk/_gEy67utDgAJ

[juj]

Adjusted changelog, created an issue and updated mailing list. Thanks for the review here!

[juj]

Oh, this PR adjusted struct_info.json, so bumped version up to 1.38.27 to rebuild cache, though looking at Binaryen repository git log, I am unsure which version would be good to tag as 1.38.27? Historically I would always tag x.yy.z versions to match with the version_xx points in time, so that users who are getting Binaryen via ports would be getting the same version of Binaryen as those users who build via tags, though looking at version history, I see that this scheme has not held since. (probably doesn't matter that much) @kripken: can you tag 1.38.27 in Binaryen side to a good point in git history?

[kripken]

Sure, I tagged 1.38.27 now (usually it is not crucial to be precise there, unless there is a breaking API change, but there wasn't one recently.)

[ghost]

For some reason I had to change my canvas ID to 'canvas' and use this option to get my build of QuakeJS working again. I don't know why this worked.