[Android][HorizontalScrollView] last item removal scroll position restore #27514

jayshah123 · 2019-12-15T07:23:08Z

Summary

Fixes #27504
Which is: Whenever a last list item in a horizontal scroll view in android is removed, the position is not reset and shows an empty/blank space in the end (gif attached in the issue).

Changelog

Similar to ReactScrollView(https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java#L774):
We add a listener which triggers whenever a list item is removed/added, and it checks if we have a scroll position that is more than the max possible scrollposition, it will reset it to max possible scroll position.

[Android] [Fixed] - last item removal scroll position restore in ReactHorizontalScrollView

Test Plan

Code should not be a problem since there is already a similar solution in ReactScrollView.
Attaching gif for the fixed version.

facebook-github-bot · 2019-12-15T07:23:26Z

Hi jayshah123! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

safaiyeh · 2019-12-15T19:41:25Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

+   * position is larger than the ScrollView's max scroll position after the content shrinks.
+   */
+  @Override
+  public void onLayoutChange(


Would setting fillViewport achieve the same behavior? If Android already handles it, the manually calculations may not needed.

https://developer.android.com/reference/android/widget/HorizontalScrollView.html#setFillViewport(boolean)

fillViewPort will change how the child views are being measured, typically it might result in different behaviour in a case where content size is less than viewport size.
I will try to come up with a case where fillViewport might break existing behaviours.

Even with fillViewPort set to true, the issue does not go away.

hence the original solution

facebook-github-bot · 2019-12-16T07:21:30Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

Peeeep · 2019-12-31T22:45:12Z

Hey, great you are tackling this issue which has been bugging me for a couple of days now! Unfortunately, I wasn't able to get this to work @0.61.5... Is there any way I can help debugging this?

velmar85 · 2020-01-01T10:28:58Z

that can be possible , all deppends how tou want fix it

…

Sent from my iPhone On Dec 31, 2019, at 5:48 PM, Alexander Kurnikowski <[email protected]<mailto:[email protected]>> wrote: Hey, great you are tackling this issue which has been bugging me for a couple of days now! Unfortunately, I wasn't able to get this to work @0.61.5... Is there any way I can help debugging this? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#27514?email_source=notifications&email_token=AMM5L3F7H6KEKBKKYXKFMK3Q3PD3VA5CNFSM4J26Z7RKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH4YOUY#issuecomment-570001235>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AMM5L3H7CXZ2L2MTK4D652DQ3PD3VANCNFSM4J26Z7RA>.

JoshuaGross · 2020-01-17T19:09:35Z

@jayshah123 it sounds like you're still working on finding a case where setting fillViewport alone would not work, right? Is this ready for review yet?

jayshah123 · 2020-01-31T10:00:42Z

I will check this weekend. and update.
Also, My current solution is just an imitation of what is already present in the regular ScrollView.

jayshah123 · 2020-02-16T06:51:41Z

Even with fillViewPort set to true, the issue does not go away.

hence the original solution

Summary: Running `./gradlew assembleRelease` fails as the path to the CLI contains a new line at the end. We don't run this command in `debug` mode, hence it passed the testing. My bad. Fixed, checked in both `debug` with `bundleInDebug: true` and `release`. Fixes facebook#28700 ## Changelog [INTERNAL] [ANDROID] - Fix `React.gradle` to build Android apps in production Pull Request resolved: facebook#28776 Test Plan: Running `./gradlew assembleRelease` works Reviewed By: hramos Differential Revision: D21287789 Pulled By: TheSavior fbshipit-source-id: dc3ec8eef7a919b072b562d2bd455e2f704bc083

Summary: This is the iOS analogue to D21290582. Changelog: [iOS][Fixed] - TurboModule cleanup Reviewed By: fkgozali Differential Revision: D21290852 fbshipit-source-id: c0975a1f320ad4ad4ef16eec82eca38389c71a0a

Summary: When I initially made TurboModuleManager.getModule thread-safe, I unintentionally broken TurboModule cleanup. This diff fixes that mistake. ## Mistake ``` Override public void onCatalystInstanceDestroy() { synchronized (mTurboModuleCleanupLock) { mTurboModuleCleanupStarted = true; } final Set<String> turboModuleNames = new HashSet<>(mTurboModuleHolders.keySet()); for (final String moduleName : turboModuleNames) { // Retrieve the TurboModule, possibly waiting for it to finish instantiating. final TurboModule turboModule = getModule(moduleName); // ERROR! if (turboModule != null) { // TODO(T48014458): Rename this to invalidate() ((NativeModule) turboModule).onCatalystInstanceDestroy(); } } ``` Before calling `getModule(moduleName)`, I set `mTurboModuleCleanupStarted = true`. This assignment makes all calls to `getModule` return `null`, which means that none of the TurboModules were having their `onCatalystInstanceDestroy()` method invoked. Changelog: [Android][Fixed] - TurboModule cleanup Reviewed By: fkgozali Differential Revision: D21290582 fbshipit-source-id: 3645b9a4f8c6f41498ebd9d51f9b5775edf2dbd7

Summary: Pull Request resolved: facebook#28779 Creating a JNI wrapper class for RuntimeExecutor so we can pass it to Fabric and TurboModules in Java. Changelog: [Internal] Reviewed By: RSNara Differential Revision: D21049385 fbshipit-source-id: f833004225d9837acf6ffafd3988f89748cf12aa

Summary: I refined the error message of scrollToIndex. When I used scrollToIndex with `index:0` and data that length is 0, I met the odd error message `Invariant Violation scrollToIndex out of range: requested index 0 but maximum is -1`. Next, I thought that scrollToIndex with `index:-1` meant scrollToTop without checking data length. I met `Invariant Violation: scrollToIndex out of range: requested index -1 but maximum is -1` Finally, I wondered what will happen to use scrollToIndex with `index:-1` and data that length is `5`. The result is `Invariant Violation: scrollToIndex out of range: requested index -1 but maximum is 5` The above error messages will confuse us. I clarified the boudaries and separated the error messages ## Changelog [General] [Fixed] - Clarified the boundaries in error message of scrollToIndex Pull Request resolved: facebook#28464 Test Plan: I added 3 test cases to cover the new error messages for VirtualizedList. Run `yarn test` and confirm it passes. Reviewed By: cpojer Differential Revision: D21140133 Pulled By: TheSavior fbshipit-source-id: 9a7a704f7ec599d833d2ed3ca2be059d950539b5

…esult Summary: This diff rejects the promise when if an error occurs while processing prefetching result, it applies the same concept as other methods in this class (e.g. see getSizeWithHeaders) changelog: [internal][Android] Internal change in RN Image prefetching Reviewed By: JoshuaGross Differential Revision: D21295612 fbshipit-source-id: c3675e5f2d9c8e38094a538b388ff63a6ea18360

Summary: This diff adds a hostname to the loading banner on iOS so it's clear which server you're loading from. Changelog: [Added] [iOS] Added hostname to loading banner. Reviewed By: PeteTheHeat Differential Revision: D21280252 fbshipit-source-id: d7733c056f5fb63e32b247a4fa1476ab42c7da17

Summary: This diff updates the loading banner text and color on iOS for better UX. Flow before: - Loading from localhost:8081... - Loading 20% (1000/5000)... - Downloading JavaScript Bundle 20% (10/50) - Downloading JavaScript Bundle... After: - Loading from Metro... - Bundling 20%... - Downloading 20%... - Downloading... Changelog: [Added] [iOS] Updated loading banner messages and color Reviewed By: PeteTheHeat Differential Revision: D21279939 fbshipit-source-id: fd7d90f85e25ce175a87087dfccf2180d49e3e98

Summary: This diff fixes an issue where the loading banner message could update while the banner hide animation is going, catching your eye for no reason. Changelog: [Fixed] [iOS] Do not update loading banner message while hiding Reviewed By: PeteTheHeat Differential Revision: D21280786 fbshipit-source-id: a10b33cd72f263d08eea6d8e94963514affbe24d

Summary: This diff fixes an issue where the initial "Loading from Metro..." message is flashed for a few milliseconds before the download progress kicks in. It's just jarring enough to be noticed and is ~600ms too fast to be read. This diff adds a delay so that if the loading progress starts within 200ms we go straight to the progress. Changelog: [Fixed] [iOS] Delay loading banner message to prevent flashing messages Reviewed By: PeteTheHeat Differential Revision: D21281870 fbshipit-source-id: d28c1eae01c2ac9d79f356f1870f17dbb22a9d84

Summary: This diff reduces the minimum loading banner time and animation time to make the locating banner faster. Changelog: [General] [iOS] Speed up loading banner animations Reviewed By: PeteTheHeat Differential Revision: D21290517 fbshipit-source-id: 111dff41df53b0246548e1da1db80c2188498a9c

Reviewed By: zertosh Differential Revision: D21683227 fbshipit-source-id: a29bc66af62fe99d803ac386261269b5cd16c18f

…mponentView Summary: Changelog: [Internal] # Problem We were recording mount child component calls with its arguments and the replaying them inside `finalizeUpdates`. However we store the events in NSDictionary where `key` was index at which the child should be added. Then in `finalizeUpdates` we iterated over this NSDictionary and added those views into our paper view. `NSDictionary` is unordered, it isn't guaranteed what was first inserted into it, will be first iterated over. # Solution Use `NSMutableArray` instead which guarantees order. Reviewed By: shergin Differential Revision: D21685993 fbshipit-source-id: 3b933f05125130eef175d7a8a56f29012ee76bb3

Summary: Changelog: [Internal] Separates EventQueues from an array into 4 ivars. EventQueues are of different type, in the future we will want to call different methods on different kind of EventQueue. Reviewed By: shergin Differential Revision: D21648905 fbshipit-source-id: 90ae65edb8a9276eecfea9770f554d8c56804797

Summary: Changelog: [Internal] `BatchedEventQueue::enqueueUniqueEvent` goes over event queue and deletes previous event of the same type and same target. This is useful for ScrollView for example where only the latest event is relevant. This only affects ScrollView scroll event, other events take the original code path. Reviewed By: mdvacca Differential Revision: D21648906 fbshipit-source-id: a80ad652058fd50ebb55e24a87229cdc1764b591

Summary: When `minPressDuration` was introduced to `Pressability`, all of the legacy Touchable components inherited the new default. This restore the former behavior for these legacy components so that only `Pressable` gets the new `minPressDuration` default value. Changelog: [General][Fixed] - Revert `minPressDuration` effect on legacy Touchable components Reviewed By: fkgozali Differential Revision: D21682764 fbshipit-source-id: b71a61843fae7f0f726155876a064fabd3ba1c64

Summary: Internal code attribution labeling update. Changelog: [Internal] Reviewed By: zlern2k Differential Revision: D21696075 fbshipit-source-id: ef689a6367e1dddfffbbefb52a6aead2c91bfefe

Summary: While we build react native 0.62.2 via our Bazel build system, encountered those following errors due to lack of appropriate imports: ``` external/React-Core/React/Base/RCTUtilsUIOverride.h:8:33: error: cannot find interface declaration for 'NSObject', superclass of 'RCTUtilsUIOverride' interface RCTUtilsUIOverride : NSObject ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ external/React-Core/React/Base/RCTUtilsUIOverride.h:12:37: error: expected a type + (void)setPresentedViewController:(UIViewController *)presentedViewController; ^ ``` Add the appropriate imports `<Foundation/Foundation.h>` and `<UIKit/UIKit.h>` fix those errors. Honestly I dont know how it's supposed to work without those imports. Also all the siblings files have the correct imports. e.g. [RCTUtils.h](https://github.com/discord/react-native/blob/15a5f3624c40624d8dd0307bbcc1f2b2aba15a1b/React/Base/RCTUtils.h) ## Changelog [iOS] [Fixed] - Fix imports in `RCTUtilsUIOverride.h` Pull Request resolved: facebook#28946 Test Plan: RN tester iOS app runs fine. Differential Revision: D21700030 Pulled By: shergin fbshipit-source-id: 9ef806b8f656bdad289fbdd3d84ecefb0dea6afb

Summary: This diff refactors the types of ART Text classes, this is necessary on the next diffs of the stack closeoncommit changelog: [internal] Reviewed By: JoshuaGross Differential Revision: D21681876 fbshipit-source-id: ea438e89df6d860b3ff8bbdae657ca123b417a1b

Summary: This diff implements the serialization of Text components to send data from C++ to java changelog: [Internal] internal changes to support ART in fabric Reviewed By: JoshuaGross Differential Revision: D21681875 fbshipit-source-id: eba31f35c95e0a2d3226ec70421832719083d7fa

Summary: This diff adds support for Text in ART Fabric Android changelog: [Internal] internal changes to fully support ART in Fabric Reviewed By: JoshuaGross Differential Revision: D21681877 fbshipit-source-id: c92e642cff56b71f8ee8f4eb9af6eea6c490f6c7

Summary: Element, Shape, Group, Text are too generic, we are renaming these classes to ARTElement, ARTBGroup, ARTShape... changelog: [Internal] internal changes to support ART in android Reviewed By: JoshuaGross Differential Revision: D21681878 fbshipit-source-id: f6b35443486a3db210f61dcaf91bd32df47fbc66

Summary: Here I'm implementing equality methods for ARTGroup, ARTShape and ARTText and I'm using these methods to update the state only when it is necessary. This will improve perf in rendering of ART changelog: [Internal] Reviewed By: JoshuaGross Differential Revision: D21695127 fbshipit-source-id: b438ddea4c34bd7a0bdf26a6aac4fd62a9f78b49

Summary: ez diff to delete unused class changelog: [internal] Reviewed By: fkgozali Differential Revision: D21699666 fbshipit-source-id: d0e725c7096906e2effd16f8fa2a57683192420f

Summary: Simple diff to add ART components into Catalyst app Android changelog: [Internal] Internal changes to add support of ART for Fabric Reviewed By: JoshuaGross Differential Revision: D21621479 fbshipit-source-id: d957c25f447d19d8f349c69aa20f5f19237d867a

Summary: Changelog: [Internal] ShadowNode shouldn't be copyable. Reviewed By: shergin Differential Revision: D21692756 fbshipit-source-id: e70dcf82f4d4c7609283936a42c741467f8f13ca

…text Reviewed By: mdvacca Differential Revision: D21696266 fbshipit-source-id: b5c9d167e9da77ed969f7b4bdea1af9dd2e471ae

…yshah123/react-native into horizontalscroll_lastitem_fix

jayshah123 · 2020-05-26T09:57:20Z

having some merging issues. will cleanup and raise

pull-bot · 2020-05-26T09:58:29Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against e363309

analysis-bot · 2020-05-26T10:12:43Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,780,314	36
android	hermes	armeabi-v7a	6,442,550	21
android	hermes	x86	7,165,437	47
android	hermes	x86_64	7,056,103	39
android	jsc	arm64-v8a	8,948,117	40
android	jsc	armeabi-v7a	8,602,741	29
android	jsc	x86	8,776,335	26
android	jsc	x86_64	9,352,644	40

Base commit: 60b7a30

analysis-bot · 2020-05-26T10:17:54Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	809,568	0

Base commit: 60b7a30

react-native-bot added Component: ScrollView Platform: Android Android applications. Bug labels Dec 15, 2019

jayshah123 marked this pull request as ready for review December 15, 2019 07:56

safaiyeh reviewed Dec 15, 2019

View reviewed changes

react-native-bot added the No CLA Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2019

jayshah123 force-pushed the horizontalscroll_lastitem_fix branch from 2e6a8f1 to b7d6ae1 Compare December 16, 2019 06:16

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2019

react-native-bot removed the No CLA Authors need to sign the CLA before a PR can be reviewed. label Dec 16, 2019

jayshah123 force-pushed the horizontalscroll_lastitem_fix branch from b7d6ae1 to 8b363f9 Compare December 21, 2019 12:42

jayshah123 force-pushed the horizontalscroll_lastitem_fix branch from 8b363f9 to e23ccda Compare January 9, 2020 11:18

jayshah123 mentioned this pull request Mar 16, 2020

[Android][Flatlist][Horizontal] Last item delete in horizontal flatlist does not adjust scroll position. #27504

Closed

grabbou and others added 11 commits April 28, 2020 14:45

Fix cleanup bug

00b3cbf

Summary: This is the iOS analogue to D21290582. Changelog: [iOS][Fixed] - TurboModule cleanup Reviewed By: fkgozali Differential Revision: D21290852 fbshipit-source-id: c0975a1f320ad4ad4ef16eec82eca38389c71a0a

generatedunixname89002005287564 and others added 18 commits May 21, 2020 10:13

Daily arc lint --take CLANGFORMAT

7e343c8

Reviewed By: zertosh Differential Revision: D21683227 fbshipit-source-id: a29bc66af62fe99d803ac386261269b5cd16c18f

update internal code attribution

e85118c

Summary: Internal code attribution labeling update. Changelog: [Internal] Reviewed By: zlern2k Differential Revision: D21696075 fbshipit-source-id: ef689a6367e1dddfffbbefb52a6aead2c91bfefe

Delete unused class

19e0b13

Summary: ez diff to delete unused class changelog: [internal] Reviewed By: fkgozali Differential Revision: D21699666 fbshipit-source-id: d0e725c7096906e2effd16f8fa2a57683192420f

Delete copy constructor and copy assignment operator in ShadowNode

1b1ebaf

Summary: Changelog: [Internal] ShadowNode shouldn't be copyable. Reviewed By: shergin Differential Revision: D21692756 fbshipit-source-id: e70dcf82f4d4c7609283936a42c741467f8f13ca

Annotate <Image> components in QPL logging using ImageAnalyticsTagCon…

60b7a30

…text Reviewed By: mdvacca Differential Revision: D21696266 fbshipit-source-id: b5c9d167e9da77ed969f7b4bdea1af9dd2e471ae

last item removal scroll position restore

55407f9

Merge branch 'horizontalscroll_lastitem_fix' of https://github.com/ja…

e363309

…yshah123/react-native into horizontalscroll_lastitem_fix

jayshah123 requested review from cpojer, gengjiawen, hramos, matt-oakes, mhorowitz and shergin as code owners May 26, 2020 09:54

jayshah123 closed this May 26, 2020

jayshah123 deleted the horizontalscroll_lastitem_fix branch May 26, 2020 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android][HorizontalScrollView] last item removal scroll position restore #27514

[Android][HorizontalScrollView] last item removal scroll position restore #27514

jayshah123 commented Dec 15, 2019 •

edited

Loading

facebook-github-bot commented Dec 15, 2019

safaiyeh Dec 15, 2019

jayshah123 Dec 16, 2019

jayshah123 Feb 16, 2020

facebook-github-bot commented Dec 16, 2019

Peeeep commented Dec 31, 2019

velmar85 commented Jan 1, 2020 via email

JoshuaGross commented Jan 17, 2020

jayshah123 commented Jan 31, 2020

jayshah123 commented Feb 16, 2020

jayshah123 commented May 26, 2020

pull-bot commented May 26, 2020

analysis-bot commented May 26, 2020

analysis-bot commented May 26, 2020 •

edited

Loading

[Android][HorizontalScrollView] last item removal scroll position restore #27514

[Android][HorizontalScrollView] last item removal scroll position restore #27514

Conversation

jayshah123 commented Dec 15, 2019 • edited Loading

Summary

Changelog

Test Plan

facebook-github-bot commented Dec 15, 2019

safaiyeh Dec 15, 2019

Choose a reason for hiding this comment

jayshah123 Dec 16, 2019

Choose a reason for hiding this comment

jayshah123 Feb 16, 2020

Choose a reason for hiding this comment

facebook-github-bot commented Dec 16, 2019

Peeeep commented Dec 31, 2019

velmar85 commented Jan 1, 2020 via email

JoshuaGross commented Jan 17, 2020

jayshah123 commented Jan 31, 2020

jayshah123 commented Feb 16, 2020

jayshah123 commented May 26, 2020

pull-bot commented May 26, 2020

analysis-bot commented May 26, 2020

analysis-bot commented May 26, 2020 • edited Loading

jayshah123 commented Dec 15, 2019 •

edited

Loading

analysis-bot commented May 26, 2020 •

edited

Loading