HTML5 API: Add support for 'input' event #23550

alibabashack · 2025-01-31T00:27:29Z

Implements the HTML DOM 'input' event as suggested in #23523.

alibabashack · 2025-01-31T00:39:37Z

In review please consider one problem: The EmscriptenInputEvent::data field is potentially a very long UTF8 string on the JS side, because the input event could be a paste of a long text. In my implementation this string is truncated to 256 chars, because I was not sure how to handle the JS to wasm handover otherwise. I think the truncation is ok for most use cases of the input event (e.g. form validation where I want to read the DOM value of the event target anyway). However, I am concerned about truncating a UTF8 string, because this could split a code point midway. I am sure there is some experience with this kind of problem in this code base. Any advice?

You may want to try the new interactive test case:
test/runner interactive.test_html5_input

alibabashack · 2025-01-31T13:16:39Z

I think I need some help to interpret the CI result. I don't understand why test_offset_converter_pthread and test_pthread_wait64_notify tests fail. They seem to be unrelated to my work. Any help is much appreciated, thanks.

Edit: Oh, I see, looks main is broken...

sbc100 · 2025-01-31T17:19:17Z

I think I need some help to interpret the CI result. I don't understand why test_offset_converter_pthread and test_pthread_wait64_notify tests fail. They seem to be unrelated to my work. Any help is much appreciated, thanks.

Edit: Oh, I see, looks main is broken...

Those tests are known to be flaky. We can re-run the CI and they should pass.

sbc100 · 2025-01-31T17:27:38Z

system/include/emscripten/html5.h

@@ -189,6 +190,15 @@ EMSCRIPTEN_RESULT emscripten_set_focus_callback_on_thread(const char *target __a
 EMSCRIPTEN_RESULT emscripten_set_focusin_callback_on_thread(const char *target __attribute__((nonnull)), void *userData, bool useCapture, em_focus_callback_func callback, pthread_t targetThread);
 EMSCRIPTEN_RESULT emscripten_set_focusout_callback_on_thread(const char *target __attribute__((nonnull)), void *userData, bool useCapture, em_focus_callback_func callback, pthread_t targetThread);

+typedef struct EmscriptenInputEvent {
+  EM_UTF8 data[EM_HTML5_LONG_STRING_LEN_BYTES];


I think we should avoid truncation here and instead consider two possible options:

Use stack allocated space. i.e. stringToNewUTF8OnStack() on the JS side. This would means it would be to up the user to make a copy of the string if they want it to outlive the callback.

Use heap allocation. i.e stringToNewUTF8 on the JS side. It would then be incumbent upon the receiver to call free on the resulting pointer when they are done with it.

I guess (2) seems safer since it avoids a possible stack overflow.

Thanks for the suggestions. I think (1) is out of question because it is very easy even for an end user to create very large change data values by Ctrl+A, Ctrl+V or similar. Crashes will happen.

I don't feel too good about (2) either because there will be different target groups of users . Some can completely ignore the provided data field. E.g. for my current use case which is form validation. I just need to know that something changed and will request the value from the DOM element anyhow. It is waste to perform an allocation here and require the user to take ownership. I can already smell all the memory leaks produced in situations where the data field is ignored. Other users typically only care about a single input char because they deal with keystroke-like stuff.

Maybe we could have independent sets of registration and callback functions:

Requests input events with full data via $stringToNewUTF8: for those who really need all the data and have to pay in terms of performance due to an allocation being made and dealing with ownership semantics

Requests input events without data; still fixed fields (inputType, etc.) could be provided: for those who don't need any data and are just waiting for an any-change hook.

Requests input events with truncated data; the data field is partially copied (as currently implemented): for those who deal with keystrokes and other small input data sets (e.g. see use case in Improve support in SDL2 for keyboard input on mobile browsers #21960) which may occur with high rate and the input delta data is still relevant; where performance is key.

I think case 2 is optional, because the target group could use case 3 as well and ignore the provided truncated data. Any thoughts?

How about anther option: Use a dyanmic / heap allocation, but have it live only for the life of the callback.

You could use realloc on a fixed pointer to avoid allocation except when the region grows.

See SDL_GetKeyName in libsdl.js for an example to using a fixed buffer with realloc.

Great, realloc looks perfect, I'll go this route. Thanks!

Implementation uses realloc for events on the default thread, now.

For all other threads I am allocating every single time as other event types seem to do. Is there some kind of free() required? I don't understand the semantics involved here.

src/lib/libhtml5.js

test/test_html5_input.c

sbc100 · 2025-01-31T17:35:39Z

test/test_html5_input.c

+
+#ifdef REPORT_RESULT
+  REPORT_RESULT(testStatus());
+#endif


Can you just do exit(testStatus()) here and use btest_exit to run the test? This might require using emscriopten_runtime_keeplive_push() in main and emscripten_runtime_keeplive_pop() here.

In fact maybe just inline testStatus here?

Can you just do exit(testStatus()) here and use ...

I did the best I can. This works when testStatus() returns 0, then the browser will close and the test suite stops. However, on non 0 testStatus(), the browser does not close and the test suite remains stuck. This is a little inconvenient.

In fact maybe just inline testStatus here?

I would like to keep the checks in a function, because the function definition can stay very close to the observed test status variable definitions. In this way it is more obvious the check function needs to be upgraded when tests are amended.

src/lib/libhtml5.js

alibabashack · 2025-02-03T14:17:43Z

I also added $registerInputEventCallback__proxy: 'sync', because it seems to be present on all other callback handlers. To be honest I have no idea what this does. Could someone explain? Thanks.

Required due to new function emscripten_set_input_callback_on_thread .

alibabashack · 2025-02-03T14:34:37Z

rebased to latest master

sbc100

lgtm, with a few minor nits, and some comments about the test code.

@kripken and @juj, I don't know much about how the html5 API evolved or why this was not originally part of it. Does this addition look reasonable to you? @dschuff too.

sbc100 · 2025-02-05T18:21:55Z

src/lib/libhtml5.js

+        var isComposing = e.isComposing;
+        const dataLengthWithTermination = lengthBytesUTF8(data) + 1;
+        const eventSize = {{{ C_STRUCTS.EmscriptenInputEvent.__size__ }}} + dataLengthWithTermination;
+        if(JSEvents.inputEvent == null){


Space after keywords like if.

sbc100 · 2025-02-05T18:22:04Z

src/lib/libhtml5.js

+#endif
+
+    var inputEventHandlerFunc = (e = event) => {
+        var data = e.data || '';


We use 2-space indentation.

sbc100 · 2025-02-05T18:23:02Z

src/lib/libhtml5.js

+        const dataLengthWithTermination = lengthBytesUTF8(data) + 1;
+        const eventSize = {{{ C_STRUCTS.EmscriptenInputEvent.__size__ }}} + dataLengthWithTermination;
+        if(JSEvents.inputEvent == null){
+            JSEvents.inputEvent = _malloc(eventSize);


No need to call _malloc at all here since _realloc works with null pointer as firs argument.

sbc100 · 2025-02-05T18:24:17Z

src/lib/libhtml5.js

+        }
+        else if(JSEvents.inputEventAllocatedSize < eventSize) {
+            JSEvents.inputEvent = _realloc(JSEvents.inputEvent, eventSize);
+            JSEvents.inputEventAllocatedSize = eventSize;


Maybe just call realloc unconditionally and then you don't need to trace inputEventAllocatedSize at all.

sbc100 · 2025-02-05T18:25:21Z

system/include/emscripten/html5.h

+typedef struct EmscriptenInputEvent {
+  bool isComposing;
+  EM_UTF8 inputType[EM_HTML5_SHORT_STRING_LEN_BYTES];
+  EM_UTF8 data[];


Perhaps worth adding a comment here. EM_UTF8 data[]; // variable size string; must be the final element of the struct

sbc100 · 2025-02-05T18:28:19Z

test/test_html5_input.c

+                               void* const userData) {
+  printEvent(inputEvent, __func__);
+
+  if (currentGoal != GoalInputSingleLetter) {


How about just assert(currentGoal == GoalInputSingleLetter) here and elsewhere?

sbc100 · 2025-02-05T18:29:12Z

test/test_html5_input.c

+
+static void* const expectedUserDataPtr = (void*)0x13370001;
+
+static bool


Would it help keep the test simpler (more readable) if you just removed all the static keywords? They don't really add anything to the test I think.

sbc100 · 2025-02-05T18:31:04Z

test/test_html5_input.c

+// * copy and paste:
+static const char* const inputType_insertFromPaste = "insertFromPaste";
+// * drag and drop:
+static const char* const inputType_insertFromDrop = "insertFromDrop";


Maybe drop the second const here since it adds visual noise?

Would it be more readable to just write #define INPUTTYPE_INSERTTEXT "insertText"?

alibabashack force-pushed the input_event_support branch from 71393fa to be16f63 Compare January 31, 2025 11:45

alibabashack mentioned this pull request Jan 31, 2025

Improve support in SDL2 for keyboard input on mobile browsers #21960

Open

sbc100 reviewed Jan 31, 2025

View reviewed changes

src/lib/libhtml5.js Outdated Show resolved Hide resolved

sbc100 reviewed Jan 31, 2025

View reviewed changes

src/lib/libhtml5.js Outdated Show resolved Hide resolved

alibabashack requested a review from sbc100 February 3, 2025 14:04

alibabashack added 10 commits February 3, 2025 15:29

HTML5 API: Add support for 'input' event

4d9030c

re-generate struct_info after array size adjustment

9fa44a4

Approve test_gen_sig_info expectations

68ee316

Required due to new function emscripten_set_input_callback_on_thread .

use logical operator || to assign default value on null

436854f

move preprocessor directives to column 0

c5439aa

fix indentation

2d5cccc

make data field variable size

b3db0cd

reorder tests

9f21410

no longer expect data field truncation

0279d81

use btest_exit

f07bcd7

alibabashack force-pushed the input_event_support branch from 4728dca to f07bcd7 Compare February 3, 2025 14:32

sbc100 reviewed Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML5 API: Add support for 'input' event #23550

HTML5 API: Add support for 'input' event #23550

alibabashack commented Jan 31, 2025 •

edited

Loading

alibabashack commented Jan 31, 2025 •

edited

Loading

alibabashack commented Jan 31, 2025 •

edited

Loading

sbc100 commented Jan 31, 2025

sbc100 Jan 31, 2025

alibabashack Jan 31, 2025

sbc100 Jan 31, 2025

sbc100 Jan 31, 2025

alibabashack Jan 31, 2025

alibabashack Feb 3, 2025

sbc100 Jan 31, 2025

alibabashack Feb 3, 2025

alibabashack commented Feb 3, 2025

alibabashack commented Feb 3, 2025

sbc100 left a comment

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025

sbc100 Feb 5, 2025


		static void* const expectedUserDataPtr = (void*)0x13370001;

		static bool

HTML5 API: Add support for 'input' event #23550

Are you sure you want to change the base?

HTML5 API: Add support for 'input' event #23550

Conversation

alibabashack commented Jan 31, 2025 • edited Loading

alibabashack commented Jan 31, 2025 • edited Loading

alibabashack commented Jan 31, 2025 • edited Loading

sbc100 commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alibabashack commented Feb 3, 2025

alibabashack commented Feb 3, 2025

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alibabashack commented Jan 31, 2025 •

edited

Loading

alibabashack commented Jan 31, 2025 •

edited

Loading

alibabashack commented Jan 31, 2025 •

edited

Loading