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

Add tooltips to the Suggestions Control #14939

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 1, 2023

targets #14938

These are currently powered by the "ToolTip" element in the completion protocol. This also adds room for "description"s in any action. This will be more relevant for Tasks, in #1595.

By all accounts, this could have been in the parent commit. I broke it apart for the ease of reviewing.

@zadjii-msft
Copy link
Member Author

gh-3121-tooltips-for-pr

@github-actions

This comment has been minimized.

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.

Accessibility is gonna be rough here. You should test it out, but I assume TeachingTips aren't read out by screen readers. What you could do though, is apply that text to the AutomationProperties.FullDescription property. That way, the user can specifically request more info whenever they want it. That info shouldn't be read out automatically by the screen reader either. Only on request.

IsOpen="False"
PreferredPlacement="Right"
ShouldConstrainToRootBounds="False"
Subtitle="">
Copy link
Member

Choose a reason for hiding this comment

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

Is "Subtitle" necessary? Why?

Copy link
Member

Choose a reason for hiding this comment

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

Same with Title?

Also, looks like the WinUI docs don't say the default value (grr). So, mind checking each of these to make sure they're necessary?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 9, 2023
@zadjii-msft
Copy link
Member Author

I assume TeachingTips aren't read out by screen readers

gods I forgot they aren't. WHY ARENT THEY.

Also, Narrator and the Command Palette are already weird. Like, navigating the cmdpal with narrator never moves the selected item. It moves focus, but not the selected item.

So of course, for the tooltips, they too do not get updated with narrator open. Frick.

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 9, 2023
@zadjii-msft
Copy link
Member Author

Notes:

  • F6 to focus the TeachingTip doesn't work. It doesn't work because the SuggestionsControl::_lostFocusHandler fires, we don't find the tip as a child of the sxnui (it's a popup). We dismiss ourselves immediately.
    • Same with clicking on it.
    • This can be fixed by looking to see if the focused thing is a child of the
  • Closing the TeachingTip doesn't focus the sxnui again. The sxnui has lost focus (it lost focus to the popup), and the popup gave it to the TermControl, and now there's no way to dismiss the sxnui.
    • fixable. Toss focus manually.
  • OS dark theme, app in light theme? That's right, the Title of the TeachingTip will be white. Fucking themeing lambda again.
  • Change the app theme at runtime? sxnui doesn't reload it.
    • I don't believe the cmdpal does either tho
  • more?

I've got a vibe that this is too wacky to land for 1.19. I'm gonna close this one for now and come back to it.

Further reading:

@github-actions
Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view or the 📜action log for details.

Unrecognized words (2)

exersize
teachingtip

Previously acknowledged words that are now absent countof cpprest homeglyphs keyevent ONLCR OSCBG OSCCT OSCFG OSCRCC OSCSCB OSCSCC OSCWT pplx pseudocode testdlls Tlgdata websockets xxyyzz :arrow_right:
To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the git@github.com:microsoft/terminal.git repository
on the dev/migrie/f/3121-tooltips branch (ℹ️ how do I use this?):

curl -s -S -L 'https://github.com/raw/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/5823618460/attempts/1'
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

@zadjii-msft
Copy link
Member Author

Attempted resurrection in dev/migrie/fhl/sxnui-tooltips-3

image

Less wacky with the TeachingTip, but positioning the menu gets harder and I haven't solved that yet.

@zadjii-msft
Copy link
Member Author

AHAHAHA I finally got it to work right!

suggestions-tooltips-final
https://github.com/microsoft/terminal/compare/dev/migrie/f/sxnui-tooltips-4

github-merge-queue bot pushed a commit that referenced this pull request Jun 12, 2024
This adds a `"description"` property to actions. Notably, the shell
completion protocol (#3121) will now also populate that.

The suggestions UI can then use those descriptions to display an
additional tooltip with that information.

TeachingTip was kinda an abject disaster last time I tried this, so this
_isn't_ a TeachingTip. It's literally a text block.

xlinks:
* #13000
* #15845 
* #14939 - the last abandoned attempt at this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Automerge-Not-Compatible Needs-Attention The core contributors need to come back around and look at this ASAP. Needs-Tag-Fix Doesn't match tag requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants