CommandBarFlyout keyboard navigation broken on RNW Islands app

[rozele]

Steps To Reproduce

Provide a detailed list of steps that reproduce the issue.

1. Run playground-win32
2. Open RNTester
3. Open a context menu on the TextInput for RNTester
4. I cannot navigate through the context menu using the keyboard (via arrow keys or tab).

Expected Results

I should be able to use the arrow keys or tab to navigate through context menu items.

Notes

1. This only occurs on CommandBarFlyout (MenuFlyout seems to work fine)
2. This only occurs on XAML Islands in the context of a react-native-windows app (cannot repro on a "standard" XAML islands app)
3. The right arrow key for sub-menu navigation seems to work when we apply Work around XAML Islands issues with TextCommandBarFlyout #8249 (but up/down arrow keys still do not work on the "main" CommandBarFlyout)

I considered filing this bug on the microsoft/microsoft-ui-xaml repo, but I am only able to reproduce the issue on react-native-windows.

Snack, code example, screenshot, or link to a repository:

In the video below, pressing Up/Down arrow keys does not allow navigation through the CommandBarFlyout items

2021-07-22.12-39-59.mp4

For context, here is the CommandBarFlyout working in a "standard" XAML Islands app:

2021-07-22.12-39-22.mp4

[chrisglein]

Let's reproduce this behavior in an RNW islands app and debug through what's going wrong.

[rectified95]

@rozele I'm not seeing the the 'Proofing' command, and the secondary sub-menu: did you customize the textbox in some way?

@asklar suggested it might be due to playground-win32 now running with the new WinUI progress ring, which your repro might not have included, but I still can't repro after checking out the repo at the point before that change was made.

[lyahdav]

@rozele I'm not seeing the the 'Proofing' command, and the secondary sub-menu: did you customize the textbox in some way?

Did you set the TextInput spellCheck property to true? It's from:

react-native-windows/vnext/Microsoft.ReactNative/Views/TextInputViewManager.cpp

