Summary:
FlatList relies heavily on onScroll events + the measure API. In Fabric, usage of `measure` relies on C++ having an accurate view of the current scroll position of the ScrollView.
We already have a mechanism for updating the scroll position in C++ using UpdateState. But, it is only used currently at the /beginning/ and /end/ of scrolling, and UpdateState is not called /during/ scrolling at all.
This means that we will see a series of events like this while scrolling:
```
Scrolling begins
UPDATE C++ STATE: scrollLeft = 0, scrollTop = 0
JS event: onScroll x=0, y=0
JS event: onScroll x=0, y=100
JS event: onScroll x=0, y=200
...
JS event: onScroll x=0, y=1000
UPDATE C++ STATE: scrollLeft = 0, scrollTop = 1000
```
Notably, not many C++ state updates are queued; and the last one is queued AFTER the JS event is sent. The last JS event and UpdateState will race, which means that sometimes the C++ update will /lose/ and C++ will have an inaccurate view of the world when FlatList receives the onScroll event and calls `measure`.
My proposed solution, gated behind a feature flag, is to delay /some/ onScroll events until the C++ UpdateState has made its way back to Java, and send UpdateStates more frequently. The balance here is that UpdateState is a relatively expensive operation, so we probably still want to call it /less/ than we call onScroll. This means that `measure` will still return some incorrect results but will return correct results more frequently. Win?
Changelog: [Internal[
Reviewed By: mdvacca
Differential Revision: D28558380
fbshipit-source-id: 11c7cd714fae67ee5a94c4413be988481413ec03
Summary:
This diff is a follow up of D28360679 (https://github.com/facebook/react-native/commit/e3367354cc93f17b830ed8dc601dff5e87748dd1), here we refactor the access of ReactFeatureFlags from C++ to use methods instead of fields
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28362066
fbshipit-source-id: caed5e7fddeb6c0d9846fb037152befa8f1ed5c2
Summary:
Support ScrollAway in ReactScrollView for Fabric/non-Fabric.
Changelog: [Android][Added] Support for ScrollAway native nav bars added to ReactScrollView
Reviewed By: mdvacca
Differential Revision: D28308855
fbshipit-source-id: 9a922159ef50fb7c8e9c484a4b97ca57ab248496
Summary:
This diff ensures that the dispatch of switch events is performed using the proper UIModule
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28204930
fbshipit-source-id: 625b536ab3106efa7dbf583589dfe268b880a6a0
Summary:
This issue fixes https://github.com/facebook/react-native/issues/30935 screenreader does not announce Image disabled accessibilityState.
As stated in AOSP View.java, the framework will handle routine focus movement, views indicate their willingness to take focus through the `isFocusable` method https://bit.ly/3dCnyHb
```
* <p>The framework will handle routine focus movement in response to user input. This includes
* changing the focus as views are removed or hidden, or as new views become available. Views
* indicate their willingness to take focus through the {link #isFocusable} method. To change
* whether a view can take focus, call {link #setFocusable(boolean)}.
```
The property is updated through its shadow node `ReactImageManager` method `setAccessible` https://bit.ly/3dDuK5L
```java
* <p>Instances of this class receive property updates from JS via @{link UIManagerModule}.
* Subclasses may use {link #updateShadowNode} to persist some of the updated fields in the node
* instance that corresponds to a particular view type.
```
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->
[Android] [Fixed] - adding setAccessible to ReactImageManager to allow screenreader announce Image accessibilityState of "disabled"
Pull Request resolved: https://github.com/facebook/react-native/pull/31252
Test Plan:
**<details><summary>CLICK TO OPEN TESTS RESULTS</summary>**
<p>
Enable audio to hear the screenreader
TEST SCENARIO
- The user moves the screenreader focus to an image and the screenreader reads the Image accessibilityLabel "plain network image"
RESULT
- The screenreader announces the accessibilityState disabled after reading the Image accessibilityLabel "plain network image"
```javascript
<Image
accessible={true}
accessibilityLabel="plain network image"
accessibilityState={{disabled: true}}
source={fullImage}
style={styles.base}
/>
```
<video src="https://user-images.githubusercontent.com/24992535/112670432-2f366d00-8e61-11eb-843f-4b56f4a06a91.mp4" width="700" />
</p>
</details>
Reviewed By: kacieb
Differential Revision: D28194597
Pulled By: lunaleaps
fbshipit-source-id: 5f89ce5c714405506261885ac6fea2c15c2e1f23
Summary: Changelog: [Fabric][iOS][Fix] Remove use of bridge from Modal by dismissing Modal with visible prop
Reviewed By: sammy-SC
Differential Revision: D28074326
fbshipit-source-id: 0278bfb031db802b59429c553ac62d83838f4cc9
Summary:
This diff fixes an IllegalArgumentException that's thrown when creating layout with negative width.
This is not a new bug, but it started firing recently (probably caused by a change in text being measured)
stacktrace:
```
stack_trace: java.lang.IllegalArgumentException: Layout: -2 < 0
at android.text.Layout.<init>(Layout.java:265)
at android.text.Layout.<init>(Layout.java:241)
at android.text.BoringLayout.<init>(BoringLayout.java:179)
at android.text.BoringLayout.make(BoringLayout.java:61)
at com.facebook.react.views.text.TextLayoutManager.createLayout(TextLayoutManager.java:290)
at com.facebook.react.views.text.TextLayoutManager.measureText(TextLayoutManager.java:384) [inlined]
at com.facebook.react.views.text.ReactTextViewManager.measure(ReactTextViewManager.java:172) [inlined]
at com.facebook.react.fabric.mounting.MountingManager.measure(MountingManager.java:349) [inlined]
at com.facebook.react.fabric.FabricUIManager.measure(FabricUIManager.java:461)
at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at java.lang.Thread.run(Thread.java:923)
```
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D28015820
fbshipit-source-id: 129cd2a4c492d95d57fcdf3883b967a0b5df639a
Summary:
D26581756 (https://github.com/facebook/react-native/commit/86321a35c03df340b6c1ea740ea7f29e2659b0ab) was hacked for fixing T85822390, which was later properly fixed in D27369721, so I removed it in this diff.
Changelog:
[Android][Fixed] - Remove previous fix for "Fix text in ReactTextView sometimes being vertically displayed"
Reviewed By: mdvacca
Differential Revision: D27501405
fbshipit-source-id: 81417069d355936721868ce659b3fe8ce916302e
Summary:
Sometimes ```hasActiveCatalystInstance()``` is used to check if it's safe to access the CatalystInstance, which will still crash in Venice.
Previously we mitigate this by changing ```reactContext.hasActiveCatalystInstance()``` to ```reactContext.hasActiveCatalystInstance() || reactContext.isBridgeless()```.
To solve this for all and good the plan is:
1, Rename ```hasActiveCatalystInstance()``` to ```hasActiveReactInstance()``` so it won't sounds like CatalystInstance-only.
2, Implement hasActiveReactInstance() for Venice. D27343867
3, Remove previous mitigation. D27343952
This diff is the first step, by xbgs there are **58** non-generated callsites of ```hasActiveCatalystInstance()``` in code base which are all renamed in this diff.
Changelog:
[Android][Changed] - Rename "hasActiveCatalystInstance" to "hasActiveReactInstance"
Reviewed By: mdvacca
Differential Revision: D27335055
fbshipit-source-id: 5b8ff5e09b79a492e910bb8f197e70fa1360bcef
Summary:
This diff contains the code from the 35 diff stack - D27210587
This diff implement and integrates Mapbuffer into Fabric text measure system
changelog: [internal] internal
Reviewed By: JoshuaGross
Differential Revision: D27241836
fbshipit-source-id: f40a780df0723f27da440f709a8676cfcca63953
Summary:
Just adding more logs I found useful while playing around with the last diff (to verify that these methods were not actually implicated at all).
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26972725
fbshipit-source-id: 0e048e1edbfbe5ed32c5277f17a7197e0afdc04f
Summary:
When the height of a HorizontalScrollView changes and there is a `layout` event, it can cause the underlying platform View code to scroll slightly to the right.
I'm... not really sure why, even after looking at the View code for a while. But it is clearly detectable and mirrors issues with RTL that were fixed recently.
This might warrant more investigation, but I believe the fix is relatively safe - we detect if there's an autoscroll only if the height changes and only if the scroll happens in "layout". That scopes the hack pretty well to just this bug.
There aren't really times when we actually want layout to scroll to the right, so... I think this is reasonable.
Changelog: [Changed][Android] Fixed issue that causes HorizontalScrollView to shift to the right when a TextInput is selected and keyboard pops up
Reviewed By: mdvacca
Differential Revision: D26972710
fbshipit-source-id: 441b1a3f07b9b68195a9e5e9a0c8d75c9d24a109
Summary:
Does not impact prod or even debug builds unless you switch on the debug flag. The issue is that we were trying to coerce booleans to integers.
Changelog: [internal]
Reviewed By: mdvacca
Differential Revision: D26970274
fbshipit-source-id: 3327029ae3afc307dd19b089c23c190cb9e3150c
Summary:
We can override the `scrollTo` method and it's likely/possible that Android internals are calling scrollTo directly. So, we can capture
more cases where the scroll position is changing and needs to be updated in Fabric State.
Unfortunately we still cannot override smoothScrollTo because it is marked as final. For now we just keep the custom `reactSmoothScrollTo` method
and hope that we can catch more cases with `scrollTo`.
Changelog: [Internal]
Reviewed By: sammy-SC, mdvacca
Differential Revision: D26887028
fbshipit-source-id: e2678f1a20640d598abbec9671d6102635f65bb2
Summary:
When debugging this class a lot, I found it helpful to have these logs and it would have been nice if they were here already - I had to rewrite these several times.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26836318
fbshipit-source-id: 08eb9ae19923fc593d1aba031586a02a193d6b2d
Summary:
changelog: [internal]
There were three separate problems preventing measure infra to work correctly with views inside horizontal scroll view in RTL environment.
1. Initial offset is wasn't communicated to Fabric. This is resolved separately as it doesn't affect only RTL: D26778991 (https://github.com/facebook/react-native/commit/630ac87591eb9d535bc32c6c42c624bfc3f1953f).
3. On Android when layout direction is RTL, offset of scrollview is calculated from right.
Reviewed By: mdvacca
Differential Revision: D26779860
fbshipit-source-id: 61572c78091a1f5417102eb38d88ba7d172e6102
Summary:
Whenever layout updates in a horizontal scrollview, in RTL mode we adjust the position - the impact *should* be that initially, we jump from position 0 to the right side of the scroll view,
such that scrolling starts from the right.
However, we were doing this entirely too aggressively before. We should only make this adjustment *if the layout changes the width*.
Changelog: [Android][Changed] Fixed jumpy RTL horizontal ScrollViews. If you have Android-specific JS hacks for handling RTL in ScrollViews, you probably can/probably want to remove them, because they should be reliable now and require fewer hacks.
Reviewed By: mdvacca
Differential Revision: D26771366
fbshipit-source-id: de11bd1cae1414018d88ce44b3583a8b15f3b330
Summary:
There are races between BackgroundExecutor and Fabric/View teardown, where, because of the way things are set up currently, a View will strongly retain a pointer into some native State object which can keep a host of other objects alive in C++, even after stopSurface, and even after RN itself starts tearing down.
To alleviate this, we more aggressively clear State from Java, without waiting for Java GC: 1) on stopSurface, 2) whenever a State object is stale from Java's perspective.
This should allow us to keep all common updateState semantics, while only introducing a new edge-case that stateWrapper can be destroyed during mounting if stopSurface happens at the same time. In those cases, checking for NPEs should be sufficient.
The possible race condition only really happens with updateState, so it's easier to check for. There should practically be no cases where there's a race between `stopSurface` and `getState`, because `getState` is only really called around state updates and view preallocation, and never after; we still check for NPEs in those cases, but it shouldn't be an issue.
Changelog: [Internal]
Reviewed By: thurn, sammy-SC, mdvacca
Differential Revision: D26858584
fbshipit-source-id: 2ef7467220865380037d69d8de322fe8797f6a12
Summary:
Found this crash when rendering ReactEditText under Venice, from comment it's supposed to be called only in Paper, so I adde a Venice check to avoid calling this method.
{F446844248}
Changelog:
[Android][Changed] - Add a new check to avoid calling this method
Reviewed By: mdvacca
Differential Revision: D26781457
fbshipit-source-id: f4c2e890156a37e35aa153c736b50924254e67bc
Summary:
setRemoveClippedSubviews in ReactHorizontalScrollContainerView.java in RTL mode is overzealous and unexpectedly clips out views in a way that is not desirable.
It seems like what is actually happening is that the computed rect for the view is "0,0" and so contents are assumed to always be outside of this rect.
For now I've disabled this feature. We can investigate as a followup.
Changelog: [Android][Changed] Clipping subviews has been temporarily disabled in HorizontalScrollView in RTL mode. Minor/negligible perf impact.
Reviewed By: sammy-SC
Differential Revision: D26808937
fbshipit-source-id: 85af9c3fb542db9ca3aae03413a475695cd53391
Summary:
Changelog: [internal]
Calling `scrollTo` does not report offset change to Fabric core and measure infra can't compute correct values. This results in unresponsive buttons if horizontal scroll view on Android has initial offset set to anything besides default value 0.
Reviewed By: JoshuaGross
Differential Revision: D26778991
fbshipit-source-id: 5cad5cb9926c7923f6efcd56cb4e15c3b958c245
Summary:
## Issue:
Sometimes ReactTextView are vertically displayed as one column with bridgeless.
## Root cause:
After debugging, I found out this is caused by a workaround in 2016 to fix a crash caused by mLayout occasionally being non-null and triggers relayout during setText.
https://github.com/facebook/react-native/pull/7011
## Fix
Revert previous hack, if the crash happens again I'll try to fix it.
Changelog:
[Android][Fixed] - Fix text in ReactTextView sometimes being vertically displayed
Reviewed By: mdvacca
Differential Revision: D26581756
fbshipit-source-id: a373d84dc1ab3d787bda7ec82f2d0865a354cf60
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056823
fbshipit-source-id: 1d25afb2d4cfd7e539214d4592e361260f98fc56
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056815
fbshipit-source-id: d0e03a69f84de47385e064c74f0a79c52c61022d
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056813
fbshipit-source-id: e3b0132fac6335e83ff5b1992424edcb98703803
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056808
fbshipit-source-id: aa7ff9520a6a2470c642f06797757e1c0362abe9
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056802
fbshipit-source-id: 72eb498673aecce48145c785a4c2a48e0432059d
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056723
fbshipit-source-id: c20183a4a1189f13b15a138968937080888a200b
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056720
fbshipit-source-id: f36a6caf4e748c915b66f66fd9b4cad6826ecacf
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056714
fbshipit-source-id: 215d8e44d7909f30f4a45f57e5d22a32a635d0ba
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056700
fbshipit-source-id: b3452125678ec8770dec9587106b8bf0f2490027
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056696
fbshipit-source-id: 50c0f01164e078b0ad32f66dda80c965f731f1fb
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056690
fbshipit-source-id: 208982ca5b53985bd8079c4b206a7eb4bb883494
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056685
fbshipit-source-id: a9abb451e62c9e378b0a5667fa4c5f82952bd02b
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056680
fbshipit-source-id: 2a57f15c01a585af9ddbe08cf6dd60922d4f64b3
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056676
fbshipit-source-id: 6baa5d9ba0ef15218ce02cbb51862f18d98b465c
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26056672
fbshipit-source-id: 2f3e4b9bbed339f15b22fe87e27d81af083fd481
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: PeteTheHeat, mdvacca
Differential Revision: D26055276
fbshipit-source-id: f38f9e94ab313fb842d1e3037ab667cd460f1043
Summary:
Support RCTModernEventEmitter+RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26054855
fbshipit-source-id: 9592d854962d53f64d836b8361256cac5c4325df
Summary:
Support RCTModernEventEmitter +RCTEventEmitter in an Event class(es). This improves perf in Fabric. Migrate any constructor callsites to the new constructor and deprecate the previous one.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26043861
fbshipit-source-id: 37757508c835cbd1181f0e0e774abc62fdbfee2b
Summary:
The `Event` interface has been improved such that:
1. `getEventData` is now a default method on Event that returns null
2. `dispatch` has a default implementation that relies on getEventName() and getEventData()
3. `dispatchModern` can detect if surfaceId and event data are present; if not, it falls back to dispatch
This will dramatically ease future migrations: at some point in the (distant) future, we can simply delete RCTEventEmitter and
all use-cases will be supported by the current `Event` class without needing to introduce a 3rd transitional interface; and 99%
of all Event classes can be simplified, delet their `dispatch` implementation and need no further work.
At their core, all Events are simply: (1) a name, (2) data, (3) a target (surfaceId and tag). The interface now reflects that but still
allows for flexibility of the data and names being generated on-demand if necessary; but for the vast majority of Event classes, code
will be dramatically simplified.
I also migrate a single Event class, ContentSizeChangeEvent, to use this new method of dispatch.
Changelog: [Android][Changed] Added convenience methods to simplify native Event classes and ease migrations
Reviewed By: mdvacca
Differential Revision: D26043325
fbshipit-source-id: bc308105f7f6e654d45fd156dbf4a2bcbc45819c
Summary:
See title. dispatchV2 was just introduced yesterday, so nothing relies on it yet / it's safe to rename (there are no released versions of RN with `dispatchV2`).
And... dispatchModern is a better name.
Changelog: [Internal]
Reviewed By: mdvacca
Differential Revision: D26039008
fbshipit-source-id: 009ae721b8caef23a389c33b51f6f8952a6a73da
