Cannot type in any HTML text field (filtering in full-screen visualization or renaming project in dashboard)

[Akirathan]

To reproduce:

• Open any table in full-screen visualization.
• Open a filter for any column
• Try to type into the text field that has just popped-up.

It can be reproduced on bd70ed6

[jdunkerley]

This is in the viz pop up as well as the full screen one.

[farmaazon]

Probably related to recent event handling (but not prevent-default).

[wdanilo]

@farmaazon I believe this is cause by prevent default, why do you think otherwise? CC @vitvakatu, as you'd probably know that best, as you've been changing prevent-default behavior recently.

[farmaazon]

I believe this is cause by prevent default, why do you think otherwise?

Thinking twice, I think I was mistaken.

[farmaazon]

The same cause seems to prevent us from renaming project name in the new dashboard. Should be checked as part of this task as well (https://discord.com/channels/401396655599124480/1113051687838560347/1113428099288277033)

Discord

Discord - A New Way to Chat with Friends & Communities

Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

[somebody1234]

well... it breaks all dashboard inputs which is kinda bad:

• renaming projects
• changing password
• search bar
• forms for creating assets
• likely also login, if you sign out

[wdanilo]

If it's related, then it's even higher priority right now, as it breaks a lot of things. @vitvakatu do I remember correctly you've been handling the prevent defaults code recently? If so, can you look at it, please? CC @sylwiabr

[somebody1234]

i think the simplest fix would be to bind all event listeners on #root instead of window. not 100% sure that's foolproof though.

the relevant code is here (not that it's useful for this issue):

enso/lib/rust/frp/src/io/keyboard.rs

Lines 440 to 457 in 343b5fb

	impl DomBindings {
	/// Create new Keyboard and Frp bindings.
	///
	/// We're preventing default on keyboard events, because we don't want the browser to handle
	/// them.
	pub fn new(keyboard: &Keyboard) -> Self {
	let key_down = Listener::new_key_down(f!([keyboard](event:&KeyboardEvent) {
	event.prevent_default();
	keyboard.source.down.emit(KeyWithCode::from(event));
	}));
	let key_up = Listener::new_key_up(f!([keyboard](event:&KeyboardEvent) {
	event.prevent_default();
	keyboard.source.up.emit(KeyWithCode::from(event));
	}));
	let blur = Listener::new_blur(f_!(keyboard.source.window_defocused.emit(())));
	Self { key_down, key_up, blur }
	}
	}

it's also worth noting that this is almost certainly due to leaked objects, which #6911 may fix

[wdanilo]

Handling of mouse events is more complex. On the Rust side we are listening to some events on window and on some events just on the WebGL canvas element. For example, we are listening for mouse move globally, but for mouse clicks, we are listening only on the canvas element. This allows us to get events correctly, when for example there is a DOM element above the canvas (then we don't want to handle the clicks, but we want to be able to handle mouse up for dropping things).

However, I don't see how this issue might be connected to either the decision on what element we are listening for the events nor how it could be connected to memory leaks on scene drop.

[somebody1234]

what element we are listening for the events: if we don't listen on window, the event shouldn't be fired for keypresses on the dashboard (because the dashboard is separate div from #root) - so it would never even get a chance to (incorrectly) preventDefault in the first place

how it could be connected to memory leaks: ideally wasm drop should free the keyboard event listeners on window, since the ide should not be running at that point

[somebody1234]

oh, but of course that wouldn't be a solution for events inside the ide itself (e.g. visualizations)

[wdanilo]

Wait, but the dashboard listens for events somehow, right? This is not connected to how we listen for events in Rust?

Regarding drop, it should work even with memory leaks. Look, we designed it this way not to "steal" events from other elements. For example, if you open performance window on top of Enso Graph, you can click there, and the events are passed correctly there. The same should be true for keyboard events. Enso should not steal events from DOM elements above it, so even with memory leaks, this should work fine.

[somebody1234]

yeah - but the issue is that dashboard UI elements don't process the events because they're being preventDefaulted by rust.

as for not stealing events, that makes sense

i guess the ideal fix would be to only prevent_default when needed. but that might be difficult since the event immediately goes into frp, so you'd need some way to get the value back from frp, and even then i think that would only work if frp is synchronous, otherwise of course prevent_default wouldn't work

an alternative solution might be to only listen to events on the canvas element on which the IDE is rendering, and check that event.current_target == event.target, if that's viable? but on second thought you'd still want to prevent_default shortcuts like ctrl+r, even if they're not handled by the visualization...

so this might be a lot more difficult than it initially seems

[somebody1234]

oh yeah re: dashboard listens for events somehow

i think it doesn't really listen to most keyboard events, the inputs not working are browser default behavior which is being preventDefaulted - since for custom event listeners it would be stopPropagation (if anything) that would prevent those from firing

[wdanilo]

I believe the solution is simpler than it seems. Why we are using preventDefault in the first place? It's an ugly beast and we should not use it if we don't have to! We use it to block some browser shortcuts like ctrl+r, that's it! Right now, we are doing prevent default on every keystroke if Im correct. Instead, we can do it on just predefined set of shortcuts, and everything should work fine. @vitvakatu, please confirm if Im right.

[farmaazon]

If it's related, then it's even higher priority right now, as it breaks a lot of things. @vitvakatu do I remember correctly you've been handling the prevent defaults code recently? If so, can you look at it, please? CC @sylwiabr

Please note @wdanilo that this task is already scheduled for this iteration. It's assigned to me, and I will do it right after finishing the current task (that means today). Let's not make a confusion in the current work.

[vitvakatu]

Instead, we can do it on just predefined set of shortcuts, and everything should work fine. @vitvakatu, please confirm if Im right.

Seems to be correct. Right now we prevent default for all keyboard events (keyup and keydown), but we can do that only for certain shortcuts (like ctrl-r). Though there might be an issue with the tab key. Apparently it causes weird bugs in IDE if not blocked, but it does not make sense to disable it in dashboard.

@somebody1234's suggestion about attaching keyboard events listeners to #root instead of window sounds good to me, but it needs to be checked.

[wdanilo]

If it's related, then it's even higher priority right now, as it breaks a lot of things. @vitvakatu do I remember correctly you've been handling the prevent defaults code recently? If so, can you look at it, please? CC @sylwiabr

Please note @wdanilo that this task is already scheduled for this iteration. It's assigned to me, and I will do it right after finishing the current task (that means today). Let's not make a confusion in the current work.

I'm extremely sorry for the confusion! Thanks Adam for pointing it out!