Line 640 in 4aa8b3a

} else if (propertyName == "spellCheck") {

But I'm pretty sure this issue repros for Eric even if there is no Proofing submenu. Are you able to navigate that context menu with keyboard?

[rozele]

@rectified95 - I did not customize the TextBox. Did you force a typo into the TextBox? I still see the Proofing menu on latest main branch in Playground-win32

[lyahdav]

I was finally able to repro this consistently, but I don't know what changed that led to the bug starting to appear (I did not upgrade Windows 10 version). But, I was then able to root cause it. The issue seems to happen only to some users (not specific version of Windows 10). And I can fix the issue if I comment out this code in Application.manifest which sets <dpiAwareness>PerMonitorV2</dpiAwareness>:
https://github.com/microsoft/react-native-windows/blob/main/packages/playground/windows/playground-win32/Application.manifest#L34-L38

So I think this is actually a XAML bug, not RNW. But I noticed that when I created an Islands app from the VS template it didn't have that dpiAwareness in the Application.manifest, so I'm wondering if it makes sense to keep it in the RNW repo given this bug.

[lyahdav]

Now I know why the issue started happening to me. I started using a different monitor. Makes sense as this is a per-monitor DPI setting.

[lyahdav]

I was curious about other values for dpiAwareness and how it affects this bug so I tested them all:

dpiAwareness	Bug present?
<Not present>	No
unaware	No
system	Yes
PerMonitor	Yes
PerMonitorV2	Yes

[jonthysell]

I have tested playground win32 on my single 1080p monitor with no DPI scaling.

If I right-click on the text box, and there is only 1 item in the list (Paste), pressing up/down on the keyboard does not work to focus the item. If there are two or more items (there was text in the text box giving Paste/Cut/Copy, or I typed and deleted text giving Paste/Undo), pressing up does nothing while pressing down always selects the 2nd item, not the first.

Using a context-menu button on my keyboard, keyboard navigation works properly in all cases as the first item is immediately highlighted.

[lyahdav]

@jonthysell can you try with DPI scaling? And also this bug is only present when there are multiple menu items. I think it's expected behavior that pressing down selects the 2nd item. When the bug happens even that doesn't work.

[jonthysell]

@lyahdav well, it would make sense that pressing down selected the second item, if the first item was actually selected. nothing is visibly selected when using right-click, though the first item is when using the context-menu button. so there's still problems even without DPI scaling.

[lyahdav]

Using CommandBarFlyout from WinUI 2 (tested with version 2.6.1-prerelease.210709001) fixes this issue but leads to a crash when the flyout is closed. Here's a fork with that change: https://github.com/lyahdav/react-native-windows/tree/win-ui-2-cbf

[lyahdav]

Here's a branch with a workaround for the crash I mentioned in previous comment: https://github.com/lyahdav/react-native-windows/tree/win-ui-2-cbf-with-crash-on-flyout-closing-fix
The workaround was to add:

 flyout.Closing([](winrt::IInspectable const &sender, auto &&) {
    const auto &flyout = sender.as<winrt::Microsoft::UI::Xaml::Controls::TextCommandBarFlyout>();
    flyout.SecondaryCommands().Clear();
  });

Also for posterity, here's the crash exception info:

Exception thrown: read access violation.
this->m_sharedState.m_ptr was nullptr.

And call stack:

 	Windows.UI.Xaml.dll!CDependencyObject::NWPropagateDirtyFlag(DirtyFlags::Value flags) Line 3055	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CUIElement::NWSetDirtyFlagsFromChildAndPropagate(DirtyFlags::Value flags, unsigned int) Line 115	C++
 	Windows.UI.Xaml.dll!CUIElement::NWSetSubgraphDirty(CDependencyObject * pTarget, DirtyFlags::Value flags) Line 315	C++
 	Windows.UI.Xaml.dll!CUIElement::RemoveLayoutTransitionRenderer(CLayoutTransitionElement * pLayoutTransitionElement) Line 3575	C++
 	Windows.UI.Xaml.dll!CLayoutTransitionElement::DetachTransition(CUIElement * pTarget, CTransitionRoot * pParent) Line 11339	C++
 	Windows.UI.Xaml.dll!CFocusRectManager::ReleaseResources(bool isDeviceLost, bool cleanupDComp, bool clearRenderData) Line 576	C++
 	Windows.UI.Xaml.dll!CFocusRectManager::UpdateFocusRect(CCoreServices * core, CDependencyObject * focusedObject, CDependencyObject * focusTargetObject, DirectUI::FocusNavigationDirection focusNavigationDirection, bool cleanOnly) Line 645	C++
 	Windows.UI.Xaml.dll!CFocusManager::UpdateFocusRect(const DirectUI::FocusNavigationDirection focusNavigationDirection, bool cleanOnly) Line 2838	C++
 	Windows.UI.Xaml.dll!CCoreServices::NWDrawTree(HWWalk * pHWWalk, CWindowRenderTarget * pRenderTarget, VisualTree * pVisualTree, unsigned int forceRedraw, XRECT_WH * prcDirtyRect) Line 6376	C++
 	Windows.UI.Xaml.dll!CCoreServices::NWDrawMainTree(CWindowRenderTarget * pIRenderTarget, bool fForceRedraw, XRECT_WH * prcDirtyRect) Line 6084	C++
 	Windows.UI.Xaml.dll!CWindowRenderTarget::Draw(CCoreServices * fForceRedraw, unsigned int prcDirtyRect, XRECT_WH *) Line 136	C++
 	Windows.UI.Xaml.dll!CXcpBrowserHost::OnTick() Line 545	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::Tick() Line 1449	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::OnReentrancyProtectedWindowMessage(HWND__ * msg, unsigned int lParam, unsigned __int64) Line 1041	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CXcpDispatcher::ProcessMessage(HWND__ *) Line 890	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::WindowProc(HWND__ * hwnd, unsigned int msg, unsigned __int64 wParam, __int64 lParam) Line 839	C++
 	Windows.UI.Xaml.dll!CDeferredInvoke::DispatchQueuedMessage(bool * dispatchedWork, bool * hasMoreWork) Line 301	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CXcpDispatcher::MessageTimerCallback() Line 1534	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::MessageTimerCallbackStatic(void * myUserData) Line 1526	C++
 	CoreMessaging.dll!00007ffa44e153db()	Unknown
 	CoreMessaging.dll!00007ffa44e3c85b()	Unknown
 	CoreMessaging.dll!00007ffa44e1996b()	Unknown
 	CoreMessaging.dll!00007ffa44e18e26()	Unknown
 	CoreMessaging.dll!00007ffa44e17061()	Unknown
 	CoreMessaging.dll!00007ffa44e16e83()	Unknown
 	user32.dll!UserCallWinProcCheckWow()	Unknown
 	user32.dll!DispatchClientMessage()	Unknown
 	user32.dll!__fnDWORD�()	Unknown
 	ntdll.dll!00007ffa4a5b0c54()	Unknown
 	win32u.dll!00007ffa48341104()	Unknown
 	user32.dll!GetMessageW()	Unknown
>	Playground-win32.exe!RunPlayground(int showCmd, bool useWebDebugger) Line 406	C++
 	Playground-win32.exe!WinMain(HINSTANCE__ * instance, HINSTANCE__ * __formal, char * __formal, int showCmd) Line 442	C++
 	Playground-win32.exe!invoke_main() Line 107	C++
 	Playground-win32.exe!__scrt_common_main_seh() Line 288	C++
 	Playground-win32.exe!__scrt_common_main() Line 331	C++
 	Playground-win32.exe!WinMainCRTStartup(void * __formal) Line 17	C++
 	kernel32.dll!00007ffa49357034()	Unknown
 	ntdll.dll!00007ffa4a562651()	Unknown

[jonthysell]

This seems to have been fixed by switching to the WinUI CommandBarFlyout and the "new" crash was fixed in 2.7 prerelease.

Do we want to switch to WinUI for the flyout as per this change: lyahdav@a36d84c

[jonthysell]

Seeing as we just upgraded to WinUI 2.6 for RNW 0.66, which is already in preview and almost ready, I think it's a little late to jump again to 2.7 without more testing. @chrisglein can you move this to 0.67 if you agree?

[lyahdav]

@jonthysell I think we should switch to the WinUI 2 CommandBarFlyout for Text and TextInput context menu to fix this bug. I was eventually going to PR this, but it might not be for a while, in case someone else can get to it sooner.

[jonthysell]

Do you mean like this? I started this PR here based on what you did: #8723

I'm also working on a PR to upgrade us from MUX 2.6 to 2.7, but that might not make RNW 0.66.

[lyahdav]

@jonthysell yes, and as you said in that PR, you should wait until the MUX 2.7 upgrade before landing it.

[chrisglein]

Seeing as we just upgraded to WinUI 2.6 for RNW 0.66, which is already in preview and almost ready, I think it's a little late to jump again to 2.7 without more testing. @chrisglein can you move this to 0.67 if you agree?

Agreed. It seems too late to switch from 2.6 to 2.7. Let's target that jump for 0.67 so we can ensure it's stable. In the meantime any individual app can bump to 2.7 on their own.