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

egui_web: constrain the IME text agent to the canvas #830

Merged
merged 6 commits into from
Nov 2, 2021

Conversation

sumibi-yakitori
Copy link
Contributor

@sumibi-yakitori sumibi-yakitori commented Oct 21, 2021

If you enter a large number of line breaks where the height of TextEdit exceeds the height of the browser window, and then enter more non-line breaks, the egui screen will be driven out of the client area

Also, since scrolling is currently disabled in a browser itself, there doesn't seem to be much benefit in changing the position of the text_agent

I apologize for my terrible English

@sumibi-yakitori
Copy link
Contributor Author

Screen Shot 2021-10-31 at 12 51 16

Screen Shot 2021-10-31 at 12 51 50

// style.set_property("top", &(y.to_string() + "px")).ok()?;
// style.set_property("left", &(x.to_string() + "px")).ok()
// })
// } else {
Copy link
Owner

Choose a reason for hiding this comment

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

did you think about what this code was being used for? have you tested that IME still works as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, I must have misunderstood a bit. I feel that by suppressing the position of the text agent from almost sticking out of the client area of the browser, the position of the IME will also stay in line with the position of the text cursor. Is this a good fix?

Copy link
Owner

Choose a reason for hiding this comment

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

@n2 wrote the original code, perhaps they have some thoughts?

@sumibi-yakitori
Copy link
Contributor Author

@emilk
I'd like to hear what @n2 thinks, but for now, I've reverted the previous wrong code and added the code to restrict the position of the text agent to within the client area.

I've also checked the IME position tracking. There is a subtle difference in the IME position tracking compared to the browser native text input, but it seems to be a practical problem.

@sumibi-yakitori
Copy link
Contributor Author

Note: For some reason the CI test was failing, so I rebased to the latest master branch. When I did that, the old commits you had linked to the review comments disappeared.

@n2
Copy link
Contributor

n2 commented Nov 2, 2021

As mentioned in the comments, move_text_cursor() is used to move IME candidate window following egui's cursor. It is better to constrain the position within the canvas, rather than just delete the function. I think your newly submitted code is suitable. Thank you for fixing the overlooked part of my code. @sumibi-yakitori

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Nice, that looks like a much better solution to me!

Thanks for your thoughts, @n2 !

@emilk emilk changed the title Fix a display problem when entering text after the size of TextEdit extends off the screen on the web egui_web: constrain the IME text agent to the canvas Nov 2, 2021
@emilk emilk merged commit b1716be into emilk:master Nov 2, 2021
@sumibi-yakitori sumibi-yakitori deleted the fix-large-text-on-the-web branch November 2, 2021 21:09
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