Add support for React Native BackHandler API on Windows #4623

DrewHiggins · 2020-04-16T18:22:28Z

Adds support for React Native's BackHandler API, which will fire an event to its listeners when one of the events that the UWP documentation suggests listening for occurs. These are:

The BackRequested event, which covers a lot of input buttons for back, such as an Xbox Controller's B button or Backspace
A GoBack virtual key press
The Alt + Left arrow keyboard shortcut
The mouse back button, which maps to the XButton1 virtual key

The implementation of the BackHandler API for Windows is pretty much the same as that of Android in the react-native repo.

Microsoft Reviewers: Open in CodeFlow

Also remove debug code

vnext/src/Libraries/Utilities/BackHandler.windows.js

NickGerleman · 2020-04-16T20:21:56Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp


+void ReactRootControl::AttachBackHandlers(XamlView const &rootView) noexcept {
+  auto rootElement(rootView.as<winrt::UIElement>());
+  if (rootElement == nullptr) {


as will already throw if we can't QI (i.e. we terminate since it hits the noexcept boundary).

The non-throwing version is the try_as.

In reply to: 409825498 [](ancestors = 409825498)

Switched to try_as with a null check

@vmoroz was there a preference towards null-checking, or was pointing towards try_as more educational?

It's splitting hairs but I actually slightly preferred the terminating version.

NickGerleman · 2020-04-16T20:37:52Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

+      });
+
+  // Handle keyboard "back" button press
+  winrt::KeyboardAccelerator goBack{};


I'm not sure this is the right way to listen for the back action. From what I can find, SystemNavigationManager has a BackRequested event to express this intent. Can we hook into that instead?

The BackRequested event does not fire in all of the scenarios where the user might be using input devices to go back. The linked UWP documentation in the PR description goes into some more detail on this, and outlines this in a table:

Oh wow, you're right. I completely missed that in the description and in code. Thanks for linking it again.

It's unfortunate official reccomendations stray from the system API, but this makes sense then. I wonder a bit whether we could let developers opt into non system back events, but could see arguments both ways.

Consider to add a comment with a link.

In reply to: 409855996 [](ancestors = 409855996)

Added a comment with a link to the docs

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

vnext/ReactUWP/Views/TouchEventHandler.cpp

NickGerleman

Requesting changes (see comments)

* Copied in Android BackHandler code from upstream * Changed the base file for the BackHandler override * Fixed weak ptr check * Removed unnecessary validation after casting XamlView to UIElement * Call JS with legacyPointer

vmoroz · 2020-04-16T21:46:18Z

CodeFlow for GitHub does not download executable files. If you trust this file and would like to run it, please download it yourself through GitHub.

Please remove this file.

Refers to: vnext/ReactUWP/RCa15532:1 in cf8a17c. [](commit_id = cf8a17c, deletion_comment = False)

vmoroz · 2020-04-16T21:48:58Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

