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

Disable focus-trap for Popover by default #3021

Merged
merged 1 commit into from
Aug 12, 2022

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Aug 12, 2022

This PR disabled the focus trap for Popovers by default for the moment. Reason is, that focusing other elements from within an Actions menu does not work anymore. E.g. here: https://github.com/nextcloud/tasks/blob/master/src/components/TaskBody.vue#L583-L588 the call to focus() has no effect.

Also see this example (copy into docs):

<template>
    <div>
        <input ref="input" />
        <Actions>
            <ActionButton @click="focusInput" :close-after-click="true">
                <template #icon>
                    <Delete :size="20" />
                </template>
                Focus input
            </ActionButton>
            <ActionButton @click="actionDelete">
                <template #icon>
                    <Delete :size="20" />
                </template>
                Delete
            </ActionButton>
        </Actions>
    </div>
</template>
<script>
import Delete from 'vue-material-design-icons/Delete'

export default {
	components: {
		Delete,
	},
	methods: {
		actionDelete() {
			alert('Delete')
		},
		async focusInput() {
			await this.$nextTick()
            this.$refs.input.focus()
		},
	},
}
</script>

The input should be focused when clicking on "Focus input" in the menu.

@raimund-schluessler raimund-schluessler added bug Something isn't working 3. to review Waiting for reviews feature: popover Related to the popovermenu component regression Regression of a previous working feature accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Aug 12, 2022
@raimund-schluessler raimund-schluessler added this to the 6.0.0 milestone Aug 12, 2022
@raimund-schluessler raimund-schluessler marked this pull request as ready for review August 12, 2022 11:47
@raimund-schluessler
Copy link
Contributor Author

I guess the problem here is that focus-trap by default returns the focus to whatever element had focus upon activation of the focus trap. Would it be ok to set returnFocusOnDeactivate to false for the focus-trap, or is this a feature one really wants?
Question would be how to then hand over the focus to another element after closing the menu. @vinicius73 @PVince81 @juliushaertl Any ideas / opinions?

@vinicius73
Copy link
Contributor

I guess the problem here is that focus-trap by default returns the focus to whatever element had focus upon activation of the focus trap. Would it be ok to set returnFocusOnDeactivate to false for the focus-trap, or is this a feature one really wants? Question would be how to then hand over the focus to another element after closing the menu. @vinicius73 @PVince81 @juliushaertl Any ideas / opinions?

This what the behavior who we want, but it should be optional.
We can provide a prop for this.

@raimund-schluessler
Copy link
Contributor Author

I guess the problem here is that focus-trap by default returns the focus to whatever element had focus upon activation of the focus trap. Would it be ok to set returnFocusOnDeactivate to false for the focus-trap, or is this a feature one really wants? Question would be how to then hand over the focus to another element after closing the menu. @vinicius73 @PVince81 @juliushaertl Any ideas / opinions?

This what the behavior who we want, but it should be optional. We can provide a prop for this.

Weird enough, setting returnFocusOnDeactive to false does not fix the issue, see #3024.

@vinicius73
Copy link
Contributor

We will merge it for now, and keep trying a solution for it.
It is important for accessibility issues.

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.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: popover Related to the popovermenu component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants