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

Proposal: Expose IsSubMenu to be a settable property in CPopup #820

Closed
alanlai-ms opened this issue Jun 7, 2019 · 15 comments
Closed

Proposal: Expose IsSubMenu to be a settable property in CPopup #820

alanlai-ms opened this issue Jun 7, 2019 · 15 comments
Assignees
Labels
area-Commanding AppBar, UICommand, MVVM, etc feature proposal New feature proposal team-Controls Issue for the Controls team

Comments

@alanlai-ms
Copy link

Proposal: [your title here]

Expose IsSubMenu to be a settable property in CPopup

Summary

Expose IsSubMenu to be a settable property in CPopup.
This is needed for using the new CommandBarFlyout and create complex scenario.

Rationale

In OneNote, I am observing the case that sometimes a (custom) sub menu that opens up from a commandBarFlyout, CommandbarFlyout itself would hide itself.

Debugging Windows code, it looks like the CommandBarFlyout is Hiding due to ::OnPopupLostFocus.

A comment within flyoutbase_partial.cpp says,
// We only want to close when we lose focus in the case where the newly focused element
// does not reside in any other flyout - otherwise, this will conflict with the functionality of child flyouts.

I have a case where OneNote code is focusing on an element that IS within a flyout but Hide is still getting called.

Looking further down the method,

if (!popupRootExists && (!popupAncestor || !popupAncestor->IsSelfOrAncestorLightDismiss()))
{ IFC_RETURN(Hide()); }

// We'll treat a popup associated with a submenu as light-dismiss
// because we know that the parent menu of a submenu is always light-dismiss.
XBOOL IsSelfOrAncestorLightDismiss() { return m_fIsLightDismiss || m_fIsSubMenu; }

We see that the Hide method is only getting skipped for a flyout that is lightDismiss.

But I dont want my flyout to be lightDismiss.
To solve this, I need a way to set the IsSubMenu to true for my flyout.

@alanlai-ms alanlai-ms added the feature proposal New feature proposal label Jun 7, 2019
@jevansaks
Copy link
Member

@alanlai-ms Can you elaborate a little more on the scenario? Making IsSubmenu settable seems like a solution based on how the code is currently written but logically I don't understand how it solves the scenario.

@jevansaks jevansaks added the area-Commanding AppBar, UICommand, MVVM, etc label Jun 7, 2019
@msft-github-bot msft-github-bot added the needs-assignee-attention Assignee needs to follow-up or investigate this issue label Jun 7, 2019
@YuliKl
Copy link

YuliKl commented Jun 7, 2019

But I dont want my flyout to be lightDismiss.

I would love to understand this better.

@alanlai-ms
Copy link
Author

@jevansaks This is essentially a bug fix kind of thing.

The scenario is we use appbarelementcontainer to host our own office buttons.
Those buttons can have fly out.

I am seeing bugs where when we set focus on the flyout, the commandbarflyout itself would Hide itself while our flyout ( which in fact is a submenu off of commandbarflyout ) is left open.

See this image, the red box is where the commandbarflyout should still be left open, but it is gone.
image

I don't want my flyout to be lightdismiss because if it were, users need an extra click to get rid of it.

Win32 Office apps aren't like that, Window explorer also isn't like that.

@YuliKl
Copy link

YuliKl commented Jun 7, 2019

I don't want my flyout to be lightdismiss because if it were, users need an extra click to get rid of it.

I'm sorry, I'm still not following. How do users close your non-light-dismiss flyout?

@alanlai-ms
Copy link
Author

We have our own LightDismissHandler.

It closes when user taps outside of the flyout, or clicks any of the button.

@YuliKl
Copy link

YuliKl commented Jun 7, 2019

We have our own LightDismissHandler.

It closes when user taps outside of the flyout, or clicks any of the button.

That sounds just like Xaml's light dismiss behavior... Can you use Xaml's built-in logic instead of something custom?

@alanlai-ms
Copy link
Author

Xaml's light dismiss behavior is eating the 1st click right?
I don't want the 1st click to be eaten.

@YuliKl
Copy link

YuliKl commented Jun 8, 2019