+  }
+
+  winrt::Windows::UI::Core::SystemNavigationManager::GetForCurrentView().BackRequested(
+      [this](winrt::IInspectable const & /*sender*/, winrt::BackRequestedEventArgs const &args) {


this [](start = 7, length = 4)

It is not safe. Please acquire std::weak_ptr instead and resolve it inside of lambda.

The same is for other case in this function.

Fixed: passed a weak ptr to this in both

NickGerleman · 2020-04-16T21:50:40Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

+void ReactRootControl::AttachBackHandlers(XamlView const &rootView) noexcept {
+  auto rootElement(rootView.as<winrt::UIElement>());
+  winrt::Windows::UI::Core::SystemNavigationManager::GetForCurrentView().BackRequested(
+      [this](winrt::IInspectable const & /*sender*/, winrt::BackRequestedEventArgs const &args) {


Do you know if it's safe to capture a raw pointer here? We should otherwise capture a weak ptr.

vmoroz · 2020-04-16T21:53:05Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

  m_SIPEventHandler->AttachView(xamlRootView, /*fireKeyboradEvents:*/ true);

  UpdateRootViewInternal();
+  AttachBackHandlers(xamlRootView);


AttachBackHandlers [](start = 2, length = 18)

We must also detach in the UninitRootView.

Added a function to remove the keyboard accelerators attached to the XAML root element and the BackRequested listener on uninitialization.

vmoroz · 2020-04-16T21:58:38Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

+      [this](winrt::KeyboardAccelerator const & /*sender*/, winrt::KeyboardAcceleratorInvokedEventArgs const &args) {
+        args.Handled(OnBackRequested());
+      });
+  rootElement.KeyboardAccelerators().Append(goBack);


Append [](start = 37, length = 6)

It also must be reversible upon unload.
The ReactRootControl could be initialized/uninitialized multiple times.

Added a function to remove the keyboard accelerators attached to the XAML root element on uninitialization.

vmoroz · 2020-04-16T22:05:05Z

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp

+    return false;
+  }
+
+  auto &legacyReactInstance = query_cast<Mso::React::ILegacyReactInstance &>(*m_weakReactInstance.GetStrongPtr());


auto & [](start = 2, length = 6)

Assigning a temporary to a reference is a recipe for disaster. VS must have a warning here.
See the ToggleInspector() for the correct usage.
You must keep the result of GetStrongPtr() in a variable when you call the query_cast. The query_cast does not increment the ref count.

Fixed to match ToggleInspector()

vmoroz

🕐

* Use weak pointers to this in event handlers * Fix safety issue in query_cast * Revoke handlers on Uninit * Remove added binary file

asklar · 2020-04-17T02:46:29Z

We should think about how this interacts with AppViewBackButton though?

vmoroz

NickGerleman · 2020-04-17T18:48:08Z

Taking a look at the validate-overrides issue. It's probably related to line ending handling in the tool.

ghost · 2020-04-17T20:39:45Z

Hello @NickGerleman!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

* Added initial implementation * Add backrequested listener * Add handlers for other back actions Also remove debug code * Change files * Remove TODO * Responding to PR comments * Copied in Android BackHandler code from upstream * Changed the base file for the BackHandler override * Fixed weak ptr check * Removed unnecessary validation after casting XamlView to UIElement * Call JS with legacyPointer * More PR feedback * Use weak pointers to this in event handlers * Fix safety issue in query_cast * Revoke handlers on Uninit * Remove added binary file * use weak ptr for BackRequested too * Fix override hash verification

DrewHigginsMSFT added 5 commits April 14, 2020 20:37

Added initial implementation

431cc69

Add backrequested listener

da8ae9b

Add handlers for other back actions

543ab74

Also remove debug code

Change files

6abf9c0

Remove TODO

cf8a17c

DrewHiggins requested a review from a team as a code owner April 16, 2020 18:22

DrewHiggins mentioned this pull request Apr 16, 2020

Support the Backhandler React Native API #4606

Closed

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vnext/src/Libraries/Utilities/BackHandler.windows.js Outdated Show resolved Hide resolved

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vnext/src/Libraries/Utilities/BackHandler.windows.js Show resolved Hide resolved

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp Outdated Show resolved Hide resolved

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp Outdated Show resolved Hide resolved

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vnext/ReactUWP/Views/TouchEventHandler.cpp Show resolved Hide resolved

NickGerleman suggested changes Apr 16, 2020

View reviewed changes

ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Apr 16, 2020

Responding to PR comments

f3f1316

* Copied in Android BackHandler code from upstream * Changed the base file for the BackHandler override * Fixed weak ptr check * Removed unnecessary validation after casting XamlView to UIElement * Call JS with legacyPointer

vmoroz reviewed Apr 16, 2020

View reviewed changes

NickGerleman reviewed Apr 16, 2020

View reviewed changes

vmoroz reviewed Apr 16, 2020

View reviewed changes

vmoroz requested changes Apr 16, 2020

View reviewed changes

ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Apr 16, 2020

More PR feedback

05e4608

* Use weak pointers to this in event handlers * Fix safety issue in query_cast * Revoke handlers on Uninit * Remove added binary file

ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Apr 16, 2020

use weak ptr for BackRequested too

7d04303

NickGerleman approved these changes Apr 17, 2020

View reviewed changes

vmoroz approved these changes Apr 17, 2020

View reviewed changes

DrewHiggins mentioned this pull request Apr 17, 2020

Share BackHandler Code Between Upstream Platforms #4629

Closed

Fix override hash verification

a4362cc

DrewHiggins force-pushed the jahiggin/backHandler branch from 9ac65dc to a4362cc Compare April 17, 2020 20:33

NickGerleman added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 17, 2020

ghost merged commit 7834106 into microsoft:master Apr 17, 2020

dannyvv mentioned this pull request Jan 29, 2021

Fix startup crash on RS3 #6980

Merged

This pull request was closed.

Add support for React Native BackHandler API on Windows #4623

Add support for React Native BackHandler API on Windows #4623

Uh oh!

Conversation

DrewHiggins commented Apr 16, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewHiggins Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NickGerleman left a comment

Choose a reason for hiding this comment

Uh oh!

vmoroz commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

asklar commented Apr 17, 2020

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

NickGerleman commented Apr 17, 2020

Uh oh!

ghost commented Apr 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrewHiggins commented Apr 16, 2020 •

edited by ghost

Loading

DrewHiggins Apr 16, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.