Skip to content

Manually dismiss popups when the window moves, or the SUI scrolls#10922

Merged
16 commits merged intomainfrom
dev/migrie/b/9320-interfacial-separation-2
Aug 16, 2021
Merged

Manually dismiss popups when the window moves, or the SUI scrolls#10922
16 commits merged intomainfrom
dev/migrie/b/9320-interfacial-separation-2

Conversation

@zadjii-msft
Copy link
Member

Summary of the Pull Request

BODGY!

This solution was suggested in microsoft/microsoft-ui-xaml#4554 (comment).

When the window moves, or when a ScrollViewer scrolls, dismiss any popups that are visible. This happens automagically when an app is a real XAML app, but it doesn't work for XAML Islands.

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Unfortunately, we've got a bunch of scroll viewers in our SUI. So I did something bodgyx2 to make our life a little easier.

DismissAllPopups can be used to dismiss all popups for a particular UI element. However, we've got a bunch of pages with scroll viewers that may or may not have popups in them. Rather than define the same exact body for all their ViewChanging events, the HasScrollViewer struct will just do it for you!

Inside the HasScrollViewer stuct, we can't get at the XamlRoot() that our subclass implements. I mean, we can, but when XAML does it's codegen, XAML won't be able to figure it out.

Fortunately for us, we don't need to! The sender is a UIElement, so we can just get their XamlRoot().

So, you can fix this for any SUI page with just a simple

-    <ScrollViewer>
+    <ScrollViewer ViewChanging="ViewChanging">
-    struct AddProfile : AddProfileT<AddProfile>
+    struct AddProfile : public HasScrollViewer<AddProfile>, AddProfileT<AddProfile>

Validation Steps Performed

  • the window doesn't close when you move it
  • the popups do close when you move the window
  • the popups close when you scroll any SUI page

@ghost ghost added Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 11, 2021
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@DHowett
Copy link
Member

DHowett commented Aug 12, 2021

I had an even wackier idea.

Is it possible to do this?

<Page
    xmlns:local="whatever local is in these projects"
<ScrollViewer ViewChanging="local:StaticDismissHelperClass.ViewChanging">

(I wouldn't block for this suggestion, but it might make our lives easier if you can bind to like... static event handlers from other classes?)

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BODGY indeed haha

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 12, 2021
@ghost
Copy link

ghost commented Aug 12, 2021

Hello @zadjii-msft!

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.

@DHowett
Copy link
Member

DHowett commented Aug 12, 2021

CONFLICTS :O

@ghost ghost merged commit 29be856 into main Aug 16, 2021
@ghost ghost deleted the dev/migrie/b/9320-interfacial-separation-2 branch August 16, 2021 13:41
DHowett pushed a commit that referenced this pull request Aug 25, 2021
…0922)

BODGY!

This solution was suggested in microsoft/microsoft-ui-xaml#4554 (comment).

When the window moves, or when a ScrollViewer scrolls, dismiss any popups that are visible. This happens automagically when an app is a real XAML app, but it doesn't work for XAML Islands.

* upstream at microsoft/microsoft-ui-xaml#4554

* [x] Closes #9320
* [x] I work here
* [ ] Tests added/passed
* [ ] Requires documentation to be updated

Unfortunately, we've got a bunch of scroll viewers in our SUI. So I did something bodgyx2 to make our life a little easier.

`DismissAllPopups` can be used to dismiss all popups for a particular UI element. However, we've got a bunch of pages with scroll viewers that may or may not have popups in them. Rather than define the same exact body for all their `ViewChanging` events, the `HasScrollViewer` struct will just do it for you!

Inside the `HasScrollViewer` stuct, we can't get at the `XamlRoot()` that our subclass implements. I mean, _we_ can, but when XAML does it's codegen, _XAML_ won't be able to figure it out.

Fortunately for us, we don't need to! The sender is a UIElement, so we can just get _their_ `XamlRoot()`.

