TeachingTip: Pressing F6 now sets focus on custom content of light-dismissable tip #3545
Conversation
ghost
added
the
StephenLPeters
added
area-TeachingTip
team-Controls
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dev/TeachingTip/TeachingTip.cpp
Outdated
| if (const auto focusableElement = winrt::FocusManager::FindFirstFocusableElement(mainContentPresenter)) | ||
| { | ||
| // A focusable element can be a Hyperlink which does not expose a GettingFocus event. As such, we use the FocusManager API here. | ||
| auto const scopedRevoker = winrt::FocusManager::GettingFocus(winrt::auto_revoke, [this, focusableElement](auto const&, const winrt::GettingFocusEventArgs& args) { |
There was a problem hiding this comment.
Looks like I missed the fact that the FocusManager::GettingFocus API is only available starting with RS5 (hence the test failures on RS4 and below).
A possible alternative could be using the FocusManager::GetFocusedEelement API as that is available since day 1 Windows 10. The updated code could then look like this:
// If there is no close button, try setting focus on the tip's custom content.
else if (const auto mainContentPresenter = m_mainContentPresenter.get())
{
if (const auto focusableElement = winrt::FocusManager::FindFirstFocusableElement(mainContentPresenter))
{
const auto previouslyFocusedElement = winrt::FocusManager::GetFocusedElement();
const bool setFocus = SetFocus(focusableElement, winrt::FocusState::Keyboard);
if (setFocus)
{
if (const auto previouslyFocusedElementAsDO = previouslyFocusedElement.try_as<winrt::DependencyObject>())
{
m_previouslyFocusedElement = winrt::make_weak(previouslyFocusedElementAsDO);
}
}
args.Handled(setFocus);
}
}@StephenLPeters @ranjeshj Your thoughts?
There was a problem hiding this comment.
On another note, I have also noticed that pressing F6 to put focus on a tip in a Xaml Island scenario does not work (no matter if focus is already in the island or outside of it at the time of F6). Is that a known issue?
One implemention issue I see with Xaml island support is that we expect our focusable elements to be of type Windows.UI.Xaml.DependencyObject, yet, for example, a WPF button is of type System.Windows.DependencyObject.
So... is the current guidance that developers using a tip in a Xaml island need to take care of keyboard accessibility (F6) on their own? And if so, we should update the teaching tip documentation to reflect that.
There was a problem hiding this comment.
@JesseCol @Austin-Lamb for the Xaml Island behavior. I would like to avoid controls having to do special behavior for Islands as much as possible.
|
See my comment about the test failures here. |
|
@Felix-Dev Keith made a recent fix for the build pipelines. can you merge with master for your PRs ? |
|
@ranjeshj Merged with master and updated implementation to use FocusManager::GetFocusedElement() to resolve test failures. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hmmm...while moving to FocusManager::GetFocusedElement() fixed the test failure on RS4, we are still failing on RS3 and RS2. I checked the used FocusManager APIs again and both APIs are available since TS1 (first public Windows 10 version): Could there be a different popup (focus/ESC keyboard) behavior on RS3 and RS2? The test assumes that we can Other than perhaps some popup changes I am not immediately sure where else the cause of the RS3 and below test failure could lie, especially if the FocusManager APIs do indeed work as expected on these versions. @ranjeshj @StephenLPeters Any ideas? |
|
I'll take a look when this new build finishes, sorry the old one aged out. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@StephenLPeters Need to merge with master to resolve the merge conflicts...you should still have access to the recent test pipeline run then, correct? |
|
yes, I will |
|
Merged with master to resolve merge conflicts. |
dev/TeachingTip/TeachingTip.cpp
Outdated
| m_previouslyFocusedElement = winrt::make_weak(args.OldFocusedElement()); | ||
| }); | ||
| const bool setFocus = f6Button.Focus(winrt::FocusState::Keyboard); | ||
| args.Handled(setFocus); | ||
| } | ||
| // If there is no close button, try setting focus on the tip's custom content. | ||
| else if (const auto mainContentPresenter = m_mainContentPresenter.get()) |
There was a problem hiding this comment.
Instead of making a seperate else block for this case, can we just expand the lambda that sets the f6Button a bit? something like:
const winrt::Button f6Button = [this]() -> winrt::Button
{
auto firstButton = m_closeButton.get();
auto secondButton = m_alternateCloseButton.get();
//Prefer the close button to the alternate, except when there is no content.
if (!CloseButtonContent())
{
std::swap(firstButton, secondButton);
}
if (firstButton && firstButton.Visibility() == winrt::Visibility::Visible)
{
return firstButton;
}
else if (secondButton && secondButton.Visibility() == winrt::Visibility::Visible)
{
return secondButton;
}
else if (const auto mainContentPresenter = m_mainContentPresenter.get())
{
return const auto focusableElement = winrt::FocusManager::FindFirstFocusableElement(mainContentPresenter);
}
return nullptr;
}();
Then we shouldn't need this block at all
There was a problem hiding this comment.
@StephenLPeters Moved code into lambda as suggested. Note that I currently am solely using the FocusManager::GetFocusedElement() API to get the previously focused element as as the GettingFocus event is not available to all possibly focusably elements (such as a Hyperlink).
That said, if you prefer we use the UIElement.GettingFocus API wherever possible (and the documentation suggests this), I can add a check for the f6FocusableElement to see if it is a UIElement and if it is, we use the GettingFocus event instead.
|
@Felix-Dev the down level issue is pretty weird. The issue is that pressing F6 the second time isn't returning focus to the previously focused element. This is because previously focused element pointer we have is not a Control, Hyperlink, ContentLink or Webview, the acceptable types down level which you can see here: microsoft-ui-xaml/dev/inc/CppWinRTHelpers.h Line 223 in 7dbf5ff I'm hoping @Austin-Lamb or @ocalvo can shed light on where focus goes when a light dismiss popup is opened. However I suspect there wont be a way for us to work around this, but maybe pressing f6 a second time should close a light dismiss teaching tip so that focus can return to the app? |
|
@StephenLPeters Yes, I am seeing the reason for the test failure now, thanks for investigating! In the light-dismissable teaching tip case, opening such a tip sets focus to the tip's internal light-dismissable popup. As such, users can press, for example, the Esc key to close the tip. However, as the popup is not one of those UI elements which has an explicit Important here is that we make sure that setting focus on a light-dismissable tip's content still gives users the ability to close the tip via the keyboard. For example, the custom content could be a button which handles, for whatever reason, the Esc key pressed event and thus prevents the tip from being closed with the Esc key when focus is on that button. So in that case there has to be a guranteed way users can still close the tip via the keyboad. In light of this, your suggestion about closing the light-dismissable tip instead when pressing F6 a second time sounds like a possible solution for now. Presumably, users would only invoke F6 again on a tip to close it anyway (i.e. via the Esc key in a follow-up keyboard interaction) so we might as well shortcut that operation here. If we do opt for this approach, we should make sure to document this behavior. |
|
I wonder if we should rethink the f6 behavior for light dismiss ones. Maybe if the teaching tip is light dismiss we should always immediately put focus in the popup. I think that is better than focus being "lost" on the light dismiss popup. If we did this, we could have the first F6 close the popup? We would also need to change the accessibility announcement from "Press F6 to go to new notification from [App name]" |
|
@SavoySchuler fyi |
A light-dismissable tip does not need to have focusable content though so in that case, we would essentially have the same visual behavior as today, right? In the framework, we seem to use the Esc key to dismiss transient UI in a few places, like a Flyout/MenuFlyout:
Then there is also the case of light-dimissable popups which are currently dismissable by pressing the Esc key. In that sense, using F6 to close a transient UI like a light-dismissabe tip seems a bit...unexpected to me as a user when other transient controls use the Esc key among other user input events. |
|
I agree with your assessment. In that case can we make is so F6 doesn't do anything when the tip is in light dismiss mode? Assuming we move focus to its focusable content when it opens by default (if it has some). |
|
Hey all, thank you for great work here! I'm just joining the thread, so allow me to get caught up. I am to understand that currently F6 will not take you to a light-dismiss tip unless there is a close button? If that is the case, that is a major accessibility bug. The heading landmark navigation (Narrator + F6) was added as an accessibility requirement to give keyboard users a way to access TeachingTips. I think there are two questions:
I'm definitely catching up here so please let me know if I missed something or details we should discuss further! 🙂 Thanks again for catching this bug, it certainly sounds like there is something here worth fixing. |
|
I chatted with @YuliKl who added some context.
|
|
To 1:
Yes, this PR aims to fix a bug where users are currently unable to set focus into the tip via the keyboard when there is no close button for the tip. This is by default the case for light-dismissable tips so pressing F6 does not yet set focus inside the tip for them (which is the bug in question). Current Narrator output when navigating focus inside a tipI tested the narrator input and for tips with a close button (aka not light-dimissable), pressing F6 puts the focus on the close button and narrator reads: "tip...close...button" where the "..." are symbolizing an "audial pause". From the focused close button, users can then use the Tab key to move focus to focusable content of the tip (if any). Narrator only reads the content of that custom content out for now (i.e. "[button_content]...button"), so no additional notice of "Teaching tip content" or any of that sort. That is also the case when the tip does not have a close button, so F6 moves focus directly to the focusable content and narrator reads "tip...[button_content]...button". To 2:
@SavoySchuler @StephenLPeters To my understanding this means that for light-dismissable tips with focusable content we should implement the suggestion made by @StephenLPeters: Pressing F6 a second time closes the tip and sets focus back to the previously focused element (before the tip was opened - as is currently the case when closing a tip). Is that correct? How, then, about light-dismissable tips then which do not have any focusable content?
Keyboard support for closing a light-dismissable tip without any focusable contentSupporting both the Esc key and F6 to close a light-dimissable tip without any focusable content might not be a bad idea because it shares a keyboard closing mechanism (F6) with light-dismissable tips which do have custom content. As such, closing a light-dismissabe tip with F6 would always be an option, no matter if custom focusable content exists or not. Narrator output improvements about how to close a tip via keyboardWhen opening a tip, Narrator for now says users can press F6 to put focus inside the tip. If we allow F6 to close a light-dismissable tip, we should update the narrator output. So for light-dimissable tips with focusable content: "Press F6 to set focus inside the tip. Press F6 a second time to close the tip" -> something like that. As for light-dimissable tips without any focusable content: There is no need for narrator to announce to users to press F6 to focus content (as there is no such content). Instead, we should probably directly announce how users can close a light-dismissable tip (i.e. via the Esc key, or, if approved, pressing the F6 key). Thoughts? "Focus lost" visual effect for light-dismissable tipI also read the comments by @SavoySchuler and @YuliKl that when we open a light-dismissable tip, we do not yet put focus on the focusable content (if any). Setting focus via the keyboard can be done by pressing F6 for the first time (and users can then use the Tab key to navigate through all the focusable tip elements). As such, we would still have the appearance of "focus being lost" when opening a tip (as focus would be on the internal popup hosting the tip and enabling the user to close the tip via the Esc key). @SavoySchuler @StephenLPeters Am I reading this correctly here? Summary of my understanding about the current questions/suggestion we are facing as part of this PRWe should:
Additional Narrator output suggestion when opening a tipAdditionally, would it make sense to have Narrator announce the type of tip being opened? When opening a tip today (either light-dismissable or not), Narrator tells users that they can press F6 to set focus inside the tip. It does net tell the user whether that tip is a light-dimissable tip (so it can be closed via Esc key, clicking elsewhere on the app UI surface...) or not. The distinction between a tip and a light-dismissable tip for now is just a) the not-existing default close button for a tip and b) a different default tip background. However, that design can be changed by developers so it might not be a visual characteristic users can uniformly rely on. Having Narrator announce an opened tip as being light-dismissable could be of help here. @SavoySchuler @StephenLPeters Thoughts? |
|
+@YuliKl for a double check on this accessibility scenario. @Felix-Dev what a write-up! Love your energy here! Thank you a TON for your passion in making sure we have a great accessibility experience. 😄 I think I have a few different takeaways:
To restate what I think need to happen:
Does that make sense? I hope my actually making it easier to implement this change by reducing the number of changes we're considering. 🙂 I know teaching tip accessibility is complex, so please let me know if you have any questions here - I am happy to try and make sure we're all super clear. I'll be out of the office until the New Year but would be happy to set up time to talk directly once I'm back if that helps? 🙂 |
|
@Felix-Dev This issue seems to be resolved in main. Please feel free to reopen is the issue persists 😊 |
Description
This PR fixes a bug where pressing F6 while a light-dismissable teaching tip is displayed would not set focus to the tip's custom content (if it is focusable). Pressing F6 again moves focus away from the content back to the tip again so users can always close it using the Esc key, for example.
More generally speaking, if a tip does not have a close button, pressing F6 now sets focus on the custom content if possible. Typically, only light-dismissable tips don't have a close button.
Motivation and Context
Fixes #3257.
How Has This Been Tested?
Tested manually and added interaction test.
GIF: