Skip to content

Commit

Permalink
Improve control over Menu and Listbox options while searching (#2471
Browse files Browse the repository at this point in the history
)

* add `get-text-value` helper

* use `getTextValue` in `Listbox` component

* use `getTextValue` in `Menu` component

* update changelog

* ensure we handle multiple values for `aria-labelledby`

* hoist regex

* drop child nodes instead of replacing its innerText

This makes it a bit slower but also more correct. We can use a cache on
another level to ensure that we are not creating useless work.

* add `useTextValue` to improve performance of `getTextValue`

This will add a cache and only if the `innerText` changes, only then
will we calculate the new text value.

* use better `useTextValue` hook
  • Loading branch information
RobinMalfait authored May 4, 2023
1 parent 0505e92 commit 67f3c4d
Show file tree
Hide file tree
Showing 14 changed files with 447 additions and 16 deletions.
6 changes: 5 additions & 1 deletion jest/create-jest-config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ module.exports = function createJestConfig(root, options) {
return Object.assign(
{
rootDir: root,
setupFilesAfterEnv: ['<rootDir>../../jest/custom-matchers.ts', ...setupFilesAfterEnv],
setupFilesAfterEnv: [
'<rootDir>../../jest/custom-matchers.ts',
'<rootDir>../../jest/polyfills.ts',
...setupFilesAfterEnv,
],
transform: {
'^.+\\.(t|j)sx?$': '@swc/jest',
...transform,
Expand Down
15 changes: 15 additions & 0 deletions jest/polyfills.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// JSDOM Doesn't implement innerText yet: https://github.com/jsdom/jsdom/issues/1245
// So this is a hacky way of implementing it using `textContent`.
// Real implementation doesn't use textContent because:
// > textContent gets the content of all elements, including <script> and <style> elements. In
// > contrast, innerText only shows "human-readable" elements.
// >
// > — https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent#differences_from_innertext
Object.defineProperty(HTMLElement.prototype, 'innerText', {
get() {
return this.textContent
},
set(value) {
this.textContent = value
},
})
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Ensure `FocusTrap` is only active when the given `enabled` value is `true` ([#2456](https://github.com/tailwindlabs/headlessui/pull/2456))
- Stop `<Transition appear>` from overwriting classes on re-render ([#2457](https://github.com/tailwindlabs/headlessui/pull/2457))
- Improve control over `Menu` and `Listbox` options while searching ([#2471](https://github.com/tailwindlabs/headlessui/pull/2471))

### Changed

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import { useEvent } from '../../hooks/use-event'
import { useControllable } from '../../hooks/use-controllable'
import { useLatestValue } from '../../hooks/use-latest-value'
import { useTrackedPointer } from '../../hooks/use-tracked-pointer'
import { useTextValue } from '../../hooks/use-text-value'

enum ListboxStates {
Open,
Expand Down Expand Up @@ -934,12 +935,13 @@ function OptionFn<

let selected = data.isSelected(value)
let internalOptionRef = useRef<HTMLLIElement | null>(null)
let getTextValue = useTextValue(internalOptionRef)
let bag = useLatestValue<ListboxOptionDataRef<TType>['current']>({
disabled,
value,
domRef: internalOptionRef,
get textValue() {
return internalOptionRef.current?.textContent?.toLowerCase()
return getTextValue()
},
})
let optionRef = useSyncRefs(ref, internalOptionRef)
Expand Down
14 changes: 10 additions & 4 deletions packages/@headlessui-react/src/components/menu/menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
import { useOwnerDocument } from '../../hooks/use-owner'
import { useEvent } from '../../hooks/use-event'
import { useTrackedPointer } from '../../hooks/use-tracked-pointer'
import { useTextValue } from '../../hooks/use-text-value'

enum MenuStates {
Open,
Expand Down Expand Up @@ -636,14 +637,19 @@ function ItemFn<TTag extends ElementType = typeof DEFAULT_ITEM_TAG>(
/* We also want to trigger this when the position of the active item changes so that we can re-trigger the scrollIntoView */ state.activeItemIndex,
])

let bag = useRef<MenuItemDataRef['current']>({ disabled, domRef: internalItemRef })
let getTextValue = useTextValue(internalItemRef)

let bag = useRef<MenuItemDataRef['current']>({
disabled,
domRef: internalItemRef,
get textValue() {
return getTextValue()
},
})

useIsoMorphicEffect(() => {
bag.current.disabled = disabled
}, [bag, disabled])
useIsoMorphicEffect(() => {
bag.current.textValue = internalItemRef.current?.textContent?.toLowerCase()
}, [bag, internalItemRef])

useIsoMorphicEffect(() => {
dispatch({ type: ActionTypes.RegisterItem, id, dataRef: bag })
Expand Down
25 changes: 25 additions & 0 deletions packages/@headlessui-react/src/hooks/use-text-value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { useRef, MutableRefObject } from 'react'
import { getTextValue } from '../utils/get-text-value'
import { useEvent } from './use-event'

export function useTextValue(element: MutableRefObject<HTMLElement | null>) {
let cacheKey = useRef<string>('')
let cacheValue = useRef<string>('')

return useEvent(() => {
let el = element.current
if (!el) return ''

// Check for a cached version
let currentKey = el.innerText
if (cacheKey.current === currentKey) {
return cacheValue.current
}

// Calculate the value
let value = getTextValue(el).trim().toLowerCase()
cacheKey.current = currentKey
cacheValue.current = value
return value
})
}
95 changes: 95 additions & 0 deletions packages/@headlessui-react/src/utils/get-text-value.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import { getTextValue } from './get-text-value'

let html = String.raw

it('should be possible to get the text value from an element', () => {
let element = document.createElement('div')
element.innerText = 'Hello World'
expect(getTextValue(element)).toEqual('Hello World')
})

it('should strip out emojis when receiving the text from the element', () => {
let element = document.createElement('div')
element.innerText = '🇨🇦 Canada'
expect(getTextValue(element)).toEqual('Canada')
})

it('should strip out hidden elements', () => {
let element = document.createElement('div')
element.innerHTML = html`<div><span hidden>Hello</span> world</div>`
expect(getTextValue(element)).toEqual('world')
})

it('should strip out aria-hidden elements', () => {
let element = document.createElement('div')
element.innerHTML = html`<div><span aria-hidden>Hello</span> world</div>`
expect(getTextValue(element)).toEqual('world')
})

it('should strip out role="img" elements', () => {
let element = document.createElement('div')
element.innerHTML = html`<div><span role="img">°</span> world</div>`
expect(getTextValue(element)).toEqual('world')
})

it('should be possible to get the text value from the aria-label', () => {
let element = document.createElement('div')
element.setAttribute('aria-label', 'Hello World')
expect(getTextValue(element)).toEqual('Hello World')
})

it('should be possible to get the text value from the aria-label (even if there is content)', () => {
let element = document.createElement('div')
element.setAttribute('aria-label', 'Hello World')
element.innerHTML = 'Hello Universe'
element.innerText = 'Hello Universe'
expect(getTextValue(element)).toEqual('Hello World')
})

it('should be possible to get the text value from the element referenced by aria-labelledby (using `aria-label`)', () => {
document.body.innerHTML = html`
<div>
<div id="foo" aria-labelledby="bar">Contents of foo</div>
<div id="bar" aria-label="Actual value of bar">Contents of bar</div>
</div>
`

expect(getTextValue(document.querySelector('#foo')!)).toEqual('Actual value of bar')
})

it('should be possible to get the text value from the element referenced by aria-labelledby (using its contents)', () => {
document.body.innerHTML = html`
<div>
<div id="foo" aria-labelledby="bar">Contents of foo</div>
<div id="bar">Contents of bar</div>
</div>
`

expect(getTextValue(document.querySelector('#foo')!)).toEqual('Contents of bar')
})

it('should be possible to get the text value from the element referenced by aria-labelledby (using `aria-label`, multiple)', () => {
document.body.innerHTML = html`
<div>
<div id="foo" aria-labelledby="bar baz">Contents of foo</div>
<div id="bar" aria-label="Actual value of bar">Contents of bar</div>
<div id="baz" aria-label="Actual value of baz">Contents of baz</div>
</div>
`

expect(getTextValue(document.querySelector('#foo')!)).toEqual(
'Actual value of bar, Actual value of baz'
)
})

it('should be possible to get the text value from the element referenced by aria-labelledby (using its contents, multiple)', () => {
document.body.innerHTML = html`
<div>
<div id="foo" aria-labelledby="bar baz">Contents of foo</div>
<div id="bar">Contents of bar</div>
<div id="baz">Contents of baz</div>
</div>
`

expect(getTextValue(document.querySelector('#foo')!)).toEqual('Contents of bar, Contents of baz')
})
81 changes: 81 additions & 0 deletions packages/@headlessui-react/src/utils/get-text-value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
let emojiRegex =
/([\u2700-\u27BF]|[\uE000-\uF8FF]|\uD83C[\uDC00-\uDFFF]|\uD83D[\uDC00-\uDFFF]|[\u2011-\u26FF]|\uD83E[\uDD10-\uDDFF])/g

function getTextContents(element: HTMLElement): string {
// Using innerText instead of textContent because:
//
// > textContent gets the content of all elements, including <script> and <style> elements. In
// > contrast, innerText only shows "human-readable" elements.
// >
// > — https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent#differences_from_innertext
let currentInnerText = element.innerText ?? ''

// Remove all the elements that shouldn't be there.
//
// [hidden] — The user doesn't see it
// [aria-hidden] — The screen reader doesn't see it
// [role="img"] — Even if it is text, it is used as an image
//
// This is probably the slowest part, but if you want complete control over the text value, then
// it is better to set an `aria-label` instead.
let copy = element.cloneNode(true)
if (!(copy instanceof HTMLElement)) {
return currentInnerText
}

let dropped = false
// Drop the elements that shouldn't be there.
for (let child of copy.querySelectorAll('[hidden],[aria-hidden],[role="img"]')) {
child.remove()
dropped = true
}

// Now that the elements are removed, we can get the innerText such that we can strip the emojis.
let value = dropped ? copy.innerText ?? '' : currentInnerText

// Check if it contains some emojis or not, if so, we need to remove them
// because ideally we work with simple text values.
//
// Ideally we can use the much simpler RegEx: /\p{Extended_Pictographic}/u
// but we can't rely on this yet, so we use the more complex one.
if (emojiRegex.test(value)) {
value = value.replace(emojiRegex, '')
}

return value
}

export function getTextValue(element: HTMLElement): string {
// Try to use the `aria-label` first
let label = element.getAttribute('aria-label')
if (typeof label === 'string') return label.trim()

// Try to use the `aria-labelledby` second
let labelledby = element.getAttribute('aria-labelledby')
if (labelledby) {
// aria-labelledby can be a space-separated list of IDs, so we need to split them up and
// combine them into a single string.
let labels = labelledby
.split(' ')
.map((labelledby) => {
let labelEl = document.getElementById(labelledby)
if (labelEl) {
let label = labelEl.getAttribute('aria-label')
// Try to use the `aria-label` first (of the referenced element)
if (typeof label === 'string') return label.trim()

// This time, the `aria-labelledby` isn't used anymore (in Safari), so we just have to
// look at the contents itself.
return getTextContents(labelEl).trim()
}

return null
})
.filter(Boolean)

if (labels.length > 0) return labels.join(', ')
}

// Try to use the text contents of the element itself
return getTextContents(element).trim()
}
1 change: 1 addition & 0 deletions packages/@headlessui-vue/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fix memory leak in `Popover` component ([#2430](https://github.com/tailwindlabs/headlessui/pull/2430))
- Ensure `FocusTrap` is only active when the given `enabled` value is `true` ([#2456](https://github.com/tailwindlabs/headlessui/pull/2456))
- Ensure the exposed `activeIndex` is up to date for the `Combobox` component ([#2463](https://github.com/tailwindlabs/headlessui/pull/2463))
- Improve control over `Menu` and `Listbox` options while searching ([#2471](https://github.com/tailwindlabs/headlessui/pull/2471))

### Changed

Expand Down
10 changes: 5 additions & 5 deletions packages/@headlessui-vue/src/components/listbox/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { Hidden, Features as HiddenFeatures } from '../../internal/hidden'
import { objectToFormEntries } from '../../utils/form'
import { useControllable } from '../../hooks/use-controllable'
import { useTrackedPointer } from '../../hooks/use-tracked-pointer'
import { useTextValue } from '../../hooks/use-text-value'

function defaultComparator<T>(a: T, z: T): boolean {
return a === z
Expand Down Expand Up @@ -731,16 +732,15 @@ export let ListboxOption = defineComponent({
})
})

let getTextValue = useTextValue(internalOptionRef)
let dataRef = computed<ListboxOptionData>(() => ({
disabled: props.disabled,
value: props.value,
textValue: '',
get textValue() {
return getTextValue()
},
domRef: internalOptionRef,
}))
onMounted(() => {
let textValue = dom(internalOptionRef)?.textContent?.toLowerCase().trim()
if (textValue !== undefined) dataRef.value.textValue = textValue
})

onMounted(() => api.registerOption(props.id, dataRef))
onUnmounted(() => api.unregisterOption(props.id))
Expand Down
10 changes: 5 additions & 5 deletions packages/@headlessui-vue/src/components/menu/menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
} from '../../utils/focus-management'
import { useOutsideClick } from '../../hooks/use-outside-click'
import { useTrackedPointer } from '../../hooks/use-tracked-pointer'
import { useTextValue } from '../../hooks/use-text-value'

enum MenuStates {
Open,
Expand Down Expand Up @@ -511,15 +512,14 @@ export let MenuItem = defineComponent({
: false
})

let getTextValue = useTextValue(internalItemRef)
let dataRef = computed<MenuItemData>(() => ({
disabled: props.disabled,
textValue: '',
get textValue() {
return getTextValue()
},
domRef: internalItemRef,
}))
onMounted(() => {
let textValue = dom(internalItemRef)?.textContent?.toLowerCase().trim()
if (textValue !== undefined) dataRef.value.textValue = textValue
})

onMounted(() => api.registerItem(props.id, dataRef))
onUnmounted(() => api.unregisterItem(props.id))
Expand Down
25 changes: 25 additions & 0 deletions packages/@headlessui-vue/src/hooks/use-text-value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { ref, Ref } from 'vue'
import { getTextValue } from '../utils/get-text-value'
import { dom } from '../utils/dom'

export function useTextValue(element: Ref<HTMLElement | null>) {
let cacheKey = ref<string>('')
let cacheValue = ref<string>('')

return () => {
let el = dom(element)
if (!el) return ''

// Check for a cached version
let currentKey = el.innerText
if (cacheKey.value === currentKey) {
return cacheValue.value
}

// Calculate the value
let value = getTextValue(el).trim().toLowerCase()
cacheKey.value = currentKey
cacheValue.value = value
return value
}
}
Loading

2 comments on commit 67f3c4d

@vercel
Copy link

@vercel vercel bot commented on 67f3c4d May 4, 2023

Choose a reason for hiding this comment

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

Successfully deployed to the following URLs:

headlessui-vue – ./packages/playground-vue

headlessui-vue-git-main-tailwindlabs.vercel.app
headlessui-vue-tailwindlabs.vercel.app
headlessui-vue.vercel.app

@vercel
Copy link

@vercel vercel bot commented on 67f3c4d May 4, 2023

Choose a reason for hiding this comment

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

Successfully deployed to the following URLs:

headlessui-react – ./packages/playground-react

headlessui-react-tailwindlabs.vercel.app
headlessui-react.vercel.app
headlessui-react-git-main-tailwindlabs.vercel.app

Please sign in to comment.