Commit daf6e29
authored
fix: flashlist disappearing items (#3600)
## 🎯 Goal
This PR addresses an issue with `MessageFlashList` where items would
disappear (but still keep their layout within the internal
`ScrollView`). The reason behind it is quite technical and it was pretty
difficult to find, even though it's just a oneliner.
The PR that introduced this was [this
one](b935bbe)
specifically.
Typically, when `overflow` is present - React Native takes a [completely
different](https://github.com/facebook/react-native/blob/8636cadb8e7d0f62c213e98d264f11dfc5ea913e/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewGroup.kt#L885)
draw path. If we look at the [concrete
implementation](https://github.com/facebook/react-native/blob/8636cadb8e7d0f62c213e98d264f11dfc5ea913e/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt#L537),
we can see the radius distinction:
- no rounded borders: `clipRect`
- rounded borders: `clipPath`
So `borderRadius` + `overflow: 'hidden'` forces the more complex rounded
path clipping branch (in other words meaning, instead of drawing a
rectangle with discrete dimensions; draw an oval shape instead).
Now, `borderRadius` also [participates
in](https://github.com/facebook/react-native/blob/8636cadb8e7d0f62c213e98d264f11dfc5ea913e/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BackgroundStyleApplicator.kt#L747)
`inset` math of the `View`. This essentially means that it creates
proper bounds for the `Canvas` underneath, should something change.
When a `border` is present (and it specifically needs to be a `border`
with a known `width`), we essentially do bounded calculations
differently.
Namely, here's a breakdown of what's (likely) going on:
1. `overflow !== visible` makes `ReactViewGroup.dispatchDraw` call
`clipToPaddingBox(...)` before drawing children
2. Because the view has rounded borders, `clipToPaddingBox(...)` uses
`canvas.clipPath(...)`, not `canvas.clipRect(...)`
3. That rounded clipping path is computed from the view bounds, border
radius and border insets
4. Removing `borderWidth` removes the border insets/border drawable
layer that had previously participated in this rounded clipping geometry
and invalidation path
5. Enter `MessageFlashList`, cells are recycled and absolutely
repositioned while their content/layout changes and that makes the
borderless rounded `clipPath` state fragile on Android: the row could
remain mounted, measured and pressable, while the draw pass clipped some
or all descendants out (this is why the bug represented either thorough
an empty message bubble or a completely gone one, depending on which
`item` type was being recycled, since single attachments are rendered
fully as a bubble)
7. Removing `overflow: 'hidden'` avoids this native clipping branch
entirely, so recycled `FlashList` cells are no longer dependent on a
stale rounded clip path
In other words, as the `CellRenderer` gets recycled from `FlashList`,
its inner children do not know that they need to recalculate and so the
new `props` are injected into a view which cannot display them and we
either get the full bubble missing or the content not being there in
text messages specifically.
Very interesting nonetheless. Also probably something important to keep
in mind for the future. If we are ever to use `overflow: 'hidden'`
specifically in `FlashList` item descendants we need to make sure that
the layout of the `View` is fully stable and measureable, so that
recycling does not accidentally skip necessary recalculations.
Also, ironically, this should also be a slight performance improvement
as well (at least on Android) as now all `MessageContent` components are
going to go down the easier drawing route.
## 🛠 Implementation details
<!-- Provide a description of the implementation -->
## 🎨 UI Changes
<!-- Add relevant screenshots -->
<details>
<summary>iOS</summary>
<table>
<thead>
<tr>
<td>Before</td>
<td>After</td>
</tr>
</thead>
<tbody>
<tr>
<td>
<!--<img src="" /> -->
</td>
<td>
<!--<img src="" /> -->
</td>
</tr>
</tbody>
</table>
</details>
<details>
<summary>Android</summary>
<table>
<thead>
<tr>
<td>Before</td>
<td>After</td>
</tr>
</thead>
<tbody>
<tr>
<td>
<!--<img src="" /> -->
</td>
<td>
<!--<img src="" /> -->
</td>
</tr>
</tbody>
</table>
</details>
## 🧪 Testing
<!-- Explain how this change can be tested (or why it can't be tested)
-->
## ☑️ Checklist
- [ ] I have signed the [Stream
CLA](https://docs.google.com/forms/d/e/1FAIpQLScFKsKkAJI7mhCr7K9rEIOpqIDThrWxuvxnwUq2XkHyG154vQ/viewform)
(required)
- [ ] PR targets the `develop` branch
- [ ] Documentation is updated
- [ ] New code is tested in main example apps, including all possible
scenarios
- [ ] SampleApp iOS and Android
- [ ] Expo iOS and Android1 parent 90b3204 commit daf6e29
2 files changed
Lines changed: 0 additions & 5 deletions
File tree
- package/src/components
- Message/MessageItemView
- Thread/__tests__/__snapshots__
Lines changed: 0 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
656 | 656 | | |
657 | 657 | | |
658 | 658 | | |
659 | | - | |
660 | 659 | | |
661 | 660 | | |
662 | 661 | | |
| |||
Lines changed: 0 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
506 | 506 | | |
507 | 507 | | |
508 | 508 | | |
509 | | - | |
510 | 509 | | |
511 | 510 | | |
512 | 511 | | |
| |||
849 | 848 | | |
850 | 849 | | |
851 | 850 | | |
852 | | - | |
853 | 851 | | |
854 | 852 | | |
855 | 853 | | |
| |||
1225 | 1223 | | |
1226 | 1224 | | |
1227 | 1225 | | |
1228 | | - | |
1229 | 1226 | | |
1230 | 1227 | | |
1231 | 1228 | | |
| |||
1559 | 1556 | | |
1560 | 1557 | | |
1561 | 1558 | | |
1562 | | - | |
1563 | 1559 | | |
1564 | 1560 | | |
1565 | 1561 | | |
| |||
0 commit comments