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.

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

Fix Text running flattenStyle multiple times #14041

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

TatianaKapos
Copy link
Contributor

@TatianaKapos TatianaKapos commented Oct 29, 2024

Description

Resolved #14038, seems that I missed some PRs while implementing the new optimized Text and left in some dead Windows code that no longer gets used. This PR integrates https://github.com/facebook/react-native/pull/45340/files and https://github.com/facebook/react-native/pull/45345/files and removes an extra flattenStyle being used.

Will need to backport this to 0.76 before the release!

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

Resolves #14038

Testing

Tested the borders still work on Paper in rntester

Changelog

no

@TatianaKapos TatianaKapos marked this pull request as ready for review October 29, 2024 23:54
@TatianaKapos TatianaKapos requested a review from a team as a code owner October 29, 2024 23:54
@TatianaKapos TatianaKapos changed the title [Draft] Fix Text running flattenStyle multiple times Fix Text running flattenStyle multiple times Oct 29, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added Area: Paper Area: Text Area: Text Style Props https://reactnative.dev/docs/text-style-props Old Architecture Broad category for issues that apply to the RN "old" architecture of Cxx Modules + Paper labels Oct 29, 2024
@@ -144,25 +146,32 @@ const Text: React.AbstractComponent<TextProps, TextForwardRef> =

let _selectable = selectable;

const processedStyle = flattenStyle(_style);
let processedStyle = flattenStyle<TextStyleProp>(_style);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why moving to let instead of const? It seems like the variable isn't reassigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this is actually an upstream file: https://github.com/facebook/react-native/pull/45340/files, so this change comes from Meta!

@TatianaKapos TatianaKapos merged commit d929b3f into microsoft:main Oct 30, 2024
59 checks passed
@TatianaKapos TatianaKapos deleted the tk-text-style branch October 30, 2024 16:06
TatianaKapos added a commit to TatianaKapos/react-native-windows that referenced this pull request Oct 30, 2024
* integrate rn #45340 and #45345

* Change files

* remove dead windows code
TatianaKapos added a commit that referenced this pull request Oct 30, 2024
…14043)

* Fix Text running flattenStyle multiple times (#14041)

* integrate rn #45340 and #45345

* Change files

* remove dead windows code

* Change files
@slswalker
Copy link
Contributor

Thanks @TatianaKapos !

acoates-ms pushed a commit to acoates-ms/react-native-windows that referenced this pull request Dec 6, 2024
* integrate rn #45340 and #45345

* Change files

* remove dead windows code
acoates-ms added a commit that referenced this pull request Dec 9, 2024
* Fix issue with prop cloning with custom native props (#14061)

* Fix issue with prop cloning with custom native props

* format

* prettier

* Change files

---------

Co-authored-by: React-Native-Windows Bot <[email protected]>

* Export MS.RN.Color ctor in Office dll (#14082)

* Export MS.RN.Color ctor in Office dll

* Change files

* Implement TxScreenToClient and TxClientToScreen

* format

---------

Co-authored-by: React-Native-Windows Bot <[email protected]>

* TextInput caret becomes visible on non-focused TextInputs on resize (#14091)

* TextInput caret becomes visible on non-focused TextInputs on resize

* Change files

---------

Co-authored-by: React-Native-Windows Bot <[email protected]>

* [Fabric] Introducing autocapitalize prop in TextInput - Take 2 (#13343)

* New implementation of autocapitalize!

* Change files

* Fixed bug for sentences scenario

* Just keep characters mode for now

* Revert "Just keep characters mode for now"

This reverts commit 60ca1ce.

* Re-apply changes minus packages.json.lock

* The original js file was deleted, re-applying changes

* Fixed snapshot and lint errors

* Fix override mismatch, added comments

* Remove stale test check

* Minor changes

* Update obsolete snapshot

* Add very basic box-shadow support (#14028)

* Add very basic box-shadow support

* Change files

* Focus should notify island host when tab loop wraps to give host oportunity to take focus (#14026)

* Focus should notify island host when tab loop wraps to give host oportunity to take focus

* Change files

* Default scroll to bring a component into view should have padding around the viewport (#14018)

* Default scroll to bring a component into view should have padding around the viewport

* Change files

* Update focus visuals to use cornerRadius and inner/outer strokes (#14008)

* Update focus visuals to use cornerRadius and inner/outer strokes.

* Change files

* Format

* lint fix

* Scale focus border for scaleFactor

* Fix Text running flattenStyle multiple times (#14041)

* integrate rn #45340 and #45345

* Change files

* remove dead windows code

* Fixing text components not rendering a border with Paper (#14054)

## Description
Text components rendered in Paper need to have check if the style they are rendering contains border style props. This bug occurs when the passed in style is an array. From what I can tell, this bug has existed for a long time, possibly forever.

[4 Years Ago](946ba7f) This PR fixed borders not rendering for text, but did not address text components with an array of styles.
[3 Years Ago](ee4d83d#diff-5beb08e50a61de475cd20c6e68588ac3a5cd687a971883167fe75fc97a0dbdea) This PR changes naming, but not the logic.

This PR switches the style that is checked for border props from the one that is passed in to the one that is flattened.

This is the type of JSX that is broken. The text does not render a border.
```tsx
<Text style={[{ borderColor: "red", borderWidth: 3 }]}>{"hello world"}</Text>
```

### Type of Change
- Bug fix (non-breaking change which fixes an issue)

### Why
This reenables borders for Paper text components.

### What
Swapped a variable in text.window.js that is used to check for border style props.

## Screenshots
Add any relevant screen captures here from before or after your changes. 

## Testing
Local tests.

## Changelog
Fixing text components not rendering a border with Paper

---------

Co-authored-by: Sam Walker <[email protected]>
Co-authored-by: Jon Thysell <[email protected]>

* [Fabric] Get Modal to host RN components in new hwnd (#13500)

* save state

* add example

* build but blank page still :(

* clean up comments

* visuals show up in new hwnd!

* clean up code

* better naming and unfork Modal examples

* testing save state

* Make the RN island a Modal member var

* Failed attempt at skipping root view in CEH, leaving it for learning purposes

* you can click on UI!

* clean up code

* Change files

* save state

* remove hardcoded rootTag

* add width/height to example

* add test

* revert simple.tsx

* remove test

* update snapshot

* feedback part 1: make Modal a RootComponentView

* feedback part2: simplify MountChildren

* fix deleting modal

* feedback round2

* remove comment

* remove imports

* feedback part 3

* fix overrides

* add simple layout - still has issues with padding/flex

* feedback part4

* lint

* update overrides

* Change files

* feedback

---------

Co-authored-by: Daniel Ayala <[email protected]>

* Support accessibilityState 'checked' (#13962)

* Implement accessibilityState checked

* Change files

* Add Testing

* Format and Update Snapshots

* Adjust Guardrails

* Merge

* Format

* Format

* Lint

* Change files

* Fix Merge Error

* Fix focus visuals being obscured by adjacent views (#14093)

* Fix focus visuals being obscured by adjacent views

* Change files

* update snapshots

* Fix uimplemented view

* review feedback

---------

Co-authored-by: React-Native-Windows Bot <[email protected]>

* [Fabric] Fix Modal position, disable input to parent hwnd, and add onShow event (#14109)

* default to Modal opening in middle of app

* add onShow event

* Change files

* disable input to parent hwnd

* fix changefile

* Implement cursor property (#14141)

* Implement cursor property

* Change files

* Disable failing UT (#14153)

* Disable failing UT

* Change files

* [Fabric] Implement onDismiss for Modal and remove titlebar (#14126)

* add onDismiss event

* remove title bar

* Change files

* fix lint and snapshots

* add option for titlebar

* upgrade override

* Views with keyUpEvents/keyDownEvents set on them should form a stacking context (#14090)

* Key and mouse events require a stacking context

* Change files

* We dont need a stacking context to bubble JS events - but we do to modify the native handling of the events

* fix

* Fix change files

* build fix

* fixes

* fix

* format

---------

Co-authored-by: React-Native-Windows Bot <[email protected]>
Co-authored-by: Daniel Ayala <[email protected]>
Co-authored-by: Tatiana Kapos <[email protected]>
Co-authored-by: Sam Walker <[email protected]>
Co-authored-by: Sam Walker <[email protected]>
Co-authored-by: Jon Thysell <[email protected]>
Co-authored-by: Chiara Mooney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Paper Area: Text Style Props https://reactnative.dev/docs/text-style-props Area: Text Old Architecture Broad category for issues that apply to the RN "old" architecture of Cxx Modules + Paper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New text.windows.js code calls flatten on styles twice if not bridgeless
4 participants