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

Pass the focused input via ActivateKeyboard #540

Closed

Conversation

ShawnCZek
Copy link
Contributor

@ShawnCZek ShawnCZek commented Nov 17, 2023

Similarly to 89a357b, I want to expand the options for integrating IME, specifically for a custom solution. Such integration must directly work with the focused text input. And what better way to do this than handling the ActivateKeyboard() method of the system interface?

The user of the library can then, for example, create an observer for the input element and later override parts of the text, move the cursor around, etc...

I admit this is a breaking change. But since master is currently heavily WIP, too, it is the best time to do so. 😉

@mikke89
Copy link
Owner

mikke89 commented Nov 21, 2023

Thanks for this and all the other PRs :)

While I do see the potential convenience for users with this change, I feel like this is not in line with the intended responsibilities of the system interface. The system interface is intended to be an abstraction over the target system, not really a place to initiate complex behavior on elements. Letting the system interface know about elements at all, in my opinion blurs its responsibilities. Generally, it introduces another dimension of potentially breaking behavior by refactoring or calling the system interface in different occasions. It also raises some questions, what if we want to show the keyboard for some reason, but there is no element?

I think instead we should consider the specific needs the client side may have here, and rather attempt to add the appropriate API on the relevant elements themselves - or alternatively submit sufficient information over the system interface to do the necessary system calls.

@ShawnCZek
Copy link
Contributor Author

I understand your point of view; you are completely right here. The concern is, as described in my first message, making a custom IME implementation, which has to work with the focused input area. And I just went with the flow of the commit mentioned above.

Passing the context instead of the text input element might be closer to the system's responsibility. However, it does not address the initial problem; in this specific case, GetFocusElement() does not equal the input element (yet).

Focusing just on the problem and forgetting this entire pull request, it might be better to implement a text input handler for the context/document or enhance the focus event (for the context) by passing the element in question. I personally would be a fan of the prior, but I will leave this up to you; maybe you have a better idea of how to resolve this.

@mikke89
Copy link
Owner

mikke89 commented Nov 26, 2023

Yeah, I like the idea of a text input handler. It sounds like you've given some thought to how it could look like, so I am very much open to hearing more about that, or seeing a quick draft.

If you can get away with retrieving the element from the focus event, then I believe it should be available as the event's target element. It will give a lot of noise from non-input elements though. But at least in principle you could filter them and look for ElementFormControlInput target elements.

@ShawnCZek
Copy link
Contributor Author

Listening to all contexts/documents for their focus event is something I considered before. Still, it is a solution that does not sound ideal, in my opinion, especially considering that the blur event is as important as the focus. Not only that, but such an input handler can serve more purposes, for example, notifying about a text change.

For the sake of keeping discussions relevant and leaving this pull request as evidence that this is not the proper solution, I will close it and later open a new one with a text input handler.

@ShawnCZek ShawnCZek closed this Nov 27, 2023
@ShawnCZek ShawnCZek deleted the activate_keyboard_focused_input branch December 5, 2023 20:49
@ShawnCZek ShawnCZek mentioned this pull request Dec 6, 2023
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.

2 participants