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

Prevent context menu of tab renamer text box from canceling edit #8979

Merged
1 commit merged into from
Feb 1, 2021

Conversation

Don-Vito
Copy link
Contributor

PR Checklist

Detailed Description of the Pull Request / Additional comments

  • We dismiss the edit each time HeaderRenamerTextBox loses focus
  • Unfortunately, this applies also to scenario where the context menu
    (copy, paste, select, etc.) is open with the right-click
  • The fix is to ignore focus loss if HeaderRenamerTextBox().ContextFlyout() is open.
  • We can do it as upon the fly-out dismiss the text box regains the focus.

RenamerContextMenu

@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Feb 1, 2021
@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 1, 2021
@ghost
Copy link

ghost commented Feb 1, 2021

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 636f436 into microsoft:main Feb 1, 2021
// If the context menu associated with the renamer text box is open we know it gained the focus.
// In this case we ignore this event (we will regain the focus once the menu will be closed).
const auto flyout = HeaderRenamerTextBox().ContextFlyout();
if (flyout && flyout.IsOpen())
Copy link
Member

Choose a reason for hiding this comment

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

qq: this doesn't make it so if you open the flyout and then navigate away from the box by clicking the terminal or something that the renamer stays open, right?

Copy link
Contributor Author

@Don-Vito Don-Vito Feb 3, 2021

Choose a reason for hiding this comment

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

Emm.. this is a default behavior of the context flyout - to return to the previously focused element when collapsed (as collapse might happen also because "paste" was chosen). I mean, we do not cancel focus loss in this code and preserve default behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Just idiot thoughts

Copy link
Member

Choose a reason for hiding this comment

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

Holy crap, I just got autocorrected! I meant to type “idle” thoughts. 😬

@ghost ghost removed the Needs-Second It's a PR that needs another sign-off label Feb 3, 2021
DHowett pushed a commit that referenced this pull request Feb 5, 2021
## PR Checklist
* [x] Closes #8975
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.

## Detailed Description of the Pull Request / Additional comments
* We dismiss the edit each time `HeaderRenamerTextBox` loses focus
* Unfortunately, this applies also to scenario where the context menu
(copy, paste, select, etc.) is open with the right-click
* The fix is to ignore focus loss if `HeaderRenamerTextBox().ContextFlyout()` is open.
* We can do it as upon the fly-out dismiss the text box regains the focus.

![RenamerContextMenu](https://user-images.githubusercontent.com/4639110/106394866-90b10100-6407-11eb-8e92-627be4f70500.gif)

(cherry picked from commit 636f436)
@ghost
Copy link

ghost commented Feb 11, 2021

🎉Windows Terminal Preview v1.6.10412.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants