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

fix: stop preventing canvas pointerdown/tapend events #3207

Merged
merged 10 commits into from Mar 16, 2021

Conversation

dwelle
Copy link
Member

@dwelle dwelle commented Mar 8, 2021

Attempt to fix #3151

Also, this fixes a case where clicking on canvas doesn't deselect selection outside Excalidraw component.

@dwelle dwelle requested a review from ad1992 March 8, 2021 13:03
@vercel
Copy link

vercel bot commented Mar 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/DUyYkKYSwWckh6GViMR2f2vEs3sr
✅ Preview: https://excalidraw-git-dontpreventcanvaspointerdown-excalidraw.vercel.app

@dwelle
Copy link
Member Author

dwelle commented Mar 8, 2021

This actually doesn't work well, as it select text outside the component on canvas double-click. Will investigate.

@dwelle
Copy link
Member Author

dwelle commented Mar 8, 2021

So, I kept not preventing default on pointerdown, but had to revert not preventing selection on canvas which was also allowing selecting stuff outside the component.

Unfortunately, this means that clicking on canvas still retains selection that's outside the component. If we need that behavior, we'll need to manually deselect on pointerdown.

src/css/styles.scss Show resolved Hide resolved
Comment on lines -2112 to -2120
// fixes pointermove causing selection of UI texts #32
event.preventDefault();
// Preventing the event above disables default behavior
// of defocusing potentially focused element, which is what we
// want when clicking inside the canvas.
if (document.activeElement instanceof HTMLElement) {
document.activeElement.blur();
}

Copy link
Member

Choose a reason for hiding this comment

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

Just need to make sure nothing breaks due to this removal else pr looks good

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely do check. I wouldn't be surprised if something did :).

@ad1992
Copy link
Member

ad1992 commented Mar 14, 2021

I am facing some issues when trying type text, so pointer doesn't change to cursor with single click on canvas, i have to double click.
Excalidraw (1)
This works fine in prod.

@dwelle
Copy link
Member Author

dwelle commented Mar 16, 2021

I am facing some issues when trying type text, so pointer doesn't change to cursor with single click on canvas, i have to double click.

I "fixed" it by reintroducing the preventDefault in case of text, and manually blurring in any case. Both of these are pretty intertwined with the logic, some of which I'm not sure even how it works.

If we want to keep these hacks it'll mean that the case this PR was supposed to fix (by not preventing the default) will not be 100% fixed (due to the text handling) — well, I think it wouldn't be either way, coz we also prevent default on mobile (touch events) for various reasons.

It could in theory be fixed by delaying (into next event loop tick) binding the wysiwyg blur event.

Ended up hacking the wysiwyg initialization so that our blur event (for submitting the wysiwyg) is bound in a next event loop frame so as to (hopefully) guarantee it fires after the blur event from the pointerdown (which would submit the wysiwyg right away).

I've also made sure we select the element after submit only when confirming the submit via a button. This aligns it with production behavior without having to rely on the preventDefault hack (which was effectively overriding the appState.selectedElementIds with {} afterwards).

@dwelle
Copy link
Member Author

dwelle commented Mar 16, 2021

Breaks mobile. Probably related to the other preventDefault calls we do for touch events.

@dwelle
Copy link
Member Author

dwelle commented Mar 16, 2021

Removing the preventDefault in onTapEnd handler seems to have fixed the mobile issue (which was preventing wysiwyg submit when clicking outside).

@dwelle dwelle changed the title fix: stop preventing canvas pointerdown event fix: stop preventing canvas pointerdown/tapend events Mar 16, 2021
@dwelle dwelle merged commit e90e564 into master Mar 16, 2021
@dwelle dwelle deleted the dont_prevent_canvas_pointerdown branch March 16, 2021 17:04
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.

Can't delete a shape when iframe loses focus
4 participants