react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

Fix ios is over target

Open stpnvntn opened this issue 4 years ago • 4 comments

Closes https://github.com/adobe/react-spectrum/issues/1807

It seems like we can't reuse state.target in isOverTarget in different handlers since target.getBoundingClientRect() can return different values. So I save the original target.getBoundingClientRect() to state.targetRect instead.

Here is how I understand what happens:

  • onPointerDown
    • save e.currentTarget to state.target
    • add onPointerUp as global listener
  • pressProps.onPointerUp
    • "Safari on iOS sometimes fires pointerup events, even when the touch isn't over the target, so double check. No sure that changes we need to use state.targetRect here
  • onPointerUp
    • checking isOverTarget the main problem is here: the coordinates of the state.target might be changed. That is why we are I using state.targetRect here
    • call triggerPressEnd with 3rd arg (wasPressed) depends on isOverTarget result
    • triggerPressEnd
      • calls onPress if wasPressed is true

✅ Pull Request Checklist:

  • [x] Included link to corresponding React Spectrum GitHub Issue.
  • [ ] Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • [x] Filled out test instructions.
  • [ ] Updated documentation (if it already exists for this component).
  • [ ] Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

I was testing this using storybook. Add the following component to e.g. Button.stories.tsx.

function ButtonAlongsideWithInputAndAdditionalContent(props: any = {}) {
  return <Flex direction="column">
    <Flex height="size-2000">
      <Text>Content</Text>
    </Flex>
    <TextField name="field" label="Field" placeholder="placeholder" />
    <Button onPress={() => alert('pressed')}>
        Button
      </Button>
  </Flex>
}

Also, I've left console.log if the first commit so it might be used for debugging.

🧢 Your Project:

n/a

stpnvntn avatar Apr 20 '21 15:04 stpnvntn

I'm not sure that this will be correct in all cases. If a button moved between mouse/touch down and up I don't think I would expect the press event to fire, because the user's pointer is not over the target anymore.

I think perhaps this is more caused by the fact that we trigger focus to the button on touch down rather than touch up. Moving focus is what causes the keyboard to hide and the page to scroll. You can see this in your demo - touching the button without aria does not hide the keyboard until touch up, whereas the aria button it hides on touch down. Perhaps we can improve our handling there, but it will require some testing to ensure we don't break anything.

devongovett avatar Apr 22 '21 17:04 devongovett

@devongovett are you saying we need to change the handling or you are going to handle that on you side? We have the bug related to this behaviour and just trying to understand how could we move forward with that issue

aaliakseyenka avatar Apr 22 '21 17:04 aaliakseyenka

Either way! If you'd like to give it a try, that would be great. Otherwise, we'll look at it at some point, but I can't guarantee when.

In the meantime you may be able to use the preventFocusOnPress option to work around this. You'll need to manually focus the button in the onPress event so it receives focus on press up rather than press down.

devongovett avatar Apr 22 '21 17:04 devongovett

@devongovett thanks for workaround. We decided to go this way for now

aaliakseyenka avatar May 06 '21 10:05 aaliakseyenka

Closing this for now since there is a work around. Thanks for the input!

dannify avatar Oct 06 '22 05:10 dannify