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

Don't return focus after close-after-click #3030

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

raimund-schluessler
Copy link
Contributor

As alternative to #3028, I would like to propose a less complex solution for the Actions component focus-trap problem in #3021. When an Action has the close-after-click prop set, the focus is not returned to the initial element after closing. This enables programmatically focusing another element (e.g. an input element) after the Actions menu is closed. Checkout the example added.

In difference to #3028 it does not introduce a scoped slot, hence is not a breaking change, but just keeps the current behaviour intact, without changes to the app.
On the other hand, It is a bit less flexible, since the dev cannot configure whether the focus is returned or not at the moment. But if one really needs this option, it could be implemented by simply adding a prop for it.

@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews feature: actions Related to the actions components discussion Need advices, opinions or ideas on this topic accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Aug 13, 2022
@raimund-schluessler raimund-schluessler added this to the 6.0.0 milestone Aug 13, 2022
@raimund-schluessler raimund-schluessler marked this pull request as ready for review August 13, 2022 12:42
@raimund-schluessler raimund-schluessler force-pushed the fix/noid/actions-focus-trap branch 3 times, most recently from 92823a3 to be3f1f7 Compare August 16, 2022 07:52
@vinicius73
Copy link
Contributor

It is a very good approach, thanks.
My only concern is it is a little implicit, and we can't avoid this behavior if we need.

@raimund-schluessler
Copy link
Contributor Author

My only concern is it is a little implicit, and we can't avoid this behavior if we need.

That's true. But we can easily add a prop to the actionText mixin allowing to configure the behavior, i.e. hand over a focusTrap config and use it here
https://github.com/nextcloud/nextcloud-vue/blob/6cf563b41e86cba9481167e936a6dc40f4c602a2/src/mixins/actionText.js#L85

Maybe something for a follow-up if it turns out to be necessary? Or do you prefer to implement this immediately (can do so in the evening, if required)?

@vinicius73
Copy link
Contributor

Maybe something for a follow-up if it turns out to be necessary? Or do you prefer to implement this immediately (can do so in the evening, if required)?

I'm suspect.

I am a big fan of composition over props...
But if it works well for your case, we can move forward.

@raimund-schluessler
Copy link
Contributor Author

Maybe something for a follow-up if it turns out to be necessary? Or do you prefer to implement this immediately (can do so in the evening, if required)?

I'm suspect.

I am a big fan of composition over props... But if it works well for your case, we can move forward.

It definitely fixes the issue I am facing in the Tasks app. I would say we move forward with this solution and iterate further if we find it creates problems.

Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Works, but I also noticed the keyboard nav is broken, when opening, the first item should be focused, it is not the case anymore.

  1. Open the menu
  2. Use arrow down to go to the second item
  3. See it doesn't work

You have to tab once to get into the first item THEN you can use arrows keys agai,

@raimund-schluessler
Copy link
Contributor Author

Works, but I also noticed the keyboard nav is broken, when opening, the first item should be focused, it is not the case anymore.

1. Open the menu

2. Use arrow down to go to the second item

3. See it doesn't work

You have to tab once to get into the first item THEN you can use arrows keys agai,

Indeed, but we have the same behavior on master, so not directly related to this PR. Could you open an issue, please?

@skjnldsv
Copy link
Contributor

Yep, this is why I approved nonetheless :)

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 17, 2022
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 2ea4bfb into master Aug 17, 2022
@PVince81 PVince81 deleted the fix/noid/actions-focus-trap branch August 17, 2022 14:49
Disabled button
</ActionButton>
</Actions>
<input ref="input" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@raimund-schluessler why do we have that in the documentation?
I feel this is not relevant with the ActionButton at all, no?
Feels like a corner case debug code leftover? 🤔 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess that depends, one could see it as an example how to focus the input field after clicking an action. But it's true, I mainly introduced it to check that focusing actually works.

Copy link
Contributor

Choose a reason for hiding this comment

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

I figured, removed in #4724
I think the example is too broad to have its place here. The nextTick and focus is common usage I would say :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working discussion Need advices, opinions or ideas on this topic feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants