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

[Flare] Ensure mouse events can use target to validate press #16172

Merged
merged 1 commit into from
Jul 22, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jul 22, 2019

This PR fixes an interesting bug that we encountered internally using the Press responder. When using mouse events (via pointer events or mouse events), we have a guarantee that the event target will be that of the one we're moving/releasing from (unlike touch events, which can have the same target as when the press was initiated). This mechanic for mouse events lets us bypass the getBoundingClientRect logic, which is:

  • expensive as it requires a layout reflow
  • can be incorrect in cases where children of the responderTarget are absolutely positioned
  • can sometimes report width and/or height as 0 when some CSS styles are applied

This behaviour already existed in the current Press implementation. But there were two bugs:

  • isTargetWithinNode wasn't taking into account the alternate fiber
  • we weren't setting isPressWithinResponderRegion to true if we knew the mouse event target was in the responder region (by using the targets).

This fixes both cases, which also fixes the internal bug we were seeing.

@sizebot
Copy link

sizebot commented Jul 22, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@trueadm trueadm merged commit 783b8f4 into facebook:master Jul 22, 2019
@trueadm trueadm deleted the mouse-validate-press branch July 22, 2019 11:31
);
if (pressTarget !== null && !targetIsDocument(pressTarget)) {
if (
pointerType === 'mouse' &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this checking for only mouse pointers?

Copy link
Contributor Author

@trueadm trueadm Jul 22, 2019

Choose a reason for hiding this comment

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

That's what it was doing before. Touch event pointers have the same target until released, so if you remove the pointer type check, pointer events never deactivate, even if you move outside the target.

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

Successfully merging this pull request may close these issues.

None yet

5 participants