Skip to content

Commit 78e0b96

Browse files
committed
fix(many): fix "not a valid selector" exception when an option ID contains quotes
The issue is that we are letting users choose any string as an ID, and the querySelector method only accepts CSS safe IDs. Luckily there is a built in method to escape stuff that it does not like Fixes INSTUI-4570
1 parent 915fab3 commit 78e0b96

File tree

6 files changed

+77
-25
lines changed

6 files changed

+77
-25
lines changed

packages/ui-drilldown/src/Drilldown/__new-tests__/Drilldown.test.tsx

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,33 @@ describe('<Drilldown />', () => {
104104
expect(options.length).toBe(0)
105105
expect(notAllowedChild).not.toBeInTheDocument()
106106
})
107+
108+
it('should not crash for weird option ids', async () => {
109+
const onSelect = vi.fn()
110+
const weirdID = 'some"_weird!@#$%^&*()\\|`id'
111+
render(
112+
<Drilldown rootPageId="page0" onSelect={onSelect}>
113+
<Drilldown.Page id="page0">
114+
{data.map((option) => (
115+
<Drilldown.Option
116+
id={weirdID + option.id}
117+
value={weirdID + option.id}
118+
key={weirdID + option.id}
119+
data-testid={weirdID + option.id}
120+
>
121+
{option.label}
122+
</Drilldown.Option>
123+
))}
124+
</Drilldown.Page>
125+
</Drilldown>
126+
)
127+
const option_1 = screen.getByTestId(weirdID + 'opt_1')
128+
await userEvent.click(option_1)
129+
130+
await waitFor(() => {
131+
expect(onSelect).toHaveBeenCalled()
132+
})
133+
})
107134
})
108135

109136
describe('id prop', () => {

packages/ui-drilldown/src/Drilldown/index.tsx

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -585,9 +585,8 @@ class Drilldown extends Component<DrilldownProps, DrilldownState> {
585585

586586
focusOption(id: string) {
587587
const container = this._containerElement
588-
589588
const optionElement = container?.querySelector(
590-
`[id="${id}"]`
589+
`[id="${CSS.escape(id)}"]`
591590
) as HTMLSpanElement
592591

593592
optionElement?.focus()
@@ -816,7 +815,7 @@ class Drilldown extends Component<DrilldownProps, DrilldownState> {
816815

817816
if (event.type === 'keydown' && href) {
818817
const optionEl = this._drilldownRef?.querySelector(
819-
`#${id}`
818+
`#${CSS.escape(id)}`
820819
) as HTMLLinkElement
821820
const isLink = optionEl.tagName.toLowerCase() === 'a'
822821

@@ -1516,7 +1515,9 @@ class Drilldown extends Component<DrilldownProps, DrilldownState> {
15161515
: this.currentPage.children[0]?.props.id
15171516

15181517
if (!targetId) return null
1519-
return this._popover?._contentElement?.querySelector(`#${targetId}`)
1518+
return this._popover?._contentElement?.querySelector(
1519+
`#${CSS.escape(targetId)}`
1520+
)
15201521
}}
15211522
elementRef={(element) => {
15221523
// setting ref for "Popover" version, the popover root

packages/ui-select/src/Select/__new-tests__/Select.test.tsx

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,29 @@ describe('<Select />', () => {
316316
consoleErrorMock.mockRestore()
317317
})
318318

319+
it('should not crash for weird option ids', async () => {
320+
const weirdID = 'some_`w@ei:r|!@#$%^&*(()|\\.l/d"id'
321+
vi.useFakeTimers()
322+
const { container } = render(
323+
<Select
324+
renderLabel="Choose an option"
325+
scrollToHighlightedOption
326+
isShowingOptions
327+
>
328+
<Select.Option id={weirdID} key="1" value="2" isHighlighted={true}>
329+
op1
330+
</Select.Option>
331+
<Select.Option id="sdfsdfsd" key="2" value="2">
332+
op2
333+
</Select.Option>
334+
</Select>
335+
)
336+
vi.advanceTimersToNextFrame()
337+
vi.useRealTimers()
338+
const input = container.querySelector('input')
339+
expect(input).toBeInTheDocument()
340+
})
341+
319342
it('should have role button in Safari without onInputChange', async () => {
320343
const { container } = render(
321344
<Select renderLabel="Choose an option">{getOptions()}</Select>

packages/ui-select/src/Select/index.tsx

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -291,22 +291,21 @@ class Select extends Component<SelectProps> {
291291
}
292292

293293
scrollToOption(id?: string) {
294-
if (this._listView) {
295-
const option = this._listView.querySelector(`[id="${id}"]`)
296-
if (!option) return
297-
298-
const listItem = option.parentNode
299-
const parentTop = getBoundingClientRect(this._listView).top
300-
const elemTop = getBoundingClientRect(listItem).top
301-
const parentBottom = parentTop + this._listView.clientHeight
302-
const elemBottom =
303-
elemTop + (listItem ? (listItem as Element).clientHeight : 0)
304-
305-
if (elemBottom > parentBottom) {
306-
this._listView.scrollTop += elemBottom - parentBottom
307-
} else if (elemTop < parentTop) {
308-
this._listView.scrollTop -= parentTop - elemTop
309-
}
294+
if (!this._listView || !id) return
295+
const option = this._listView.querySelector(`[id="${CSS.escape(id)}"]`)
296+
if (!option) return
297+
298+
const listItem = option.parentNode
299+
const parentTop = getBoundingClientRect(this._listView).top
300+
const elemTop = getBoundingClientRect(listItem).top
301+
const parentBottom = parentTop + this._listView.clientHeight
302+
const elemBottom =
303+
elemTop + (listItem ? (listItem as Element).clientHeight : 0)
304+
305+
if (elemBottom > parentBottom) {
306+
this._listView.scrollTop += elemBottom - parentBottom
307+
} else if (elemTop < parentTop) {
308+
this._listView.scrollTop -= parentTop - elemTop
310309
}
311310
}
312311

packages/ui-tabs/src/Tabs/index.tsx

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,10 +331,12 @@ class Tabs extends Component<TabsProps, TabsState> {
331331
// this is needed because keypress cancels scrolling. So we have to trigger the scrolling
332332
// one "tick" later than the keypress
333333
setTimeout(() => {
334-
this.state.withTabListOverflow &&
335-
this.showActiveTabIfOverlayed(
336-
this._tabList!.querySelector(`#tab-${id}`)
337-
)
334+
if (this.state.withTabListOverflow) {
335+
const tab = id
336+
? this._tabList!.querySelector(`#tab-${CSS.escape(id)}`)
337+
: null
338+
this.showActiveTabIfOverlayed(tab)
339+
}
338340
}, 0)
339341
}
340342

packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ class TopNavBarSmallViewportLayout extends Component<
310310
setTimeout(() => {
311311
const container = document.getElementById(this._trayContainerId)
312312
const firstOption = container?.querySelector(
313-
`[id="${targetId}"]`
313+
`[id="${CSS.escape(targetId)}"]`
314314
) as HTMLSpanElement
315315
firstOption?.focus()
316316
if (this._drilldownRef) {

0 commit comments

Comments
 (0)