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

Auto focus on page load #3325

Merged
merged 7 commits into from
Aug 7, 2024
Merged

Auto focus on page load #3325

merged 7 commits into from
Aug 7, 2024

Conversation

AmelBawa-msft
Copy link
Contributor

@AmelBawa-msft AmelBawa-msft commented Jun 28, 2024

Summary of the pull request

References and relevant issues

https://task.ms/50153184

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated
  • Telemetry compliance tasks completed for added/updated events

@krschau
Copy link
Contributor

krschau commented Jun 28, 2024

Is this the same thing @dhoehna implemented manually? Can we replace that with this?

@dhoehna
Copy link
Contributor

dhoehna commented Jun 28, 2024

Is this the same thing @dhoehna implemented manually? Can we replace that with this?

Amir petitioned for behaviors instead of pages. I'm okay with either approach to auto focus as long as new pages only need to do one extra thing to get auto-focus behavior.

However, only one approach should be used.

@AmelBawa-msft how will new pages and existing pages opt-in to using auto-focus behavior?

@krschau
Copy link
Contributor

krschau commented Jun 28, 2024

@dhoehna @AmelBawa-msft Can we do both? I like how simple the behavior is / someday we could just get it from CommunityToolkit, but agree that we don't want every dev to have to add something to every page they create. Instead of putting <commonBehaviors:AutoFocusBehavior /> only on one page, should we put it on DevHomePage.xaml so everyone gets it?

@dhoehna
Copy link
Contributor

dhoehna commented Jun 28, 2024

An issue Amir had with my implementation is a lack of control. On the environments pages and the app pages, focus needs to be moved after the UI is loaded. If this change allows child pages to DevHome "Please load me know" cool.

Otherwise, I don't agree on having two solutions solving the same auto-focus problem. I am fine with either approach, but only 1.

EDIT:
Amir, if you feel strongly about your solution then please update all the pages and remove DevHome* views.

@AmelBawa-msft
Copy link
Contributor Author

@krschau , @dhoehna, we don't have a XAML for the parent class. I vaguely remember that inheritance was non-trivial or maybe not possible when the parent class has a corresponding XAML page. Agree with Darren, I do prefer a single solution, and they both seem to be doing the same identical thing, but one from XAML and the other from code-behind. I do prefer to put things in XAML as much as possible and maintain minimal code/logic in code-behind, promoting simplicity and reusability.

I will go ahead and update the other pages.

@dhoehna, I don't fully recall what was your use case, but I also found FocusBehavior:

This behavior sets the focus on the first control of Targets which accepts it. The focus will be set following the Targets order. The first control being ready and accepting the focus will receive it. The focus can be set to another control with a higher priority if it loads before FocusEngagementTimeout.

Usage:

<interactivity:Interaction.Behaviors>
    <behaviors:FocusBehavior>
        <behaviors:FocusTarget Control="{x:Bind myTextBox}" />
        <behaviors:FocusTarget Control="{x:Bind myButton}" />
    </behaviors:FocusBehavior>
</interactivity:Interaction.Behaviors>

<TextBox x:Name="myTextBox" />
<Button x:Name="myButton" />

@dhoehna
Copy link
Contributor

dhoehna commented Jun 28, 2024

@dhoehna, I don't fully recall what was your use case, but I also found FocusBehavior:

Shimmer and loading adaptive cards. In these cases the UI finished loading before theViewModel is done getting information. This causes the auto-focus to sometimes focus on the wrong element.

@AmelBawa-msft AmelBawa-msft marked this pull request as draft July 1, 2024 23:15
@AmelBawa-msft AmelBawa-msft marked this pull request as ready for review July 3, 2024 02:11
@AmelBawa-msft AmelBawa-msft added the Needs-Second Pull request that needs another approval label Aug 7, 2024
@AmelBawa-msft AmelBawa-msft added this to the Dev Home v0.17 milestone Aug 7, 2024
@krschau krschau removed the Needs-Second Pull request that needs another approval label Aug 7, 2024
@AmelBawa-msft AmelBawa-msft merged commit c126ed2 into main Aug 7, 2024
4 checks passed
@AmelBawa-msft AmelBawa-msft deleted the user/amelbawa/autofocus branch August 7, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants