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 incorrect calculation of onMouseEnter/Leave component path #11164

Merged
merged 4 commits into from
Oct 9, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 9, 2017

Fixes #10906 and #11152.
We forgot to compare alternates so in some cases, the from/to path was calculated incorrectly.

See individual commits. The first one adds a test, the last one fixes the bug. The ones in the middle prepare the code to look readable after the bugfix.

I verified both #10906 and #11152 reproduced before, but no longer reproduce after the bugfix.

This will be easier to follow when we add more code there.
So that I can add a block scoped variable.
while (from && from !== common) {
const common = from && to ? getLowestCommonAncestor(from, to) : null;
const pathFrom = [];
while (true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We try to avoid while (true) since (at least in older versions) it has been known to deopt.

I appreciate the rewrite for the temp variable and easy of read. But arguably this just make the condition seem a lot more complicated than it really is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't it deopt when you have continues rather than break? I don't remember. @trueadm

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Nit: I think I prefer the other writing style, even if that includes a temporary variable assignment inline in the condition.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 9, 2017

Like this?

  while (from && from !== common && (!from.alternate && from.alternate !== common)) {

I'm not sure how else to write it in a single line. I’m finding this really hard to follow, especially because the null check (since common can be null) complicates the condition.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 9, 2017

Or, rather,

  while (from && from !== common && (!from.alternate || from.alternate !== common)) {

See, I already made a mistake there. 😛

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 9, 2017

Do you like this?

  while (from && (!common || (from !== common && from.alternate !== common))) {

If we check for common first then we shouldn't need to check alternate there.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 9, 2017

Meh. I find all of these hard to follow and I don't know if they're correct.
If you feel strongly let me know what style you prefer and I'll rewrite in a separate PR.

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.

In React 16, onMouseEnter is triggered an extra time when entering a new child
3 participants