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

Allow starting selections from padding area #6343

Merged
2 commits merged into from
Jul 7, 2020
Merged

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Jun 4, 2020

WinUI's Margin and Padding work very similarly. Margin distances
ourselves from our parent. Whereas Padding distances our children from
ourselves.

Terminal's padding setting is actually implemented by defining
Margin on the SwapChainPanel. This means that the "padding" that is
created is actually belongs to SwapChainPanel's parent: Grid (not to be
confused with its parent, "RootGrid").

When a user clicks on the padded area, input goes to Grid. But there's a
twist: you can't actually hit Grid. To be able to hit Grid, you can't
just set IsHitTestVisible. You need to set it's Visibility to Visible,
and it's Background to Transparent (not null) [2].

Validation Steps Performed

  • Start a selection from the padding area
  • Click on a SearchBox if one is available
    • The SearchBox gets first dibs on the hit test so none gets through
      to the SwapChainPanel

References

[1] https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.uielement.ishittestvisible
[2] https://docs.microsoft.com/en-us/windows/uwp/xaml-platform/events-and-routed-events-overview#hit-testing-and-input-events

Closes #5626

@carlos-zamora carlos-zamora added Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) Product-Terminal The new Windows Terminal. labels Jun 4, 2020
@ghost ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. labels Jun 4, 2020
@carlos-zamora
Copy link
Member Author

⚠ This is what's left for me to implement, and why it's a Draft PR. ⚠
Last, we originally "selected" when the pointer drags a quarter of a cell (in TermControl). But we also clamp the converted "cell" to be on the buffer (in TerminalCore). This results in being able to select the first cell while you're still in the padding area. This needs to be fixed such that you have to actually be on the cell you're trying to select.

Marking as ready for review. The issue above just exposes #5099 more. I'm working on that for this milestone anyways.

@carlos-zamora carlos-zamora marked this pull request as ready for review June 26, 2020 18:29
@carlos-zamora carlos-zamora added this to the Terminal v1.2 milestone Jun 26, 2020
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sln/padding-sln branch from fceab19 to 0b2564d Compare June 26, 2020 18:32
@zadjii-msft zadjii-msft added Area-Input Related to input processing (key presses, mouse, etc.) and removed Area-Interaction Interacting with the vintage console window (as opposed to driving via API or hooks) labels Jul 7, 2020
@zadjii-msft zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Jul 7, 2020
@ghost ghost requested review from miniksa and leonMSFT July 7, 2020 17:05
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I don't understand exactly what's going on here. Can you explain who owns the padding and the margin now? You've removed the SwapChainPanel's margin from calculations, but you're .. still setting it? And the grid that is receiving input doesn't have a margin OR padding?

Can you still click in Search, even when MC is running?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 7, 2020
@carlos-zamora
Copy link
Member Author

I don't understand exactly what's going on here. Can you explain who owns the padding and the margin now? You've removed the SwapChainPanel's margin from calculations, but you're .. still setting it? And the grid that is receiving input doesn't have a margin OR padding?

GetPadding() is unchanged. So we still use SwapChainPanel().Margin(). I just replaced a few SwapChainPanel().Margin() with GetPadding() because I'm already here. So, when a user sets a padding, SCP pushes itself away from it's parent (Grid).

When a user interacts with that newly added space, you are not hitting SCP. So, you instead hit the parent (Grid). So I moved the pointer handlers up to the parent. Now, if you click on...

  • SCP: it falls through to the parent Grid because there's no handler (this case works, I'm just assuming this is how WinUI implemented it)
  • the "padding": you directly hit the Grid
  • Search Box: you hit the search box, where the event gets handled and goes no further

The last bit is that to actually pass a hit test, you need to be "visible". So, I made Grid Visible and Transparent.

Can you still click in Search, even when MC is running?

Works as expected :) mouse input is handled on the Search Box and does not leak through to the underlying MC. But if I click on MC while the Search Box is open, focus moves to MC and you actually interact with MC (same as before).

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 7, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 7, 2020
@ghost
Copy link

ghost commented Jul 7, 2020

Hello @DHowett!

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 63fbd9f into master Jul 7, 2020
@ghost ghost deleted the dev/cazamor/sln/padding-sln branch July 7, 2020 23:42
@ghost
Copy link

ghost commented Jul 22, 2020

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

Handy links:

@ghost ghost mentioned this pull request Jul 22, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start selection from padding
3 participants