Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

[Fabric] Text renders borders twice #13445

Merged
merged 5 commits into from
Jul 16, 2024
Merged

[Fabric] Text renders borders twice #13445

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9c49ee9
[Fabric] Text renders borders twice
acoates-ms Jul 15, 2024
d3931c7
Change files
acoates-ms Jul 15, 2024
35d625b
Add fabric test for text borders
acoates-ms Jul 15, 2024
961d149
snapshot updates
acoates-ms Jul 16, 2024
fee1554
snapshots
acoates-ms Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/react-native-windows-63f67386-8c58-4d24-88c1-84e80443b8d4.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "[Fabric] Text renders borders twice",
"packageName": "react-native-windows",
"email": "30809111+acoates-ms@users.noreply.github.com",
"dependentChangeType": "patch"
}
1 change: 1 addition & 0 deletions vnext/src-win/Libraries/Text/Text.windows.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ const Text: React.AbstractComponent<
} else {
let styleProps: ViewStyleProp = (restProps.style: any);
if (
global.RN$Bridgeless !== true && // [Windows] Fabric text handles borders, but on paper we need to wrap it in an extra view
Copy link
Member

Choose a reason for hiding this comment

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

Is global.RN$Bridgeless the agreed upon JS check for new arch? Or just a convenient approximation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not quite an accurate check. Core has specifically not exposed an agreed upon JS is fabric check, as core wants to avoid people doing JS checks for fabric. We still have a test flag to switch bridge mode back to use the bridge in fabric which would cause this check to be wrong. It is not a supported flag and requires magic strings to set - at this point I'm not sure if it even runs with that flag. I added it in when we first moved to bridgeless to help verify if fallout from bridgeless was caused by bridgeless - but maybe we should remove it now that fabric bridge mode has been completely untested for quite a while.

Copy link
Member

Choose a reason for hiding this comment

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

So based on core's opinion it sounds like there will never be a JS check for Fabric? Do we agree with that? If not, can we push for one? Maybe it's just Windows, and maybe it's only an interim state while Fabric parity is not where Paper parity was and apps/modules need to navigate that... but a JS check would be quite useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From Meta: Happy to provide some context, as this question has been asked multiple times in the past.

In general, we'd like to avoid exposing a "isBridgeless" or "isFabricEnabled" public API, as we'd like to avoid split implementations. In general APIs that are working on Bridgeless are working also on Bridge (the inverse is not true though).

For example for Android, the correct way to dispatch events is to use the EventDispatcher. That API works on both Bridgeless and Bridge mode.
We'd like for folks to dont' branch their code for old/new arch and instead migrate to those newer APIs.

--- That's the philosophical vision ---

Realistically speaking then, more complex libraries, need to know if they're running on a Bridgeless/NewArch environment or not. I'd say that the RN$Bridgeless global is not going away and is ok to use (we use it also internally).
We'd like to remove it in the long run though

styleProps &&
styleProps.borderColor &&
(styleProps.borderWidth ||
Expand Down
Loading