fix(Android)!: overflowing text in native header #2325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add PoC for Fabric fix (not entirely working yet) * Move header inset updating code to the header config - it didn't make much sense for this code to live in screen. * Moved all the code fix from `viewDidLayoutSubviews` to separate method * Added few helper methods on HeaderConfig
in this commit react-navigation is on v6 branch. You need to change it back to v7 and apply changes to the v7 branch. This is the reference PR to v6: react-navigation/react-navigation#12135
satya164
pushed a commit
to react-navigation/react-navigation
that referenced
this pull request
Oct 7, 2024
…id (#12125) **Motivation** Together with corresponding PR in `react-native-screens`: * software-mansion/react-native-screens#2325 this PR fixes software-mansion/react-native-screens#1946 tldr: text truncation in header title should work properly from now on. This PR changes the layout model of the native-header. Previously Yoga has been informed that the children are positioned absolutely and its only responsibility was to determine the view sizes. Right now, on Yoga layer (but not in native layer) the header is laid out with flexbox model. **Test plan** I've conducted the tests manually on `TestHeaderTitle` test screen in `FabricExample` application in `react-native-screens` repository. Below is the spreadsheet from these tests.  Corresponding v6 PR: * #12135 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…ecture-consistecy checks
kkafar
changed the title
alduzy
reviewed
Oct 8, 2024
| headerConfig: { | ||
| flexDirection: 'row', | ||
| width: '100%', | ||
| justifyContent: 'space-between', |
There was a problem hiding this comment.
Don't we need to center subviews horizontally?
Suggested change
| justifyContent: 'space-between', | |
| justifyContent: 'space-between', | |
| alignItems: 'center', |
There was a problem hiding this comment.
justifyContent centers the elements horizontally in this case.
Note that justifyContent centers the elements with respect to the "main axis". Due to flexDirection: 'row', main axis is the horizontal axis.
kkafar
deleted the
@kkafar/shopify-not-truncated-title-with-headerright-custom-shadownodes-2
branch
2 tasks
ja1ns
pushed a commit
to WiseOwlTech/react-native-screens
that referenced
this pull request
Oct 9, 2024
## Description > [!caution] This PR includes **BREAKING CHANGES** Corresponding PR in `react-navigation`: * react-navigation/react-navigation#12125 Fixes software-mansion#1946 This PR refactors the header config component to use flex-box model instead of absolute positioning in Yoga layer. This is required so that the Yoga layouts children of header config with respect to one another (not absolutely), so that the title can be properly truncated. In the end, the subviews are laid out by system anyway, thus we send additional information, such as padding / margins from native side to the shadow tree, so that we can adjust for these in Yoga layout. > [!important] This PR introduces a bug in very first few frames - before the information from HostTree is propagated to ShadowTree the header title might not be truncated properly. This is known issue and based on the same mechanism and "jumping content" due to not including header dimensions in first Yoga layout. <!-- ### Before / After `headerTitleAlign: 'left'`, long title as string ⬇️ | Before ✅ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/ec7dd405-7244-4c18-b96d-5ffd79be78b8"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/7b0086b5-3554-4b76-99d5-302d03cab96e"></img> | | Works fine, cause `title` is passed as regular string to native side, where it is simply assigned to `toolbar.title` & `AppCompatTextView` is used by Android. `headerTitleAlign: 'left'`, `headerLeft`, long title as string ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/228f64b9-4ca8-4c65-8128-a5097bbae595"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/e1426702-a020-4113-a271-588c62e385db"></img> | | Title despite being a plain string is wrapped inside `HeaderTitle` component & rendered as a `Text` inside `SubviewLeft` together with `HeaderLeft`. The text overflows, because subview is positioned as `absolute`: it does not respect native toolbar's content inset (left padding), as Yoga has no information of it & the text view does measure itself with width of whole containing box (which is the `HeaderConfig` which has width of full screen). What's also important to notice is that in this configuration `backButtonIcon` is disabled. `headerTitleAlign: 'left'`, `headerRight`, long title as string ⬇️ | Before ✅ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/2af7a845-4c7c-419a-a9bc-5e44525fb935"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/7afaa643-7a00-42a4-8cb2-3d4fa9a9781b"></img> | | Works ok, because text is assigned to `toolbar.title`, thus `AppCompatTextView` is in use & native layout takes care of truncating the text & respecting the `HeaderRight`. We good here. `headerTitleAlign: 'left'`, `headerLeft`, `headerRight`, long title as string ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/186b8534-00e0-4dc6-b4b0-45a12e9a84c5"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/3bfc1bd6-3e9f-4ede-a375-7fa329e6a4f0"></img> | | Text is put together with `HeaderLeft` into `SubviewLeft` & rendered inside `Text`. Subviews are positioned as `absolute` & Yoga measures text with full width of the screen & does not take into account `absolute` siblings. Notice that in the second row of screenshots `backButtonIcon` is also not present. `headerTitleAlign: 'left'`, long title as `Text` ⬇️ | Before ✅ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/ec7dd405-7244-4c18-b96d-5ffd79be78b8"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/7b0086b5-3554-4b76-99d5-302d03cab96e"></img> | | `headerTitleAlign: 'left'`, `headerLeft`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/34046637-886e-44e7-8588-2143cc5ec7bc"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/b57d9beb-0c5e-405d-b920-393bee801a07"></img> | | `headerTitleAlign: 'left'`, `headerRight`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/83afc32a-b8f5-4ffc-9570-8b9034e4abb9"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/04d58470-53c3-45bb-a335-f6265b34c1e6"></img> | | `headerTitleAlign: 'left'`, `headerLeft`, `headerRight`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/30dcc02e-4fa4-4a89-9add-b925e1290bb2"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/716de207-aeec-4766-8cab-09f535f2fac2"></img> | | `headerTitleAlign: 'center'`, long title as string ⬇️ | Before ✅ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/58194a20-9373-4352-a2b6-ada2982c2646"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/6c31121d-09d9-40f5-8bf8-688cd4d1a28e"></img> | | `headerTitleAlign: 'center'`, `headerLeft`, long title as string ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/c8c2507a-fb23-4265-b257-e2a469e0cbae"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/cdc6efaa-83f8-4e7d-9aba-68ad0b276bbb"></img> | | `headerTitleAlign: 'center'`, `headerRight`, long title as string ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/c83cf46a-0b5a-49cf-96b2-c602df090a83"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/e33623ae-0a32-46d6-9355-2ce353c60a66"></img> | | `headerTitleAlign: 'center'`, `headerLeft`, `headerRight`, long title as string ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/a8c626b8-7b9d-4b91-b2f9-4a94ae10fd4a"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/90dc5994-2aca-4ce2-8360-4baea67734be"></img> | | `headerTitleAlign: 'center'`, long title as `Text` ⬇️ | Before ✅ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/b119f64a-4638-4e28-abf8-33b23290d195"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/0f0a92e4-305e-429a-b852-623dc06f43f8"></img> | | `headerTitleAlign: 'center'`, `headerLeft`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/1966cca9-1cce-47f7-8a87-a0b687edfe7a"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/5ea1bb3a-1926-4abf-839c-3ddac73c74aa"></img> | | `headerTitleAlign: 'center'`, `headerRight`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/c2801a4d-7f63-4e67-acb3-0ce55e09e937"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/789de0b6-f703-45dd-aeb0-8e3082bbf267"></img> | | `headerTitleAlign: 'center'`, `headerLeft`, `headerRight`, long title as `Text` ⬇️ | Before ❌ | After ❓| |--------|--------| | <img width="480" height="200" src="https://github.com/user-attachments/assets/9159990b-845f-4d26-b103-d1a5ecdec656"></img> | | | <img width="480" height="200" src="https://github.com/user-attachments/assets/07cdf821-9503-4f68-a87f-9dc7a96ea032"></img> | | --> ## Changes <!-- Please describe things you've changed here, make a **high level** overview, if change is simple you can omit this section. For example: - Updated `about.md` docs --> <!-- ## Screenshots / GIFs Here you can add screenshots / GIFs documenting your change. You can add before / after section if you're changing some behavior. ### Before ### After --> ## Test code and steps to reproduce I've tried to test these changes carefully, however there still might be some bugs. It would be nice if we could get rid of them during beta-phase of 4.0. Below I paste my test matrix I conducted on `Test1649` on both platforms and architectures.  ## Checklist - [x] Included code example that can be used to test this change - [ ] Ensured that CI passes
alduzy
added a commit
that referenced
this pull request
Oct 10, 2024
## Description This PR fixes headerConfig's incorrect layout with custom subviews after recent changes in #2325. > [!note] @kkafar: Previously, before #2325, all children of the headerConfig component have been positioned absolutely, thus the headerConfig was always of height 0, not impacting layout of other components. After #2325, headerConfig's children are positioned using flexbox. This implies that it has no longer height of 0, thus it impacts the layout of other other elements, in particular `ScreenContentWrapper`, which is offset by the height of the highest header config subview. > > The initial idea to solve this was to set `height: 0; overflow: visible`, however, for some yet unknown reason the subviews become invisible with such styles set of headerConfig. Note that if you set the `height: 1` it works as expected. > > Due to above hindrance we decided to position the headerConfig approximately at the position of native header, by setting `top: -100%`. To prevent the headerConfig from blocking gestures we set `pointerEvents: 'box-none'`. > > In the end I want to note, that it would be best if we came out with solution that excludes headerConfig from layout as it was before #2325. ## Changes - added `Test2395.tsx` repro - adjusted headerConfig's styles ## Screenshots / GIFs ### Before  ### After  ## Test code and steps to reproduce - use `Test2395.tsx` repro ## Checklist - [x] Included code example that can be used to test this change - [x] Ensured that CI passes --------- Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Caution
This PR includes BREAKING CHANGES
Corresponding PR in
react-navigation:Fixes #1946
This PR refactors the header config component to use flex-box model instead of absolute positioning in Yoga layer. This is required so that the Yoga layouts children of header config with respect to one another (not absolutely), so that the title can be properly truncated.
In the end, the subviews are laid out by system anyway, thus we send additional information, such as padding / margins from native side to the shadow tree, so that we can adjust for these in Yoga layout.
Important
This PR introduces a bug in very first few frames - before the information from HostTree is propagated to ShadowTree the header title might not be truncated properly. This is known issue and based on the same mechanism and "jumping content" due to not including header dimensions in first Yoga layout.
Changes
Test code and steps to reproduce
I've tried to test these changes carefully, however there still might be some bugs. It would be nice if we could get rid of them during beta-phase of 4.0. Below I paste my test matrix I conducted on
Test1649on both platforms and architectures.Checklist