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.

Sign up for GitHub

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

Jump to bottom

Fix iOS Paper Scroll Event RTL check #40751

Closed
wants to merge 3 commits into from
Closed

Fix iOS Paper Scroll Event RTL check #40751

wants to merge 3 commits into from

Commits on Oct 10, 2023

  1. Deterministic onLayout event ordering for iOS Paper 

    Summary:
The ordering of `onLayout` events is non-deterministic on iOS, due to nodes being added to an `NSHashTable` before iteration, instead of an ordered collection.

We don't do any lookups on the collection, so I think this was chosen over `NSMutableArray` for the sake of `[NSHashTable weakObjectsHashTable]`, to avoid retain/release. Using a collection which does retain/release seems to cause a crash due to double release or similar, so those semantics seem intentional (though I'm not super familiar with the model here).

We can replicate the memory semantics with ordering by using `NSPointerArray`. This change does that, so we get consistetly top-down layout events (matching Fabric, and Android Paper after a recent change). This lets us use multiple layout events to calculate right/bottom edge insets deterministically.

Changelog:
[iOS][Changed] -  Deterministic onLayout event ordering for iOS Paper

Differential Revision: D50093411

fbshipit-source-id: d9229f98d8032673707fb8b9e0fd169f660edb08
    @NickGerleman @facebook-github-bot
    NickGerleman authored and facebook-github-bot committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    d5f12be View commit details
    Browse the repository at this point in the history

  2. Remove code to support bottom-up layout events in horizontal RTL 

    Summary:
We can dramatically simplify this code and remove quirks, now that we can assume layout events are always fired top down.

Changelog: [Internal]

Differential Revision: D49628669

fbshipit-source-id: daa14665a13c8587f7d03dfd0d7242429c3f9329
    @NickGerleman @facebook-github-bot
    NickGerleman authored and facebook-github-bot committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    abe81cd View commit details
    Browse the repository at this point in the history

  3. Fix iOS Paper Scroll Event RTL check (facebook#40751) 

    Summary:
Pull Request resolved: facebook#40751

In D48379915 I fixed inverted `contentOffset` in `onScroll` events on iOS. I thought I tested on Paper, but I think this was during a period where the Paper route in Catalyst was actually launching Fabric (oops).

In Paper, at least under `forceRTL` and English, `[UIApplication sharedApplication].userInterfaceLayoutDirection` is not set to RTL. We instead have a per-view `reactLayoutDirection` we should be reading.

This sort of thing isn't currently set on Fabric, which checks application-level RTL. This seems... not right with being able to set `direction` in a subtree context, but Android does the same thing, and that would take some greater changes.

Changelog:
[iOS][Fixed] - Fix iOS Paper Scroll Event RTL check

Reviewed By: luluwu2032

Differential Revision: D50098310

fbshipit-source-id: 43c3351cf01f624643c5f7be5c544e3a99dcb927
    @NickGerleman @facebook-github-bot
    NickGerleman authored and facebook-github-bot committed Oct 10, 2023
    Configuration menu
    Copy the full SHA
    067d2bf View commit details
    Browse the repository at this point in the history