feat: Add iOS Paper implementation of inset logical properties

[gabrieldonadel]

Summary

This PR adds Paper support to inset logical properties on iOS as requested on #34425. This implementation includes the addition of the following style properties

• inset, equivalent to top, bottom, right and left.
• insetBlock, equivalent to top and bottom.
• insetBlockEnd, equivalent to bottom.
• insetBlockStart, equivalent to top.
• insetInline, equivalent to right and left.
• insetInlineEnd, equivalent to right or left.
• insetInlineStart, equivalent to right or left.

Android changes are in a separate PR to facilitate code review #36242

Changelog

[IOS] [ADDED] - Add Paper implementation of inset logical properties

Test Plan

1. Open the RNTester app and navigate to the View page
2. Test the new style properties through the Insets section

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,458,541	-93
android	hermes	armeabi-v7a	7,781,811	-84
android	hermes	x86	8,934,442	-93
android	hermes	x86_64	8,791,524	-100
android	jsc	arm64-v8a	9,092,729	-43
android	jsc	armeabi-v7a	8,290,826	-43
android	jsc	x86	9,143,541	-49
android	jsc	x86_64	9,402,401	-54

Base commit: 9718c17
Branch: main

[NickGerleman]

One thing that still needs to be figured out is how to respect the same precedence as here when using Paper (cc @NickGerleman)

Yeah, I think that is the main extra thing we need to merge this. Just ensuring that insetBlockStart and insetBlockEnd do not overwrite position: top and position: bottom.

We are allowed to have a sequence that looks like:

1. Set positionTop: 5
2. Set insetBlockStart: 10
3. Clear positionTop

There, we should evaluate a top position of "5" at steps 1 and 2, then "10" at step 3. So if we clear out one prop, the state for the other one still needs to be around.

The way I solved this in the Fabric case was that we always had the full set of props available to read from (this is already Fabric's prop parsing model), then we generated the flattened YGStyle after parsing everything. The model for how prop parsing works on paper is different, but I imagine we might need the state for both potential props around, to do this sort of thing.

[gabrieldonadel]

Thanks for the review @NickGerleman, I managed to fix the precedence problem by adding a _positionMetaProps param to the ShadowView, using a similar approach to what we already have for padding, margin, and borders. Please let me know if this approach looks good to you, if so I'll try to do something similar for android.

[NickGerleman]

Ah, that looks correct. Were you able to validate it against the scenario?

[facebook-github-bot]

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[gabrieldonadel]

Ah, that looks correct. Were you able to validate it against the scenario?

Yeah, from my testing everything is working as expected

[facebook-github-bot]

@NickGerleman merged this pull request in 33d4e2d.

[NickGerleman]

Hey @gabrieldonadel, we had to revert this change since it caused a few screenshot test failures that seem legitimate. I think this example is setting position via start. FYI @necolas as well.

[gabrieldonadel]

Hey @NickGerleman, do you mind sharing some more details about this screenshot test? Which combination of props were you using? Setting positions using start and end seems to be working fine on my end

[NickGerleman]

Hey @NickGerleman, do you mind sharing some more details about this screenshot test? Which combination of props were you using? Setting positions using start and end seems to be working fine on my end

It looks like start: 'n%', repeatedly updated, with width, height, marginStart, and position: 'absolute' all set, but static.

[gabrieldonadel]

It looks like start: 'n%', repeatedly updated, with width, height, marginStart, and position: 'absolute' all set, but static.

Hmm, not sure why that would happen, maybe we're not computing the position on the first pass somehow?

[lunaleaps]

Let me see if I can get a small repro of the issue out so we can debug and also see if the Android version suffers the same issue

[lunaleaps]

@gabrieldonadel I can reproduce on Paper with this:

<View
    style={{
       backgroundColor: 'blue',
       height: 20,
       position: 'absolute',
       width: 20,
       start: '10%'
    }}
/>

Screenshot for Fabric | Screenshot for Paper

[gabrieldonadel]

Sorry for the delay on this, I'll take a look this week*!

[lunaleaps]

Hey @gabrieldonadel do you have a status update on this?

[gabrieldonadel]

Hey @gabrieldonadel do you have a status update on this?

Hi @lunaleaps unfortunately I was still not able to reproduce this issue, are you testing it through RNTester?

[lunaleaps]

Yes, essentially the same as RNTester, it's an empty surface. You're not able to reproduce on the legacy renderer?

[gabrieldonadel]

Yes, essentially the same as RNTester, it's an empty surface. You're not able to reproduce on the legacy renderer?

Yes @lunaleaps, I'm trying to reproduce this inside the View Example of RNTester running Paper

Screen.Recording.2023-03-21.at.07.35.14.mov

[gabrieldonadel]

Any thoughts on how can I reproduce this @lunaleaps using RNTester?

[lunaleaps]

Hey @gabrieldonadel sorry for the delay, I just tested it on the 0.72 release, picking this commit and I was able to reproduce in RNTester

Here's the repro code lunaleaps@29c081b