Fix unintentional hovering causing theme previews

[squarewave]

Closes #1634
Closes #1561

This fixes the first bug by requiring that the mouse has slowed below some threshold (10 pixels in 100 ms) before previewing the theme. It assumes that anything faster than that is just the user trying to mouse through the element, not them trying to hover over it.

It should fix the second bug by requiring that the mouse moves past some threshold (5 pixels) before previewing the theme. When we get a mouseover event simply because a video closes (and the mouse is sitting on top of the theme image), the user hasn't moved their mouse yet. Once they move their mouse, they'll likely be moving it faster than the first threshold, and so they won't accidentally see the theme preview.

I was a little shaky on the tests. They reliably pass for me, but I could see them potentially being flaky. Guidance / alternate solutions are very welcome!

[mstriemer]

Thanks for doing this!

I had looked for a react library for this before and didn't find anything. I did find a vanilla JS version of a jQuery plugin though https://github.com/tristen/hoverintent/blob/gh-pages/index.js. Your version looks much shorter than that which is great.

A couple things:

I noticed the other library uses Math.abs() instead of squaring the mouse position. Does this accomplish the same thing or is there some subtle difference?

Also, it would be nice to get this code out of the component itself so it can be reused and make the component a bit clearer. I'm not sure the best way to do this but if you've got ideas I'd love to hear them.

I came up with two ways, neither of which I'm totally thrilled about, emitting a wrapper div to attach listeners to and an HOC that accepts callbacks at the module level. I put some examples below.

---

Attach the onIntentStart and onIntentEnd handlers in the HoverIntent component which would render to something like <div onMouseOver={this.maybeHoverIntent} {...moreHandlers}>{children}</div>.

render() {
  return (
    <div>
      <HoverIntent
        sensitivity={5} timeout={100}
        onIntentStart={this.previewTheme} onIntentEnd={this.resetPreview}>
        <img src={this.props.previewImg} />
      </HoverIntent>
    </div>
  );
}

---

A more standard HOC would work too but the callbacks would need to be defined at the module level. I think this would be fine for our use case but is a little limiting. You'd also need to define a component to wrap this with which isn't ideal.

function onHoverIntent(target) {
  previewTheme(target.dataset.browserTheme);
}

function onHoverIntentEnd() {
  resetPreviewTheme();
}

export default compose(
  withHoverIntent({
    onHoverIntent,
    onHoverIntentEnd,
    sensitivity: 5, // These can of course be defaults.
    timeout: 100,
  }),
)(MyComponent);

---

If you have time to extract this code I'd be happy to look it over. This looks like a good solution regardless, though. Thanks!

[squarewave]

Hmm, what about something like this (just avoiding wrapping the child in a div):

import React, { PropTypes } from 'react';

export default class HoverIntent extends React.Component {
  static propTypes = {
    sensitivity: PropTypes.number,
    interval: PropTypes.number,
    onHoverIntent: PropTypes.func.isRequired,
    onHoverIntentEnd: PropTypes.func.isRequired,
  }

  static defaultProps = {
    sensitivity: 5,
    interval: 100,
  }

  componentWillUnmount() {
    this.clearHoverIntentDetection();
  }

  clearHoverIntentDetection() {
    clearInterval(this.interval);
  }

  onMouseOver = (e) => {
    e.persist();
    const currentTarget = e.currentTarget;

    const sq = (x) => x * x;
    const distanceSq = (p1, p2) => sq(p1.x - p2.x) + sq(p1.y - p2.y);

    const sensitivitySq = sq(this.props.sensitivity);

    const initialPosition = { x: e.clientX, y: e.clientY };
    let previousPosition = initialPosition;
    this.currentMousePosition = initialPosition;

    this.interval = setInterval(() => {
      const currentPosition = this.currentMousePosition;
      if (distanceSq(initialPosition, currentPosition) > sensitivitySq &&
        distanceSq(previousPosition, currentPosition) < sensitivitySq) {
        this.clearHoverIntentDetection();

        // construct a fake event cloned from e, but with e's initial currentTarget
        // (currentTarget will change as the event bubbles up the DOM)
        this.props.onHoverIntent(Object.assign({}, e, {currentTarget}));
      }

      previousPosition = currentPosition;
    }, this.props.interval);
  }

  onMouseOut = (e) => {
    this.clearHoverIntentDetection();
    this.props.onHoverIntentEnd(e);
  }

  onMouseMove = (e) => {
    this.currentMousePosition = { x: e.clientX, y: e.clientY };
  }

  render() {
    const child = React.Children.only(this.props.children);

    return React.cloneElement(child, {
      onMouseOver: this.onMouseOver,
      onMouseOut: this.onMouseOut,
      onMouseMove: this.onMouseMove,
    });
  }
}

I'm not sure if the div wrapping is what you didn't like about that first solution. This solution is a little funky since we overwrite the existing handlers on the child element (if it has any), but it's a tiny bit cleaner for inline elements?

Regarding squaring the distance, this is just for correctness since distance(a, b) = sqrt((a.x - b.x)^2 + (a.y - b.y)^2). Comparing to distanceSq rather than distance is just a tiny optimization to avoid a sqrt. The script you linked is kind of funky in that it uses sensitivity to make a bounding box rather than a bounding circle.

[mstriemer]

Regarding squaring the distance, this is just for correctness since distance(a, b) = sqrt(a^2 + b^2).

Ah, of course. Thinking the number through with Math.abs() would mean less actual distance needed to trigger the sensitivity while moving diagonally.

Using React.cloneElement() looks good to me. I didn't know about that but it's exactly what I wanted.

[squarewave]

I think this is ready to be reviewed? Also, I don't seem to be able to request a review - is that expected / is there a permission controlling that?

[tofumatt]

It could be possible that to request a review you need write access to the repo. :-(

[mstriemer]

The HoverIntent component looks really good!

I'm requesting some changes for the tests since I think they could be done without the timeouts. These few tests would be quite slow and could cause intermittent failures so if we can make them synchronous I'd really like that.

Sorry for the delayed review, was out of town over the weekend and Monday.

[mstriemer]

I thought e.persist() would take it out of react's pool of event objects. Wouldn't that prevent any modification to the object or am I misunderstanding what this is trying to prevent.

[squarewave]

e.persist() will remove it from React's event pool so that it won't be reused for separate events, but e.currentTarget is a bit of a special beast, because it changes for this event as it bubbles up the DOM. If we don't do this, currentTarget ends up being null, since it has bubbled all the way up and then been cleared.

[mstriemer]

Can you throw here if child.props has onMouseOver, onMouseOut or onMouseMove already, please?

[mstriemer]

Is this definitely needed? If components aren't being unmounted we should probably solve this for all of our tests.

[squarewave]

AFAIK usually components don't need to be unmounted in tests. renderIntoDocument doesn't actually render them into a document, it just creates a div and renders into that (https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L128). This means that normally, it all just gets garbage collected and we don't have to worry about it. However the HoverIntent component creates a closure attached to a setInterval, and so it can't be GC'd.

All of that being said, I think I was reacting to a boogeyman - I got an out of memory error after running npm run unittest:dev for ~30 minutes, and I thought it was due to setInterval leaking, but I can't seem to reproduce this. I don't know enough about Karma's internals, but everything does seem to get destroyed after the tests run.

[mstriemer]

The out of memory thing is a known issue unfortunately. We're not really sure what causes it.

It might be a good idea to do this in our test setup script. I guess the componentWillUnmount callback isn't triggered when the element gets garbage collected?

We could have other components that are leaking memory, but that might not matter much with karma's architecture. I think it's the node part that keeps crashing, I believe each test suite run is in an iframe that gets removed after completion.

[mstriemer]

These two functions could be moved above the component.

[mstriemer]

If you move the closure into another function you should be able to test this synchronously.

onMouseOver = (e) => {
  setInterval(this.createHoverIntentDetector(e), this.props.interval);
}

createHoverIntentDetector(e) {
  e.persist();
  const { currentTarget } = e;

  const sensitivitySq = sq(this.props.sensitivity);
  const initialPosition = { x: e.clientX, y: e.clientY };
  let previousPosition = initialPosition;
  this.currentMousePosition = initialPosition;

  return () => {
    if (distanceSq(initialPosition, currentPosition) > sensitivitySq &&
      distanceSq(previousPosition, currentPosition) < sensitivitySq) {
      this.clearHoverIntentDetection();
      this.isHoverIntended = true; 
      this.props.onHoverIntent(Object.assign({}, e, { currentTarget }));
    }
    previousPosition = currentPosition;
  }
}

it('triggers when the mouse slows down', () => {
  const e = { currentTarget, persist, x: 1, y: 2 };
  hoverIntentDetector = root.createHoverIntentDetector(e);
  Simulate.mouseMove({ x: 7, y: 7 }); // Moved too fast, not hover intent
  hoverIntentDetector();
  assert.isNotOk(props.hoverIntent.called);
  Simulate.mouseMove({ x: 8, y: 8 }); // Only moved a little, hover intent
  hoverIntentDetector();
  assert.ok(props.hoverIntent.called);
  
})

[squarewave]

Good idea! I'll do that :)

[squarewave]

So, it turns out sinon has methods for faking timers. I went with those because it made it simpler to keep the coverage at 100%. Does that seem reasonable?

[mstriemer]

Can you assert after each tick? I think this test would pass if it was just mapping onHoverIntent to onHover.

const { onHoverIntent, onHoverIntentEnd } = props;
Simulate.mouseOver(innerElement, { clientX: 1, clientY: 2 });
clock.tick(75);
assert.isNotOk(onHoverIntent.called);

Simulate.mouseMove(innerElement, { clientX: 10, clientY: 0 });
clock.tick(50);
assert.ok(onHoverIntent.calledOnce);

Simulate.mouseMove(innerElement, { clientX: 10, clientY: 1 });
clock.tick(50);
assert.ok(onHoverIntent.calledOnce);
assert.isNotOk(onHoverIntentEnd.called);

Simulate.mouseOut(innerElement);
assert.ok(onHoverIntent.calledOnce);
assert.ok(onHoverIntentEnd.calledOnce);

[mstriemer]

And the same for the other tests, whichever assertions seem reasonable.

[mstriemer]

Thanks for moving it to the createHoverIntentDetector version, I think that makes following the code much nicer.

Using the sinon fake timers looks good to me. It shows the intent of what's happening better than calling detectHoverIntent() directly.

I think the tests should be more specific but it all looks good after that.

[mstriemer]

Looks great, thanks for doing this!

[squarewave]

Of course! Thank you for the reviews!

[muffinresearch]

@squarewave thanks very much for contributing this, it's much appreciated!