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

Pull in upstream fixes to expose hover props on Pressable #884

Merged
merged 21 commits into from
Dec 7, 2021

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Nov 4, 2021

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

In facebook#32405 , I exposed the props onHoverIn / onHoverOut on Pressable. This change brings the same

Changelog

[General] [Fixed] - Pressable not passing hover props and event handlers to PressabilityConfig

Test Plan

Updated pressable test page in rn-tester

Saadnajmi and others added 18 commits March 22, 2021 14:59
[pull] master from microsoft:master
#2)

Co-authored-by: Ryan Linton <ryanlntn@gmail.com>
[pull] master from microsoft:master
[pull] master from microsoft:master
* Deprecated api (microsoft#853)

* Remove deprecated/unused context param
* Update a few Mac deprecated APIs

* Packing RN dependencies, hermes and ignoring javadoc failure,  (microsoft#852)

* Ignore javadoc failure

* Bringing few more changes from 0.63-stable

* Fixing a patch in engine selection

* Fixing a patch in nuget spec

* Fixing the output directory of nuget pack

* Packaging dependencies in the nuget

* Fix onMouseEnter/onMouseLeave callbacks not firing on Pressable (microsoft#855)

* add pull yml

* match handleOpenURLNotification event payload with iOS (microsoft#755) (#2)

Co-authored-by: Ryan Linton <ryanlntn@gmail.com>

* fix mouse evetns on pressable

* delete extra yml from this branch

* Add macOS tags

* reorder props to have onMouseEnter/onMouseLeave always be before onPress

Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com>
Co-authored-by: Ryan Linton <ryanlntn@gmail.com>

* Grammar fixes. (microsoft#856)

Updates simple grammar issues.

Co-authored-by: Nick Trescases <42704557+ntre@users.noreply.github.com>
Co-authored-by: Anandraj <anandrag@microsoft.com>
Co-authored-by: Saad Najmi <saadnajmi2@gmail.com>
Co-authored-by: pull[bot] <39814207+pull[bot]@users.noreply.github.com>
Co-authored-by: Ryan Linton <ryanlntn@gmail.com>
Co-authored-by: Muhammad Hamza Zaman <mh.zaman.4069@gmail.com>
Summary:
Several desktop forks (`react-native-macos`, `react-native-windows`, `react-native-web`) support mouse events, and while the stock Pressable component has the ability to support mouse events, it seems we aren't forwarding some props properly from Pressable -> Pressability.

Pressability will calculate onMouseEnter / onMouseLeave event handlers based on the `onHoverIn/onHoverOut` callbacks passed into PressabilityConfig.
https://github.com/facebook/react-native/blob/ad0d4534a751ed05f84ff971714c8f7a4d1deb3a/Libraries/Pressability/Pressability.js#L552
 However, Pressable does not pass take in onHoverIn/onHoverOut props to pass to PressabilityConfig, so we can't take advantage of this functionality. This change should simply address that by passing the props through.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Pressabel not passing hover props and event handlers to PressabilityConfig

Pull Request resolved: facebook#32405

Test Plan: I fixed a similar issue in `react-native-macos` that I am now trying to contribute back upstream. microsoft#855

Reviewed By: yungsters

Differential Revision: D31667737

Pulled By: sota000

fbshipit-source-id: f0bbe48302703bb2c45280d2afeec8d7a4586b6a
@Saadnajmi Saadnajmi requested a review from alloy as a code owner November 4, 2021 20:12
@Saadnajmi Saadnajmi changed the title Pull in upstream fixes to expose Hover props on Pressable Pull in upstream fixes to expose hover props on Pressable Nov 4, 2021
@rectified95
Copy link

I know this is already upstream, but I'm wondering how onHoverin/out is different from mouseEnter/Leave

@Saadnajmi
Copy link
Collaborator Author

I know this is already upstream, but I'm wondering how onHoverin/out is different from mouseEnter/Leave

I think the name difference is Microsoft and Facebook independently adding mouse support for different scenarios:
Facebook for react-native-web, Microsoft for our desktop forks. Poking around in react-native-windows, it looks like we added onMouseEnter/onMouseLeave, then when Facebook added onHoverIn/onHoverOut , rn-windows decided to support both in Pressability. For my FHL a few weeks ago, I was trying to figure out how to align the hover props across RN Core / windows / macOS, and I mostly landed on just following facebooks' onHoverIn & onHoverOut naming and making sure we followed that for rn-macos

@acoates-ms
Copy link
Collaborator

Its interesting that Facebook went with a different naming scheme than the web APIs, which is what we copied the onMouseEnter API after. Having said that, I do prefer the non input specific API. -- Its more like what we moved to in windows with pointerEntered.

@Saadnajmi
Copy link
Collaborator Author

Its interesting that Facebook went with a different naming scheme than the web APIs, which is what we copied the onMouseEnter API after. Having said that, I do prefer the non input specific API. -- Its more like what we moved to in windows with pointerEntered.

I think it's also something to do with Pressable trying to be a little more platform agnostic than View, At the end of the day Pressable just wraps a View component, and forwards the onHoverIn event handler as an onMouseEnter event handler to View. The benefit of Pressable is it handles extra details like delayHoverIn / whatever else.

@acoates-ms
Copy link
Collaborator

Yup, my guess is that a gaze gesture in VR might trigger onHoverIn etc. This is why I like the input agnostic naming. The web APIs were designed a long time ago back before computers had so many input modalities.

@harrieshin harrieshin merged commit f8e26e3 into microsoft:master Dec 7, 2021
harrieshin pushed a commit that referenced this pull request Dec 7, 2021
…#887)

* Expose Pressability Hover config props in Pressable (facebook#32405)

Summary:
Several desktop forks (`react-native-macos`, `react-native-windows`, `react-native-web`) support mouse events, and while the stock Pressable component has the ability to support mouse events, it seems we aren't forwarding some props properly from Pressable -> Pressability.

Pressability will calculate onMouseEnter / onMouseLeave event handlers based on the `onHoverIn/onHoverOut` callbacks passed into PressabilityConfig.
https://github.com/facebook/react-native/blob/ad0d4534a751ed05f84ff971714c8f7a4d1deb3a/Libraries/Pressability/Pressability.js#L552
 However, Pressable does not pass take in onHoverIn/onHoverOut props to pass to PressabilityConfig, so we can't take advantage of this functionality. This change should simply address that by passing the props through.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Pressabel not passing hover props and event handlers to PressabilityConfig

Pull Request resolved: facebook#32405

Test Plan: I fixed a similar issue in `react-native-macos` that I am now trying to contribute back upstream. #855

Reviewed By: yungsters

Differential Revision: D31667737

Pulled By: sota000

fbshipit-source-id: f0bbe48302703bb2c45280d2afeec8d7a4586b6a

* Additional changes

* undo extra unnecessary changes

* remove another line

* update podfile lock
harrieshin pushed a commit that referenced this pull request Dec 7, 2021
…#923)

* Expose Pressability Hover config props in Pressable (facebook#32405)

Summary:
Several desktop forks (`react-native-macos`, `react-native-windows`, `react-native-web`) support mouse events, and while the stock Pressable component has the ability to support mouse events, it seems we aren't forwarding some props properly from Pressable -> Pressability.

Pressability will calculate onMouseEnter / onMouseLeave event handlers based on the `onHoverIn/onHoverOut` callbacks passed into PressabilityConfig.
https://github.com/facebook/react-native/blob/ad0d4534a751ed05f84ff971714c8f7a4d1deb3a/Libraries/Pressability/Pressability.js#L552
 However, Pressable does not pass take in onHoverIn/onHoverOut props to pass to PressabilityConfig, so we can't take advantage of this functionality. This change should simply address that by passing the props through.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Pressabel not passing hover props and event handlers to PressabilityConfig

Pull Request resolved: facebook#32405

Test Plan: I fixed a similar issue in `react-native-macos` that I am now trying to contribute back upstream. #855

Reviewed By: yungsters

Differential Revision: D31667737

Pulled By: sota000

fbshipit-source-id: f0bbe48302703bb2c45280d2afeec8d7a4586b6a

* Add back macOS test

* update podfile

* remove unnecessary line

* update podfile
@Saadnajmi Saadnajmi deleted the upstream-pressable-hover branch December 7, 2021 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants