Remove faulty logic in ScrollView

[Saadnajmi]

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

RCTScrollView forwards some of its methods to its underlying _scrollView of type RCTCustomScrollView. Some of these always return YES, instead of properly implementing the logic. Let's remove the faulty forwarding, so that instead inherited superclass implementation (RCTView) is used.

Background

Normally, you expect a 1:1 mapping between the JS and native component (e.g: <View> maps to RCTView). There are notable exceptions, like <ScrollView>.

<ScrollView>maps to RCTScrollView which is a subclass of RCTView, not UIScrollView / NSScrollView. So how does RCTScrollView do its scrolling logic if the underlying component is just a subclass of RCTView? It turns out,RCTScrollView contains a member variable _scrollView (the real NSScrollView / UIScrollView). It then forwards appropriate methods / variables to the underlying "real" _scrollView.

(Side note): That's a little simplified. The real inheritance / ownership relationship looks like:

RCTScrollView **has-a** RCTCustomScrollView 
RCTCustomScrollView **is-a** RCTUIScrollView 
RCTUIScrollView  **is-a** UIScrollView (iOS) / NSScrollView (macOS)

The issue is that in the case of accepting/forwarding first responder status, this didn't work. RCTScrollView would forward the calls to canBecomeFirstResponder / becomeFirstResponder / resignFirstResponder to it's underlying _scrollView, which were overridden to always YES. This is bad because:

1. The _scrollView always accepted first responder status, even when its outer RCTScrollView didn't want it (e.g: focusable={false}). This blocked FocusZone (macOS): move focus between first responders instead of key views fluentui-react-native#2329 , because the _scrollView would receive focus even when JS told it to not to.

2. Even though RCTScrollView forwards the call to becomeFirstResponder to its _scrollView, that didn't actually cause _scrollView to become first responder. It would just return YES, and Appkit would proceed with making the outer RCTScrollView first responder anyway.

I see an issue that _scrollView never actually becomes first responder, but instead an NSView wrapping it does. However, It think that's a much bigger fix and, the outer view managing first responder status has worked in production for a while. Let's just remove the faulty logic implying the inner _scrollView does get first responder status for now.

Changelog

[macOS] [Fixed] - Don't override responder methods in ScrollView

Test Plan

I tested the ScrollViewSimpleExample, ScrollView, and Flatlist test pages to make sure that the key view loop was the same and that you could still tab to a ScrollView and scroll it with PageUp/Down.

Video of Flatlist test page:

Screen.Recording.2022-11-14.at.3.39.59.PM.mov

[chiuam]

If it's feasible, should we forward the overrides to the scrollview's subviews?

[Saadnajmi]

If it's feasible, should we forward the overrides to the scrollview's subviews?

Which subviews?