[Fabric] Text renders borders twice #13445
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
Is global.RN$Bridgeless the agreed upon JS check for new arch? Or just a convenient approximation?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Looks like we didn't have any snapshots for text with borders. Added a couple of the tests. |
For the fabric tests we need to make some of the test elements accessible - which adds another node in the non-fabric snapshots. -- The actual text elements in the paper snapshots are unchanged. |
* [Fabric] Text renders borders twice * Change files * Add fabric test for text borders * snapshot updates * snapshots
* [Fabric] Move to WinAppSDK types for KeyStatus, and ensure usages account for locked flag (#13435) * [Fabric] Move to WinAppSDK types for KeyStatus, and ensure usages account for locked flag * Change files * Use scrollEnabled flag to enable/disable the ScrollView (#13427) * Fix scrollview * Change files * Address feedback * Return false for scroll event handlers when scroll is disabled * Update scrollbar color based on scrollEnabled * Rename OnThemeChanged to UpdateColorForScrollBarRegions * [Fabric] Fix image component reference cycle (#13440) * [Fabric] Fix image component reference cycle * Change files * format * disable aggressive component deleted assert * Use weak_ref for image didReceiveImage callback * typo * Unsubscribe from imageresponseobserver when deleted - aligns with core * format * [Fabric] call reportMount to implement UIManagerMountHook support (#13443) * [Fabric] call reportMount to implement UIManagerMountHook support * Change files * Resolve Transform Matrix before Animation (#13450) * Resolve transform before animation * Change files * Minor naming fix * noexcept * [Fabric] Text renders borders twice (#13445) * [Fabric] Text renders borders twice * Change files * Add fabric test for text borders * snapshot updates * snapshots * fix change file types --------- Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com>
* [Fabric] Text renders borders twice * Change files * Add fabric test for text borders * snapshot updates * snapshots
* [Fabric] Patch yoga to include changes to correctly recalculate layout on scale change (#13407) * Patch yoga to handle dynamic scale changes * Change files * Update vnext/overrides.json Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/overrides.json Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/overrides.json Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/overrides.json Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/overrides.json Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/config/Config.cpp Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/config/Config.h Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/config/Config.h Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/node/LayoutResults.cpp Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * Update vnext/ReactCommon/TEMP_UntilReactCommonUpdate/yoga/yoga/node/LayoutResults.h Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * review feedback from yoga PR --------- Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> * [Fabric] Handle scale factor changes (#13406) * [Fabric] Handle scalefactor changes * Change files * [Fabric] Handle changes to BorderRadius (#13422) * Handle changes to borderradius * Change files * format * [Fabric] [Win32] Add image response APIs to win32 exports (#13428) * Add image response APIs to win32 exports * Change files * fix * [Fabric] Move to WinAppSDK types for KeyStatus, and ensure usages account for locked flag (#13435) * [Fabric] Move to WinAppSDK types for KeyStatus, and ensure usages account for locked flag * Change files * Use scrollEnabled flag to enable/disable the ScrollView (#13427) * Fix scrollview * Change files * Address feedback * Return false for scroll event handlers when scroll is disabled * Update scrollbar color based on scrollEnabled * Rename OnThemeChanged to UpdateColorForScrollBarRegions * [Fabric] Fix image component reference cycle (#13440) * [Fabric] Fix image component reference cycle * Change files * format * disable aggressive component deleted assert * Use weak_ref for image didReceiveImage callback * typo * Unsubscribe from imageresponseobserver when deleted - aligns with core * format * [Fabric] call reportMount to implement UIManagerMountHook support (#13443) * [Fabric] call reportMount to implement UIManagerMountHook support * Change files * Resolve Transform Matrix before Animation (#13450) * Resolve transform before animation * Change files * Minor naming fix * noexcept * [Fabric] Text renders borders twice (#13445) * [Fabric] Text renders borders twice * Change files * Add fabric test for text borders * snapshot updates * snapshots * align overrides for 0.75 branch * snapshot --------- Co-authored-by: Marlene Cota <1422161+marlenecota@users.noreply.github.com> Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com>
Description
In paper, the Text rendering was unable to render borders, so we wrap text with border with an additional View component to render the border. The fabric Text rendering is able to render the border, so we end up with the Text border and the View border.
Before:
After:
Microsoft Reviewers: Open in CodeFlow