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

[Peek] Update FilePreviewer to prevent tooltips from obscuring title bar controls #34718

Merged
merged 3 commits into from
Sep 23, 2024

Conversation

daverayment
Copy link
Contributor

Summary of the Pull Request

Update to prevent ImagePreview and VideoPreview controls' tooltips from obscuring the Main Window's title bar.

PR Checklist

Detailed Description of the Pull Request / Additional comments

When the user hovers over the top-right portion of an ImagePreview or VideoPreview, the associated tooltip is opened, with the text bound to InfoToolTip. This worked, but unfortunately the tooltip could obscure the Windows controls such as the close icon, which was an accessibility problem. This PR dynamically sets the Placement of the tooltip so it opens below the mouse pointer in the top portion of the previewers. The PointerMoved handler is generic, so will work with any future previewer which needs a tooltip.

The tooltips are proper child elements of the previewer controls now, as they need to be instantiated.

I updated the StringBuilder which appends the tooltip contents to have an initial capacity of 256 characters. This prevents the builder from resizing itself multiple times. This is a tiny perf/allocation improvement.

Validation Steps Performed

Manual tests only:

  • Tested with a mix of image and movie files.
  • Tested starting with a different previewer which doesn't use tooltips then navigating to an image or movie file which does need a tooltip.
  • Tested starting with an image or movie file then navigating to a different file type then opening another image or movie file.

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Sep 18, 2024
@jaimecbernardo
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Working well for me too. Thank you for the fix!

@jaimecbernardo jaimecbernardo merged commit a70aafb into microsoft:main Sep 23, 2024
9 checks passed
@daverayment
Copy link
Contributor Author

Thanks for the merge, @jaimecbernardo and sorry you had to fix up the XAML. Is the rule for props that they should be in alphabetical order?

@daverayment daverayment deleted the peek-infotip-fix branch September 24, 2024 16:10
@jaimecbernardo
Copy link
Collaborator

@daverayment , there's some forced rules by CI to make sure we don't get many merge issues due to concurrent XAML changes.
You can run .\.pipelines\applyXamlStyling.ps1 -Main in Powershell locally and it'll fix your XAML files as well :)
Anyway, when I see that CI is complaining on a PR I just do that myself and push ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants