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

fix(Select): fix touch devices bugs #1472

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 8 additions & 23 deletions packages/radix-vue/src/Select/Select.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import Select from './story/_SelectTest.vue'
import type { DOMWrapper, VueWrapper } from '@vue/test-utils'
import { mount } from '@vue/test-utils'
import { nextTick } from 'vue'
import { fireEvent } from '@testing-library/vue'
import { handleSubmit } from '@/test'

describe('given default Select', () => {
Expand Down Expand Up @@ -34,10 +33,7 @@ describe('given default Select', () => {

describe('opening the modal', () => {
beforeEach(async () => {
await wrapper.find('button').trigger('pointerdown', {
button: 0,
ctrlKey: false,
})
await wrapper.find('button').trigger('click')
await nextTick()
})

Expand All @@ -51,16 +47,15 @@ describe('given default Select', () => {
})

it('should show the modal content', () => {
expect(wrapper.html()).toContain('Fruits')
expect(wrapper.html()).toContain('Apple')
})

describe('after selecting a value', () => {
beforeEach(async () => {
const selection = wrapper.findAll('[role=option]')[1];
(selection.element as HTMLElement).focus()
await selection.trigger('pointerup')
// Needs 2 pointerup because SelectContentImpl prevents accidental pointerup's
await fireEvent.pointerUp(selection.element)
await selection.trigger('click')
})

it('should show value correctly', () => {
Expand All @@ -74,7 +69,7 @@ describe('given default Select', () => {

describe('after opening the modal again', () => {
beforeEach(async () => {
await wrapper.find('button').trigger('pointerdown', {
await wrapper.find('button').trigger('click', {
button: 0,
ctrlKey: false,
})
Expand Down Expand Up @@ -111,16 +106,11 @@ describe('given select in a form', async () => {

describe('after selecting option and clicking submit button', () => {
beforeEach(async () => {
await wrapper.find('button').trigger('pointerdown', {
button: 0,
ctrlKey: false,
})
await wrapper.find('button').trigger('click')
await nextTick()
const selection = wrapper.findAll('[role=option]')[1];
(selection.element as HTMLElement).focus()
await selection.trigger('pointerup')
// Needs 2 pointerup because SelectContentImpl prevents accidental pointerup's
await fireEvent.pointerUp(selection.element)
await selection.trigger('click')
await wrapper.find('form').trigger('submit')
})

Expand All @@ -132,16 +122,11 @@ describe('given select in a form', async () => {

describe('after selecting other option and click submit button again', () => {
beforeEach(async () => {
await wrapper.find('button').trigger('pointerdown', {
button: 0,
ctrlKey: false,
})
await wrapper.find('button').trigger('click')
await nextTick()
const selection = wrapper.findAll('[role=option]')[4];
(selection.element as HTMLElement).focus()
await selection.trigger('pointerup')
// Needs 2 pointerup because SelectContentImpl prevents accidental pointerup's
await fireEvent.pointerUp(selection.element)
await selection.trigger('click')
await wrapper.find('form').trigger('submit')
})

Expand Down
6 changes: 1 addition & 5 deletions packages/radix-vue/src/Select/SelectContentImpl.vue
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,6 @@ watchEffect((cleanupFn) => {
}
}
const handlePointerUp = (event: PointerEvent) => {
// Prevent options from being untappable on touch devices
// https://github.com/unovue/radix-vue/issues/804
if (event.pointerType === 'touch')
return

// If the pointer hasn't moved by a certain threshold then we prevent selecting item on `pointerup`.
if (pointerMoveDelta.x <= 10 && pointerMoveDelta.y <= 10) {
event.preventDefault()
Expand Down Expand Up @@ -254,6 +249,7 @@ provideSelectContentContext({
<template>
<FocusScope
as-child
:trapped="rootContext.open.value"
@mount-auto-focus.prevent
@unmount-auto-focus="
(event) => {
Expand Down
37 changes: 23 additions & 14 deletions packages/radix-vue/src/Select/SelectItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export interface SelectItemProps extends PrimitiveProps {
<script setup lang="ts">
import {
computed,
nextTick,
onMounted,
ref,
toRefs,
Expand All @@ -55,8 +54,9 @@ const isFocused = ref(false)
const textValue = ref(props.textValue ?? '')
const textId = useId(undefined, 'radix-vue-select-item-text')

async function handleSelect(ev?: PointerEvent) {
await nextTick()
let pointerTypeRef: PointerEvent['pointerType'] = 'touch'

function handleSelect(ev?: PointerEvent) {
if (ev?.defaultPrevented)
return

Expand All @@ -66,30 +66,31 @@ async function handleSelect(ev?: PointerEvent) {
}
}

async function handlePointerMove(event: PointerEvent) {
await nextTick()
function handlePointerMove(event: PointerEvent) {
if (event.defaultPrevented)
return

pointerTypeRef = event.pointerType

if (disabled.value) {
contentContext.onItemLeave?.()
}
else {
else if (pointerTypeRef === 'mouse') {
// even though safari doesn't support this option, it's acceptable
// as it only means it might scroll a few pixels when using the pointer.
(event.currentTarget as HTMLElement).focus({ preventScroll: true })
}
}

async function handlePointerLeave(event: PointerEvent) {
await nextTick()
function handlePointerLeave(event: PointerEvent) {
if (event.defaultPrevented)
return
if (event.currentTarget === document.activeElement)
if (event.currentTarget === document.activeElement) {
contentContext.onItemLeave?.()
}
}

async function handleKeyDown(event: KeyboardEvent) {
await nextTick()
function handleKeyDown(event: KeyboardEvent) {
if (event.defaultPrevented)
return
const isTypingAhead = contentContext.searchRef?.value !== ''
Expand Down Expand Up @@ -145,11 +146,19 @@ provideSelectItemContext({
:as-child="asChild"
@focus="isFocused = true"
@blur="isFocused = false"
@pointerup="handleSelect"
@click="() => {
if (pointerTypeRef !== 'mouse') {
handleSelect()
}
}"
@pointerup="() => {
if (pointerTypeRef === 'mouse') {
handleSelect()
}
}"
@pointerdown="(event) => {
(event.currentTarget as HTMLElement).focus({ preventScroll: true })
pointerTypeRef = event.pointerType;
}"
@touchend.prevent.stop
@pointermove="handlePointerMove"
@pointerleave="handlePointerLeave"
@keydown="handleKeyDown"
Expand Down
44 changes: 19 additions & 25 deletions packages/radix-vue/src/Select/SelectTrigger.vue
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ const props = withDefaults(defineProps<SelectTriggerProps>(), {
})
const rootContext = injectSelectRootContext()

// eslint-disable-next-line prefer-const
let pointerTypeRef: PointerEvent['pointerType'] = 'touch'

const isDisabled = computed(() => rootContext.disabled?.value || props.disabled)

const { forwardRef, currentElement: triggerElement } = useForwardExpose()
Expand All @@ -35,19 +38,18 @@ const collectionItems = injectCollection()

const { search, handleTypeaheadSearch, resetTypeahead }
= useTypeahead(collectionItems)
function handleOpen() {
function handleOpen(pointerEvent?: MouseEvent | PointerEvent) {
if (!isDisabled.value) {
rootContext.onOpenChange(true)
// reset typeahead when we open
resetTypeahead()
}
}

function handlePointerOpen(event: PointerEvent) {
handleOpen()
rootContext.triggerPointerDownPosRef.value = {
x: Math.round(event.pageX),
y: Math.round(event.pageY),
if (pointerEvent) {
rootContext.triggerPointerDownPosRef.value = {
x: Math.round(pointerEvent.pageX),
y: Math.round(pointerEvent.pageY),
}
}
}
</script>
Expand All @@ -72,21 +74,22 @@ function handlePointerOpen(event: PointerEvent) {
:as-child="asChild"
:as="as"
@click="
(event: MouseEvent) => {
(event: PointerEvent) => {
// Whilst browsers generally have no issue focusing the trigger when clicking
// on a label, Safari seems to struggle with the fact that there's no `onClick`.
// We force `focus` in this case. Note: this doesn't create any other side-effect
// because we are preventing default in `onPointerDown` so effectively
// this only runs for a label 'click'
(event?.currentTarget as HTMLElement)?.focus();

if (pointerTypeRef !== 'mouse') {
handleOpen(event);
}
}
"
@pointerdown="
(event: PointerEvent) => {
// Prevent opening on touch down.
// https://github.com/unovue/radix-vue/issues/804
if (event.pointerType === 'touch')
return event.preventDefault();
pointerTypeRef = event.pointerType;

// prevent implicit pointer capture
// https://www.w3.org/TR/pointerevents3/#implicit-pointer-capture
Expand All @@ -97,28 +100,19 @@ function handlePointerOpen(event: PointerEvent) {

// only call handler if it's the left button (mousedown gets triggered by all mouse buttons)
// but not when the control key is pressed (avoiding MacOS right click)
if (event.button === 0 && event.ctrlKey === false) {
handlePointerOpen(event)
if (event.button === 0 && event.ctrlKey === false && event.pointerType === 'mouse') {
handleOpen(event)
// prevent trigger from stealing focus from the active item after opening.
event.preventDefault();
}
}
"
@pointerup.prevent="
(event: PointerEvent) => {
// Only open on pointer up when using touch devices
// https://github.com/unovue/radix-vue/issues/804
if (event.pointerType === 'touch')
handlePointerOpen(event)
}
"
@keydown="
(event) => {
const isTypingAhead = search !== '';
const isModifierKey = event.ctrlKey || event.altKey || event.metaKey;
if (!isModifierKey && event.key.length === 1)
if (isTypingAhead && event.key === ' ') return;
handleTypeaheadSearch(event.key);
if (!isModifierKey && event.key.length === 1) handleTypeaheadSearch(event.key);
if (isTypingAhead && event.key === ' ') return;
if (OPEN_KEYS.includes(event.key)) {
handleOpen();
event.preventDefault();
Expand Down
Loading