Yes, as I had explained in another comment, Xaml's light dismiss layer eats the click that dismisses it. This behavior is consistent across all UWP apps today. While I'm open to revisiting this choice for Xaml all up, I don't think the right answer is for OneNote to implement custom light dismiss behavior. Because the purpose of this proposal is to enable OneNote's hack, exposing IsSubMenu shouldn't be added to WinUI.

@alanlai-ms, please open a new issue to discuss the current light dismiss behavior. I would like to gauge community reaction to this kind of change.. If others agree with you that Xaml got this wrong, we should make the change in Xaml not in OneNote.

@YuliKl YuliKl added declined and removed needs-assignee-attention Assignee needs to follow-up or investigate this issue labels Jun 8, 2019
@msft-github-bot
Copy link
Collaborator

We appreciate the feedback, however this doesn’t currently align to the project’s goals and roadmap and so will be automatically closed. Thank you for your contributions to WinUI!

@alanlai-ms
Copy link
Author

@YuliKl, I'll open a new issue to discuss the light dismiss behavior.

Q1: But hmmm, why is having our own lightdimiss behavior a hack? An app should be allowed to do what we want?

Q2: But let me phrase my question another way, and since you closed this item, I think I will open a bug for this then?

The bug is, Hide is getting called even when new focus is getting set under a popup

Why is the above a bug?

  1. A windows dev told me he expects nothing to happen if the newly focused element is itself in a popup. "I believe what you’re describing would be expected to cause OnPopupLostFocus to be called – I’m not sure why it wouldn’t be doing so. It should be fine for that to be called, though – we only do anything in response to that if the newly focused element is not itself in a popup, so that should not block the scenario you’re describing."

  2. The OnPopupLostFocus method's comment states,
    A comment within flyoutbase_partial.cpp says,
    // We only want to close when we lose focus in the case where the newly focused element
    // does not reside in any other flyout - otherwise, this will conflict with the functionality of child flyouts.

@YuliKl
Copy link

YuliKl commented Jun 10, 2019

Q1: But hmmm, why is having our own lightdimiss behavior a hack? An app should be allowed to do what we want?

A platform should provide some level of consistency for its users. Users learn that their actions have a certain effect, and we want to allow them to apply these learnings to a variety of apps without having to re-adjust their expectations for each one. While apps are encouraged to provide unique experiences (e.g. in the area of branding), certain foundational behaviors ought not to be unique per app. For example, it would be very confusing to user if in some apps buttons were activated with a single click while in other apps with a double click. The light dismiss behavior is similarly foundational - we don't want to keep users guessing about its behavior across different apps. And when an app invents a rather elaborate workaround to deliberately change the default platform experience, I think that fits into the definition of "a hack".

@jevansaks
Copy link
Member

I'm seeing that Outlook and Word use the "eat-the-click" light dismiss behavior. Does OneNote have data showing that users prefer click-through behavior for menu dismissal?

@YuliKl
Copy link

YuliKl commented Jun 10, 2019

I'm seeing that Outlook and Word use the "eat-the-click" light dismiss behavior. Does OneNote have data showing that users prefer click-through behavior for menu dismissal?

This is a great question that I would love to discuss in a new issue so we can get input from the rest of the community as well. I'm sure users in this repo have an opinion and would like to weigh in, but "Expose IsSubMenu to be a settable property in CPopup" isn't a descriptive name for the "should light dismiss eat the first click" topic 😉

@alanlai-ms
Copy link
Author

alanlai-ms commented Jun 10, 2019

@jevansaks, Word Win32 right? Outlook Win32 right?
They don't eat the click.

Try right clicking somewhere and with context menu opened, click "new Email"
the new email registers

I don't have the data except bug report I have seen.

Does platform team has data that user prefers "eat the first click" then?
Why the change when existing Win32 Office app, as well as explorer.exe behaves as click-through?

@jevansaks
Copy link
Member

Weird! So in Outlook, clicking on the ribbon area works, but clicking through to the "Reply" button or the email message list doesn't work. So it's not even locally consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Commanding AppBar, UICommand, MVVM, etc feature proposal New feature proposal team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

4 participants