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

[GLFW] Fix the bug that prevents any text on the site from being deleted #22879

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

uysalibov
Copy link

If glfw is initizalized, it prevents the deletion of text in the input and textarea elements on the site. If this is not filtered like this, no form on the site works as desired.
Related issue: #22866

@sbc100 sbc100 changed the title Fixing the bug that prevents any text on the site from being deleted [GLFW] Fix the bug that prevents any text on the site from being deleted Nov 7, 2024
if (event.keyCode === 8 /* backspace */ || event.keyCode === 9 /* tab */) {
// Prevent default behavior for backspace and tab keys when the event target is not an input or textarea element.
// If prevented when input and textarea are prevented, no text in the site can be deleted
if ( event.target.localName !== "input" && event.target.localName !== "textarea" &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No space after (.

@@ -419,7 +419,10 @@ var LibraryGLFW = {
// This logic comes directly from the sdl implementation. We cannot
// call preventDefault on all keydown events otherwise onKeyPress will
// not get called
if (event.keyCode === 8 /* backspace */ || event.keyCode === 9 /* tab */) {
// Prevent default behavior for backspace and tab keys when the event target is not an input or textarea element.
// If prevented when input and textarea are prevented, no text in the site can be deleted
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about Otherwise, no text in the site can be deleted.

Can you wrap this comment at 80 columns?

Should we make the same change to library_sdl.js perhaps?

if (event.keyCode === 8 /* backspace */ || event.keyCode === 9 /* tab */) {
// Prevent default behavior for backspace and tab keys when the event target is not an input or textarea element.
// If prevented when input and textarea are prevented, no text in the site can be deleted
if ( event.target.localName !== "input" && event.target.localName !== "textarea" &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use target.localName rather than, for example, target.tabName? I'm not sure what localName is so maybe you can help me out?

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this BTW! I think we should probably add an interactive test to make sure its working as expected.

@uysalibov
Copy link
Author

uysalibov commented Nov 7, 2024

I tried to make the improvements you mentioned. I have not used tagName before, thank you for the advice, it seems to be a better choice. Also I added same things for sdl. @sbc100

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

I tried to make the improvements you mentioned. I have not used tabName before, thank you for the advice, it seems to be a better choice. Also I added same things for sdl. @sbc100

Sorry I meant tagName: https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

@sbc100
Copy link
Collaborator

sbc100 commented Nov 7, 2024

It looks like you somehow included copies of some other changes that already landed when you rebased/merged here.

@uysalibov
Copy link
Author

I wanted to remove old one for new 2 commit. But somehow i added unrelated commits.

@kripken
Copy link
Member

kripken commented Nov 7, 2024

Is this related to #6143 ? I don't think we had a clear resolution there.

@uysalibov
Copy link
Author

Is this related to #6143 ? I don't think we had a clear resolution there.

Yes, I try to fix that problem.

@kripken
Copy link
Member

kripken commented Nov 7, 2024

@uysalibov I understand, but see @juj's point in that older discussion - there are tradeoffs here, and it's not obvious that flipping it the other way is better. And it would be disruptive to existing users.

I don't have a strong preference myself, except that if we change a long-standing tradeoff in a disruptive way then we should have a very strong reason.

@uysalibov
Copy link
Author

@kripken I can't find a way to remove these related preventation in the structure in the old version. Assuming that the whole page is canvas doesn't seem very correct to me today. It's easier to block these keys (backspace and tab) in the project. On the contrary, we cannot find a solution to this situation since 2018.

@kripken
Copy link
Member

kripken commented Nov 7, 2024

Assuming that the whole page is canvas doesn't seem very correct to me today.

I agree it might not be the best default. It does help porting fullscreen games and similar applications, which historically has been popular with Emscripten. I think that's why we ended up here.

I agree we should think about how to improve things. Can you explain how the "INPUT", "TEXTAREA" changes in this PR work? I'm trying to figure out how disruptive this will be.

@uysalibov
Copy link
Author

uysalibov commented Nov 7, 2024

Imagine a page with canvas and input. Normally backspace and tab operations will be prevented because there is a canvas. But if you click on the input element and try to delete something, this if expressiom will not be entered and you can delete the text as you wish. The main point is to click on the input and textarea elements. Otherwise it will continue to work as before

@kripken
Copy link
Member

kripken commented Nov 8, 2024

@uysalibov I understand that, but I am asking specifically about "INPUT" and "TEXTAREA". Does the user need to specify those in the HTML as the id or something like that? If so we should document it.

Another option might be to check if the event was sent to the canvas, and swallowed if so.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 8, 2024

@uysalibov I understand that, but I am asking specifically about "INPUT" and "TEXTAREA". Does the user need to specify those in the HTML as the id or something like that? If so we should document it.

No, those are the names of the HTML tags that accept text as input (Here we are looking at the tagName of the target). Seems like a pretty safe change to me.

Another option might be to check if the event was sent to the canvas, and swallowed if so.

@kripken
Copy link
Member

kripken commented Nov 8, 2024

Oh, thanks, I was confusing Id with Tag I guess. In that case this does sound pretty safe, sgtm.

I do think we should add testing for it. Relevant tests is browser.test_glfw_events, perhaps we can add to that? The source file is

https://github.com/emscripten-core/emscripten/blob/main/test/browser/test_glfw_events.c

and you can run it with ./test/runner browser.test_glfw_events

@uysalibov
Copy link
Author

Another option might be to check if the event was sent to the canvas, and swallowed if so.

I think it's not an option. Because the key event does not always come from the canvas element. Most of the time it comes from the body. If we do this it will break porting projects. input and textarea filtering seems safer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants