Duplicate menu open

[ladifire]

🐛 Bug Report

When fast click two menu triggers, they both displayed.

🤔 Expected Behavior

preview video

[devongovett]

Hmm not sure what's going on here. Maybe something transition animation related? Would need to investigate more.

[snowystinger]

Looking at this, the easiest way to repro it so far is set the Transition timeout to something pretty long, 3500 for instance.
Press on one of the buttons, staying pressed, move to second button. Release. Press second button.
If this is a cold start, repeat once more.

Working on a test for it, general idea is something like this:

  function pointerEvent(type, opts) {
    let evt = new Event(type, {bubbles: true, cancelable: true});
    Object.assign(evt, {
      ctrlKey: false,
      metaKey: false,
      shiftKey: false,
      button: opts.button || 0
    }, opts);
    return evt;
  }

  it.only('should always close the menu when outside things are clicked', function () {
    let menuTrigger1Ref = React.createRef();
    let button1Ref = React.createRef();
    let menuTrigger2Ref = React.createRef();
    let button2Ref = React.createRef();
    global.PointerEvent = {};
    let {getByLabelText, getByText} = render(
      <Provider theme={theme}>
        <MenuTrigger ref={menuTrigger1Ref}>
          <Button ref={button1Ref} aria-label="button1"
                  onPressStart={() => console.log('1onPressStart')}
                  onPressEnd={() => console.log('1onPressEnd')}
                  onPress={() => console.log('1onPress')} />
          <Menu items={withSection}>
            {item => (
              <Section key={item.name} items={item.children} title={item.name}>
                {item => <Item key={item.name} childItems={item.children}>{item.name}</Item>}
              </Section>
            )}
          </Menu>
        </MenuTrigger>
        <MenuTrigger ref={menuTrigger2Ref}>
          <Button ref={button2Ref} aria-label="button2"
                  onPressStart={() => console.log('2onPressStart')}
                  onPressEnd={() => console.log('2onPressEnd')}
                  onPress={() => console.log('2onPress')} />
          <Menu aria-label="foo" selectedKeys={['alpha']}>
            <Item key="alpha">Alpha</Item>
            <Item key="bravo">Bravo</Item>
          </Menu>
        </MenuTrigger>
      </Provider>
    );
    let button1 = getByLabelText('button1');
    let button2 = getByLabelText('button2');
    button1.getBoundingClientRect = jest.fn(() => ({
      left: 0,
      right: 5,
      top: 0,
      bottom: 5
    }));
    button2.getBoundingClientRect = jest.fn(() => ({
      left: 6,
      right: 10,
      top: 0,
      bottom: 5
    }));

    fireEvent(button1, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 1, clientY: 1}));
    fireEvent(button1, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse'}));
    expect(getByText('Heading 1')).toBeVisible();
    expect(() => getByText('Alpha')).toThrow();
    act(() => {jest.advanceTimersByTime(10);});
    fireEvent(button1, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    fireEvent(button2, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    fireEvent(button2, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse'}));
    fireEvent(button2, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    fireEvent(button2, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    act(() => {jest.advanceTimersByTime(10);}); // don't run full amount of time, we want 1 to be still transitioning out
    expect(getByText('Heading 1')).toBeVisible();
    expect(getByText('Alpha')).toBeVisible();

    act(() => {jest.advanceTimersByTime(10);});
    fireEvent(button1, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 1, clientY: 1}));
    fireEvent(button1, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse'}));
    act(() => {jest.advanceTimersByTime(10);});
    fireEvent(button1, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    expect(getByText('Heading 1')).toBeVisible();
    expect(getByText('Alpha')).toBeVisible();
    fireEvent(button2, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    fireEvent(button2, pointerEvent('pointerdown', {pointerId: 1, pointerType: 'mouse'}));
    fireEvent(button2, pointerEvent('pointermove', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    fireEvent(button2, pointerEvent('pointerup', {pointerId: 1, pointerType: 'mouse', clientX: 7, clientY: 1}));
    act(() => {jest.advanceTimersByTime(10);});
    expect(getByText('Alpha')).toBeVisible();
    expect(getByText('Heading 1')).toBeVisible();
    act(() => {jest.advanceTimersByTime(700);});
    expect(getByText('Alpha')).toBeVisible();
    expect(() => getByText('Heading 1')).toThrow();
  });

It's a little gross, but the events line up now for the most part. I'll have to think of some nice utilities for this in the future :)
There's a onMouseUp that fires in jest that doesn't fire in the browser. I haven't figured out where that's coming from yet

[ladifire]

It should listen for any click outside and close menu immediately.

[snowystinger]

Update, we've made it further and the general direction we're headed is that onInteractOutside should fire on mouse down, instead of on 'up'.
should? onInteractOutside should listen for the capture phase of the event so that usePress on a button doesn't prevent the event from reaching onInteractOutside. (currently not prevented anyways, see next comment)
Also, we should only pass onInteractOutside prop in useOverlay when it isOpen.

Now we have a case where the first click of Menu opens it
second click will then close it (on mouse down from onInteractOutside closes) followed immediately by a new open from the trigger handling the same on mouse down.

Missing event from above test potentially related to missing focus/blur events.

[snowystinger]

Next update, we've been hit by React Synthetic events.
usePress will stopPropagation, but it's only on the synthetic event, so onInteractOutside still gets that event.
Now we need to figure out how to best deal with this. It seems like it'd be bad to stop(Immediate)Propagation on all native events in usePress.
Should clicking the trigger of a menu count as an outside interaction?

[ladifire]

It's still duplicate open if we press (Click and hold, don't release mouse button) on 2nd menu button (while 1st menu opened).
I think it's better handle onPressEnd instate onPressStart event for menu trigger.

[snowystinger]

I'm unable to reproduce with your instructions, however, I was able to reproduce today with a double Picker. It's much harder now though. We'll keep looking into it, you're welcome to take a go as well :)
Were you trying with out latest code? or are you using what we've released to npm?

[ladifire]

@snowystinger I'm using latest code on github, not npm release version!
It happend when:
Step 1: Click "Menu Button 1" to open menu 1
Step 2: Click and press "Menu Button 2" (Don't mouse up) to open menu 2

[snowystinger]

Thanks for clarifying the version you're using.
Can you provide more information on the system / browser that you're using? As I'm still unable to reproduce with your method. Right now, my method is a lot of rapid fire clicking, which is not great for trying to chase down the cause of the bug and I'd love to be able to reliably reproduce it with your steps. Also, if you could share your story setup?