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

[Run][Plugin Manager] Accessibility and UI #9653

Closed
mykhailopylyp opened this issue Feb 11, 2021 · 49 comments
Closed

[Run][Plugin Manager] Accessibility and UI #9653

mykhailopylyp opened this issue Feb 11, 2021 · 49 comments
Assignees
Labels
Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Run-Plugin Manager Issues with the PowerToys Run plugin manager

Comments

@mykhailopylyp
Copy link
Contributor

mykhailopylyp commented Feb 11, 2021

Link to parent issue #5273

  • Fix tab navigation for ListView. In list view, we do not navigate to the next list view item on tab. The only option is to use arrow up and down keys
  • When focusing on listview item, the narrator reads out Microsoft.PowerToys.Settings.UI.Library.ViewModels.PowerLauncherPluginViewModel. It should narrate the plugin's name
  • Change the color of selected item. It is tricky as Application.Current.Resources["SystemChromeMediumColor"] taken from code behind does not work for the dark theme.
  • Show warning when a plugin can not be reached from PT Run. Do not show the warning when a plugin is disabled
  • Consider validating Action keyword for * and ~
  • Reconsider author placement Basic settings for Plugin Manager #9650 (comment)
  • Navigation to next or previous item does not work if the focus is on controls from AdditionalInfoPanel
  • Plugin icons do not change on windows theme change without rendering the whole page again.
  • Consider showing a warning when all plugins are disabled
  • Consider disabling plugin controls if a plugin is disabled
  • Consider validation on textbox edit event. Show warning when plugin is not accessible #9747 (comment)
@mykhailopylyp mykhailopylyp added Product-PowerToys Run Improved app launch PT Run (Win+R) Window Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Feb 11, 2021
@mykhailopylyp mykhailopylyp self-assigned this Feb 11, 2021
@htcfreek
Copy link
Collaborator

Show warning when a plugin can not be reached from PT Run. Do not show the warning when a plugin is disabled

What is meant with this warning? Will the warning shown in PT Run in case of a plugin has an error?

We should have a visual hint in case the user disabled all plugins an PT Run has no usecase anymore.

@mykhailopylyp
Copy link
Contributor Author

Will the warning shown in PT Run in case of a plugin has an error?

No, The warning would be shown in settings.

What is meant with this warning?

We show that warning when a plugin is enabled and there is no way to get any result from it while using PT Run. In our case, Is Global checkbox is not checked and the action key is empty.

@mykhailopylyp
Copy link
Contributor Author

@niels9001

  • Fix tab navigation for ListView. In list view, we do not navigate to the next list view item on tab. The only option is to use arrow up and down keys

It looks like an issue with ListView because ListView in Color Picker settings has the same behavior. Any idea how to fix it? Maybe we can use different control?

@niels9001
Copy link
Contributor

@niels9001

  • Fix tab navigation for ListView. In list view, we do not navigate to the next list view item on tab. The only option is to use arrow up and down keys

It looks like an issue with ListView because ListView in Color Picker settings has the same behavior. Any idea how to fix it? Maybe we can use different control?

This is actually expected behavior (at least for UWP): tab control is used to move between controls. Navigation with keys is used to navigate within the control itself. For ListView, that means that navigation with key up/down is the way to go.

If we would introduce tab control, there's no way to move quickly to control that are below the ListView. Therefore, I'd say this requirement breaks guidelines and proper accessibility :). Do we actually need it?

@mykhailopylyp
Copy link
Contributor Author

If we would introduce tab control, there's no way to move quickly to control that are below the ListView. Therefore, I'd say this requirement breaks guidelines and proper accessibility :). Do we actually need it?

Good to know. I was mistakenly thinking that it is an accessibility issue.

@mykhailopylyp mykhailopylyp removed the Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams label Feb 15, 2021
@niels9001
Copy link
Contributor

Link to parent issue #5273

  • Change the color of selected item. It is tricky as Application.Current.Resources["SystemChromeMediumColor"] taken from code behind does not work for the dark theme.

@mykhailopylyp What will this do? Is it the background when a plug-in is expanded? If so, I think a ListView will already do this as part of its template?

For example, devices page in W10 Settings:

image

@mykhailopylyp
Copy link
Contributor Author

mykhailopylyp commented Feb 15, 2021

@niels9001

Is it the background when a plug-in is expanded?

Yes

If so, I think a ListView will already do this as part of its template?

We set option SelectionMode="None" so we do not have default behavior. That option was added because multiple select adds checkboxes and in order to get rid of them, we have to specify ControlTemplate which would be verbose. Maybe we can consider SelectionMode="Single"...

UPD: Is it possible to make the kind of if statement in xaml or something similar?

<StackPanel Background="{Collapsed ? Transparent : SystemChromeMediumColor}">
<\StackPanel>

@mykhailopylyp
Copy link
Contributor Author

Consider validating Action keyword for * and ~

@enricogior @davidegiacometti
It seems like there is no need to validate action keywords at all. Suppose a user uses ~ as an action keyword for some plugin then the folder plugin will not be shown. Any concerns?

@davidegiacometti
Copy link
Collaborator

@mykhailopylyp Folder plugin should work but shouldn't be able to parse ~ as user home.
I expect the same behaviour with \ and network shares.

@niels9001
Copy link
Contributor

@niels9001

Is it the background when a plug-in is expanded?

Yes

If so, I think a ListView will already do this as part of its template?

We set option SelectionMode="None" so we do not have default behavior. That option was added because multiple select adds checkboxes and in order to get rid of them, we have to specify ControlTemplate which would be verbose. Maybe we can consider SelectionMode="Single"...

See GIF here: #9715 (comment)

Isn't this the behavior we want? So it already works out of the box right? Maybe I'm misunderstanding :)?

@enricogior
Copy link
Contributor

@mykhailopylyp

t seems like there is no need to validate action keywords at all. Suppose a user uses ~ as an action keyword for some plugin then the folder plugin will not be shown. Any concerns?

I think we need to reduce the amount of work we do to support the community. So I strongly suggest that we don't allow certain keys or we document very well the side effect of using those keys.

@htcfreek
Copy link
Collaborator

@mykhailopylyp

t seems like there is no need to validate action keywords at all. Suppose a user uses ~ as an action keyword for some plugin then the folder plugin will not be shown. Any concerns?

I think we need to reduce the amount of work we do to support the community. So I strongly suggest that we don't allow certain keys or we document very well the side effect of using those keys.

I prefer blocking this keys and showing a hint that the key isn't allowed.
If we only document the keys we may get the issues that something isn't working and we have to answer and close them.

@mykhailopylyp
Copy link
Contributor Author

Isn't this the behavior we want? So it already works out of the box right? Maybe I'm misunderstanding :)?

@niels9001
Right now an item is highlighted only if it is hovered. I want it to be highlighted when it is hovered or selected. An item becomes selected when someone clicks on it and more information is shown.

@niels9001 niels9001 changed the title [Run][Plugin Manager] Accessability and UI [Run][Plugin Manager] Accessibility and UI Feb 16, 2021
@niels9001
Copy link
Contributor

Isn't this the behavior we want? So it already works out of the box right? Maybe I'm misunderstanding :)?

@niels9001
Right now an item is highlighted only if it is hovered. I want it to be highlighted when it is hovered or selected. An item becomes selected when someone clicks on it and more information is shown.

Ah yes, I see what you mean now. I have moved some of the selection logic to enable this. Here's the PR: #9735

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 16, 2021

We should add/show the following warning message between settings group header and the first plugin, if all plugins are disabled: "You can not use PT Run. Please enable at least one plugin."

An other possible solution would be to show a warning in PT Run's result list if no plugin is enabled or accessible (no activation phrase and no global results).

@htcfreek
Copy link
Collaborator

Something I thought about now: Do we disable or collapse the expanded per plugin settings (action phrase, global results) if the plugin is disabled?

@mykhailopylyp
Copy link
Contributor Author

Something I thought about now: Do we disable or collapse the expanded per plugin settings (action phrase, global results) if the plugin is disabled?

We do not do anything

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 17, 2021

Something I thought about now: Do we disable or collapse the expanded per plugin settings (action phrase, global results) if the plugin is disabled?

We do not do anything

I think we should do it. It makes no sense to configure this settings if the plugin is disabled.

I prefer disabling the controls.
Reason: If a plugin is disabled for example because it has an problem, it's possible to look what is set for this plugin anyway and the user can better configure an other (new) plugin.

@davidegiacometti
Copy link
Collaborator

@mykhailopylyp I was playing with feature/plugin-manager
If we add a list of exclusion for action keyworkds we should add \ since it breaks shared folders and OS drive paths.

@htcfreek
Copy link
Collaborator

@mykhailopylyp , @niels9001 , @enricogior
At the moment we have the plugin settings group between "Search & results" and "Appearance". I think we should move it at the end after "Appearance" because "Plugins" is the biggest settings group.
What's your thoughts on this?

@enricogior
Copy link
Contributor

@htcfreek
good call.

@htcfreek
Copy link
Collaborator

@niels9001

Consider disabling plugin controls if a plugin is disabled

Is it possible to disable the image so it looks like PowerToy is off?
image

I tried something like

<ContentControl IsEnabled="False">
    <Image/>
</ContentControl>

When I disable PT Run the "Plugins" header is disabled, but not the "no plugins" warning:
image

(For the not disable color links I will open an issue.)

@mykhailopylyp
Copy link
Contributor Author

@htcfreek

I have fresh compiled PT and got this:
image

How do we initialize the plugin list on a fresh install?

You do not have any plugins in the list, right?
Check if PT Run still works. If you tested PRs into plugin-manager branch it can be the reason. In such case just delete "PowerToys Run\settings.json".

@mykhailopylyp
Copy link
Contributor Author

How do we initialize the plugin list on a fresh install?

It is initialized by PT Run process if the list does not exist or is empty.

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 24, 2021

@htcfreek

I have fresh compiled PT and got this:
image
How do we initialize the plugin list on a fresh install?

You do not have any plugins in the list, right?
Check if PT Run still works. If you tested PRs into plugin-manager branch it can be the reason. In such case just delete "PowerToys Run\settings.json".

I have investigate this. It is a problem after (to fast) first launch of settings. When I open settings and then navigate to pt run there are now plugins. After switching the page and going back to pr run the list is there. (I can reproduce it by deleting "C:\Users\Heiko\AppData\Local\Microsoft\PowerToys\PowerToys Run\settings.json".)

Does a "loading plugins" circle makes sense here?

@htcfreek
Copy link
Collaborator

Should we update the description of the "Url handler" plugin from "Handels urls" to "Opens urls in your default browser."?

@htcfreek
Copy link
Collaborator

Should we update the description of the "Url handler" plugin from "Handels urls" to "Opens urls in your default browser."?

Btw: In Edge "url" is written as "URL".

@htcfreek
Copy link
Collaborator

@niels9001, @enricogior , @mykhailopylyp
Disabling the module doesn't collapse the extended plugin settings. This is first happen on switching the page.
Is this something we should fix?

image

@mykhailopylyp
Copy link
Contributor Author

@htcfreek

Does a "loading plugins" circle makes sense here?

We should open an issue for it.

@enricogior
Copy link
Contributor

@mykhailopylyp

We should open an issue for it.

Is it expected that it takes so long to load?

@mykhailopylyp
Copy link
Contributor Author

@enricogior

Is it expected that it takes so long to load?

No, I did not expect that this will happen. Actually, I tried to reproduce it before.
We can not do much about it now as settings are created by the PowerLauncher process.

@mykhailopylyp
Copy link
Contributor Author

@htcfreek

Is this something we should fix?

IMO we should leave it as it is.

@htcfreek
Copy link
Collaborator

@enricogior

Is it expected that it takes so long to load?

No, I did not expect that this will happen. Actually, I tried to reproduce it before.
We can not do much about it now as settings are created by the PowerLauncher process.

Can we add a text that is shown when no plugins available to display (instead of empty list and error text): <circle dots> PT Run is initialising. Please come back in a few minutes.

@enricogior
Copy link
Contributor

@htcfreek @mykhailopylyp
let's open a dedicated issue and let's try to fix it instead of spending time on providing a cover up for it.

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 24, 2021

@htcfreek @mykhailopylyp
let's open a dedicated issue and let's try to fix it instead of spending time on providing a cover up for it.

It would be nice if someone of you open it.

If we can't fix it before release, we should warn the user about this known issue.

@mykhailopylyp
Copy link
Contributor Author

@enricogior @niels9001

Consider validation on textbox edit event. #9747 (comment)

Is it a real issue? It looks like default behavior and I would not change it unless it is common.

@htcfreek
Copy link
Collaborator

@enricogior
Beside the settings bug when initialize the plugin list, we here have two remaining work items:

Shold I open new issues (one per work item)?

@enricogior
Copy link
Contributor

@htcfreek
yes please, we will close this issue and new issues should be one for each bug.

@htcfreek
Copy link
Collaborator

@enricogior
All the new issues for each of the remaining work items in this issue are created.

@enricogior enricogior added Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Run-Plugin Manager Issues with the PowerToys Run plugin manager and removed Status-In progress This issue or work-item is under development labels Feb 27, 2021
@crutkas
Copy link
Member

crutkas commented Mar 4, 2021

Great news! This was resolved in the newly released 0.33 version of PowerToys! Head to https://aka.ms/installpowertoys to grab latest.

@crutkas crutkas closed this as completed Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Fix Committed Fix is checked in, but it might be 3-4 weeks until a release. Run-Plugin Manager Issues with the PowerToys Run plugin manager
Projects
None yet
Development

No branches or pull requests

6 participants