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

TatianaKapos · 2024-11-19T22:01:43Z

Description

Implements onDismiss for Modal and removed the titlebar (Both Flyout/Popup/Modal don't have their own titlebar so by default Modal shouldn't have one either but it's easy to add it back in if needed! My guess is that the titlebar should be a windows-specific property).

This also means you no longer get the visual indicator that Modal needs to be dealt with before closing but Flyout/Popup also don't give a visual indicator.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Why

Resolves #13762

What

onDismiss doesn't work if you just implement the native-side because the Fabric-UI Manager destroys the Modal before the event can be fired. This PR enables the JavaScript portion of onDismiss for Modal that upstream implemented (see facebook/react-native#42601)

Screenshots

Testing

tested locally

Changelog

yes

Implements onDismiss for Modal and removes titlebar

Microsoft Reviewers: Open in CodeFlow

jonthysell · 2024-11-22T22:31:19Z

vnext/Microsoft.ReactNative/Fabric/Composition/Modal/WindowsModalHostViewComponentView.cpp

@@ -95,7 +100,7 @@ void WindowsModalHostComponentView::EnsureModalCreated() {
  m_hwnd = CreateWindow(
      c_modalWindowClassName,
      L"React-Native Modal",
-      WS_OVERLAPPEDWINDOW,
+      (WS_POPUP),


Is it this that causes the titlebar to disappear?

yes! And another line for adjusting the size for the titlebar but just added them behind m_showTitleBar!

jonthysell · 2024-11-22T22:35:10Z

We should probably (at least) have an internal m_showTitleBar member bool, maybe default to false (if that's the default behavior we want), and use that variable to control whether or not there's a title bar. Then maybe you could have whatever new title bar property you create (to lets users change it) be in a future PR.

…#14126) * add onDismiss event * remove title bar * Change files * fix lint and snapshots * add option for titlebar * upgrade override

* [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 * fix change files * Core didn't have most values of cursor yet * Fix overrides --------- Co-authored-by: Tatiana Kapos <[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 * [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 * fix change files * Update overrides --------- Co-authored-by: Tatiana Kapos <[email protected]>

* 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]>

TatianaKapos added 3 commits November 18, 2024 16:38

add onDismiss event

edfe358

remove title bar

6f4cefe

Change files

0546fa0

TatianaKapos requested a review from a team as a code owner November 19, 2024 22:01

Merge branch 'main' into tk-modal3

a51ab4e

microsoft-github-policy-service bot added Area: Component Views Area: Fabric Support Facebook Fabric Area: Modal New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric labels Nov 19, 2024

TatianaKapos added 3 commits November 22, 2024 11:21

merger

9d94ac6

fix lint and snapshots

7891343

fix merge

ebdab1d

jonthysell reviewed Nov 22, 2024

View reviewed changes

TatianaKapos added 4 commits November 25, 2024 15:49

add option for titlebar

0222ae9

Merge branch 'main' into tk-modal3

577ad59

upgrade override

be7dcce

Merge branch 'main' into tk-modal3

7f0f505

jonthysell approved these changes Dec 4, 2024

View reviewed changes

TatianaKapos merged commit d7b4b48 into microsoft:main Dec 4, 2024
59 checks passed

TatianaKapos deleted the tk-modal3 branch December 4, 2024 00:54

acoates-ms mentioned this pull request Dec 6, 2024

[0.74] Cursor support #14169

Merged

acoates-ms mentioned this pull request Dec 6, 2024

[0.75] Cursor support #14170

Merged

acoates-ms mentioned this pull request Dec 6, 2024

[0.76] Cherry-pick changes #14171

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TatianaKapos commented Nov 19, 2024 •

edited

Loading

jonthysell Nov 22, 2024

TatianaKapos Nov 25, 2024

jonthysell commented Nov 22, 2024 •

edited

Loading

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

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

Conversation

TatianaKapos commented Nov 19, 2024 • edited Loading

Description

Type of Change

Why

What

Screenshots

Testing

Changelog

Microsoft Reviewers: Open in CodeFlow

jonthysell Nov 22, 2024

Choose a reason for hiding this comment

TatianaKapos Nov 25, 2024

Choose a reason for hiding this comment

jonthysell commented Nov 22, 2024 • edited Loading

TatianaKapos commented Nov 19, 2024 •

edited

Loading

jonthysell commented Nov 22, 2024 •

edited

Loading