So, you can fix this for any SUI page with just a simple

```diff
-    <ScrollViewer>
+    <ScrollViewer ViewChanging="ViewChanging">
```

```diff
-    struct AddProfile : AddProfileT<AddProfile>
+    struct AddProfile : public HasScrollViewer<AddProfile>, AddProfileT<AddProfile>
```

* the window doesn't close when you move it
* the popups _do_ close when you move the window
* the popups close when you scroll any SUI page
@ghost
Copy link

ghost commented Aug 31, 2021

🎉Windows Terminal Preview v1.10.2383.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Aug 31, 2021

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost pushed a commit that referenced this pull request Oct 12, 2021
## Summary of the Pull Request
When the window moves, hide any visible content dialog (only one can be shown at a time) and ensure its associated async operation is terminated.

#10922 dismisses any open popups when the window is moved or any scroll viewer scrolls. However, if you just close a Popup from the UI tree, the async operation associated to a ContentDialog (started with `dialog.ShowAsync`) does not terminate. The dialog lock that prevents opening multiple dialogs at the same time is not released, and no further dialog can be shown. 
Explicitly dismissing the only visible ContentDialog using its `Hide` method terminates the operation.

## Validation Steps Performed
Manual tests, open up dialogs and move the window (like in #11425)

References #10922 
Closes #11425
PankajBhojwani pushed a commit that referenced this pull request Oct 13, 2021
## Summary of the Pull Request
When the window moves, hide any visible content dialog (only one can be shown at a time) and ensure its associated async operation is terminated.

#10922 dismisses any open popups when the window is moved or any scroll viewer scrolls. However, if you just close a Popup from the UI tree, the async operation associated to a ContentDialog (started with `dialog.ShowAsync`) does not terminate. The dialog lock that prevents opening multiple dialogs at the same time is not released, and no further dialog can be shown. 
Explicitly dismissing the only visible ContentDialog using its `Hide` method terminates the operation.

## Validation Steps Performed
Manual tests, open up dialogs and move the window (like in #11425)

References #10922 
Closes #11425
PankajBhojwani pushed a commit that referenced this pull request Oct 13, 2021
## Summary of the Pull Request
When the window moves, hide any visible content dialog (only one can be shown at a time) and ensure its associated async operation is terminated.

#10922 dismisses any open popups when the window is moved or any scroll viewer scrolls. However, if you just close a Popup from the UI tree, the async operation associated to a ContentDialog (started with `dialog.ShowAsync`) does not terminate. The dialog lock that prevents opening multiple dialogs at the same time is not released, and no further dialog can be shown. 
Explicitly dismissing the only visible ContentDialog using its `Hide` method terminates the operation.

## Validation Steps Performed
Manual tests, open up dialogs and move the window (like in #11425)

References #10922 
Closes #11425
ghost pushed a commit that referenced this pull request Jan 27, 2022
There are a couple places where we now bail immediately on startup, if we think the window is going to get created without any tabs. We do that to prevent a blank window from flashing on the screen when launching auto-elevate profiles. Unfortunately, those broke defterm in a particularly hard to debug way. In the defterm invocation, there actually aren't any tabs when the app completes initialization. We use the initialization to actually accept the defterm handoff. So what would happen is that the window would immediately close itself gracefully, never accepting the handoff.

In my defense, #8514, the original auto-elevated PR, predates defterm merging (906edf7) by a few months, so I totally forgot to test this when rolling it into the subsequent iterations of that PR.

* Related to: 
  * #7489
  * #12137 
  * #12205 
* [x] Closes #12267 
* [x] I work here
* [ ] No tests on this code unfortunately
* [x] Tested manually

Includes a semi-related code fix to #10922 to make that quieter. That is perpetually noisy, and when trying to debug defterm, you've only got about 30s to do that before it bails, so the `sxe eh` breaks in there are quite annoying.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-SettingsUI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

default profile menu once opened in startup settings goes above the window when scrolling down

3 participants