Skip to content

revert #168, fix #167 #190

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 24, 2023
Merged

revert #168, fix #167 #190

merged 5 commits into from
Jan 24, 2023

Conversation

Rich-Harris
Copy link
Member

#168 introduced a bug with focus management — if you click on a link inside the iframe, it focuses the editor. This reverts that PR and fixes #167 by emitting a message when the iframe receives a pointerdown event

@vercel
Copy link

vercel bot commented Jan 23, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
learn-svelte-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 24, 2023 at 7:41PM (UTC)

@Rich-Harris Rich-Harris mentioned this pull request Jan 23, 2023
Merged
dummdidumm
dummdidumm reviewed Jan 24, 2023
View reviewed changes
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Looks good, only got one suggestion about code organization 👍

src/routes/tutorial/[slug]/state.js Outdated
* while the editor is focused. Refocus the editor in these cases.
* This boolean tracks whether or not the editor should be refocused.
*/
export let preserve_editor_focus = false;
Copy link
Member

Choose a reason for hiding this comment

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

This state is only relevant to the editor, I'd suggest keeping it in there. We already have svelte:window in Editor.svelte, which we can use to listen to messages as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, updated

@dummdidumm dummdidumm merged commit 00892b4 into main Jan 24, 2023
@dummdidumm dummdidumm deleted the revert-focus-fix branch January 24, 2023 19:47
@tomoam
Copy link
Contributor

tomoam commented Jan 26, 2023 •
edited
Loading

Thank you, but it appears that the issue similar to #167 has occurred.
I reopen #167. Sorry, I am not capable of reopening the issue.

167.mov

@dummdidumm
Copy link
Member

Thanks for reporting, I pushed a fix that hopefully resolved this

@tomoam
Copy link
Contributor

tomoam commented Jan 26, 2023

@dummdidumm
Thank you very much. However, it now appears that another issue has occurred.
When the iframe reloads while I am typing, the text I type is not entered correctly.

lack.mov

I typed export let data;, but entered export let dat;. The a is missing.
Perhaps text entered during the 100ms (the time of refocusing editor) would be lost.
I'll try to figure out how to fix it too.

@dummdidumm
Copy link
Member

God this iframe stuff is so annoying - thanks for taking another look. Maybe also write down a list of manual tests for future reference so we know that further tweaks don't introduce regressions.

@tomoam
Copy link
Contributor

tomoam commented Feb 1, 2023

@dummdidumm
I have created tests with Playwright that cover #150, #167, and this issue (tomoam/learn.svelte.dev:add-test).
Based on this tests, I will create a PR to fix this issue. If you don't need the PR, let me know.

@tomoam tomoam mentioned this pull request Feb 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dummdidumm dummdidumm dummdidumm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot focus on input element inside iframe when the editor has focus.
3 participants
@Rich-Harris @tomoam @dummdidumm