Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Work around XAML Islands issues with TextCommandBarFlyout #8249

Merged
merged 6 commits into from
Aug 2, 2021

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Jul 14, 2021

As reported in microsoft/microsoft-ui-xaml#5341, the singleton TextCommandBarFlyout for the XAML TextBox and TextBlock components are limited to one window. When the flyout opens on a different window after already being opened on another, the first window the flyout was opened on gets focus, and the flyout is immediately closed. This change creates a TextCommandBarFlyout per TextInput native view and per selection TextBlock native view.

Additionally, it includes logic to work around an issue with the Proofing sub-menu flyout, which crashes due to an animation bug on Windows 10 versions lower than 21H1.

Microsoft Reviewers: Open in CodeFlow

@rozele rozele requested a review from a team as a code owner July 14, 2021 19:30
rozele added 2 commits July 14, 2021 16:41
As reported in
microsoft/microsoft-ui-xaml#5341, the
singleton TextCommandBarFlyout for the XAML TextBox and TextBlock
components are limited to one window. When the flyout opens on a
different window after already being opened on another, the first window
the flyout was opened on gets focus, and the flyout is immediately
closed. This change creates a TextCommandBarFlyout per TextInput native
view and per selection TextBlock native view.
@rozele rozele force-pushed the multiWindowFlyout branch from 98540d7 to ae2d021 Compare July 14, 2021 20:41
@rozele rozele force-pushed the multiWindowFlyout branch from 33c73d1 to 2de3624 Compare July 15, 2021 16:49
@rozele rozele changed the title Work around XAML Islands issue with TextCommandBarFlyout Work around XAML Islands issues with TextCommandBarFlyout Jul 15, 2021
@rozele rozele force-pushed the multiWindowFlyout branch 3 times, most recently from a01d4a9 to 93fa45e Compare July 15, 2021 17:32
@rozele rozele force-pushed the multiWindowFlyout branch from 93fa45e to a707b66 Compare July 15, 2021 17:35
@rozele
Copy link
Collaborator Author

rozele commented Jul 15, 2021

Easy way to repro these issues in playground-win32:

  1. Comment out the code that creates a new thread for new windows in Playground-win32.cpp:
    https://github.com/microsoft/react-native-windows/blob/main/packages/playground/windows/playground-win32/Playground-Win32.cpp#L133
  2. For the multi-window issue, load two instances of RNTester and attempt to show the context menu on each window. Note that the first window will get focused when you attempt to show the context menu on the second.
  3. For the Proofing menu crash, open RNTester, navigate to a TextInput field with autocorrect, type text that would have text suggestions, open the context menu and move the mouse around to open and close the proofing menu flyout multiple times on Windows 10 < 21H1.

@NickGerleman
Copy link
Collaborator

@rozele is this one ready for merge?

@rozele
Copy link
Collaborator Author

rozele commented Jul 20, 2021

@NickGerleman - going to implement @lyahdav's suggestion, then this should be ready 😅

Update: now it should be ready :)

@lyahdav
Copy link
Collaborator

lyahdav commented Aug 2, 2021

@NickGerleman Any update on this? It's blocking another PR I'd like to submit.

@NickGerleman NickGerleman merged commit 4aa8b3a into microsoft:main Aug 2, 2021
@NickGerleman
Copy link
Collaborator

Sorry, missed the notification that it is ready for merge. I don't think GH updates send notifications, so feel free to give me a mention is there is a change to an older PR, and I will take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants