From 3d0c4aa61d83a422d9a361373dd572b63c6ca63d Mon Sep 17 00:00:00 2001 From: Matyas Forian-Szabo Date: Mon, 2 Jun 2025 16:44:50 +0200 Subject: [PATCH] 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 --- .../__new-tests__/Drilldown.test.tsx | 27 ++++++++++++++++ packages/ui-drilldown/src/Drilldown/index.tsx | 9 +++--- .../src/Select/__new-tests__/Select.test.tsx | 23 ++++++++++++++ packages/ui-select/src/Select/index.tsx | 31 +++++++++---------- packages/ui-tabs/src/Tabs/index.tsx | 10 +++--- .../SmallViewportLayout/index.tsx | 2 +- 6 files changed, 77 insertions(+), 25 deletions(-) diff --git a/packages/ui-drilldown/src/Drilldown/__new-tests__/Drilldown.test.tsx b/packages/ui-drilldown/src/Drilldown/__new-tests__/Drilldown.test.tsx index 8cec5e08d3..f4fc6537fb 100644 --- a/packages/ui-drilldown/src/Drilldown/__new-tests__/Drilldown.test.tsx +++ b/packages/ui-drilldown/src/Drilldown/__new-tests__/Drilldown.test.tsx @@ -104,6 +104,33 @@ describe('', () => { expect(options.length).toBe(0) expect(notAllowedChild).not.toBeInTheDocument() }) + + it('should not crash for weird option ids', async () => { + const onSelect = vi.fn() + const weirdID = 'some"_weird!@#$%^&*()\\|`id' + render( + + + {data.map((option) => ( + + {option.label} + + ))} + + + ) + const option_1 = screen.getByTestId(weirdID + 'opt_1') + await userEvent.click(option_1) + + await waitFor(() => { + expect(onSelect).toHaveBeenCalled() + }) + }) }) describe('id prop', () => { diff --git a/packages/ui-drilldown/src/Drilldown/index.tsx b/packages/ui-drilldown/src/Drilldown/index.tsx index 725ea55850..e358302ed6 100644 --- a/packages/ui-drilldown/src/Drilldown/index.tsx +++ b/packages/ui-drilldown/src/Drilldown/index.tsx @@ -585,9 +585,8 @@ class Drilldown extends Component { focusOption(id: string) { const container = this._containerElement - const optionElement = container?.querySelector( - `[id="${id}"]` + `[id="${CSS.escape(id)}"]` ) as HTMLSpanElement optionElement?.focus() @@ -816,7 +815,7 @@ class Drilldown extends Component { if (event.type === 'keydown' && href) { const optionEl = this._drilldownRef?.querySelector( - `#${id}` + `#${CSS.escape(id)}` ) as HTMLLinkElement const isLink = optionEl.tagName.toLowerCase() === 'a' @@ -1516,7 +1515,9 @@ class Drilldown extends Component { : this.currentPage.children[0]?.props.id if (!targetId) return null - return this._popover?._contentElement?.querySelector(`#${targetId}`) + return this._popover?._contentElement?.querySelector( + `#${CSS.escape(targetId)}` + ) }} elementRef={(element) => { // setting ref for "Popover" version, the popover root diff --git a/packages/ui-select/src/Select/__new-tests__/Select.test.tsx b/packages/ui-select/src/Select/__new-tests__/Select.test.tsx index 99bd7c69b5..d7df05ce80 100644 --- a/packages/ui-select/src/Select/__new-tests__/Select.test.tsx +++ b/packages/ui-select/src/Select/__new-tests__/Select.test.tsx @@ -316,6 +316,29 @@ describe(' + + op1 + + + op2 + + + ) + vi.advanceTimersToNextFrame() + vi.useRealTimers() + const input = container.querySelector('input') + expect(input).toBeInTheDocument() + }) + it('should have role button in Safari without onInputChange', async () => { const { container } = render( diff --git a/packages/ui-select/src/Select/index.tsx b/packages/ui-select/src/Select/index.tsx index 2c421c033c..77f6841b3a 100644 --- a/packages/ui-select/src/Select/index.tsx +++ b/packages/ui-select/src/Select/index.tsx @@ -291,22 +291,21 @@ class Select extends Component { } scrollToOption(id?: string) { - if (this._listView) { - const option = this._listView.querySelector(`[id="${id}"]`) - if (!option) return - - const listItem = option.parentNode - const parentTop = getBoundingClientRect(this._listView).top - const elemTop = getBoundingClientRect(listItem).top - const parentBottom = parentTop + this._listView.clientHeight - const elemBottom = - elemTop + (listItem ? (listItem as Element).clientHeight : 0) - - if (elemBottom > parentBottom) { - this._listView.scrollTop += elemBottom - parentBottom - } else if (elemTop < parentTop) { - this._listView.scrollTop -= parentTop - elemTop - } + if (!this._listView || !id) return + const option = this._listView.querySelector(`[id="${CSS.escape(id)}"]`) + if (!option) return + + const listItem = option.parentNode + const parentTop = getBoundingClientRect(this._listView).top + const elemTop = getBoundingClientRect(listItem).top + const parentBottom = parentTop + this._listView.clientHeight + const elemBottom = + elemTop + (listItem ? (listItem as Element).clientHeight : 0) + + if (elemBottom > parentBottom) { + this._listView.scrollTop += elemBottom - parentBottom + } else if (elemTop < parentTop) { + this._listView.scrollTop -= parentTop - elemTop } } diff --git a/packages/ui-tabs/src/Tabs/index.tsx b/packages/ui-tabs/src/Tabs/index.tsx index a0f7479794..f4537df159 100644 --- a/packages/ui-tabs/src/Tabs/index.tsx +++ b/packages/ui-tabs/src/Tabs/index.tsx @@ -331,10 +331,12 @@ class Tabs extends Component { // this is needed because keypress cancels scrolling. So we have to trigger the scrolling // one "tick" later than the keypress setTimeout(() => { - this.state.withTabListOverflow && - this.showActiveTabIfOverlayed( - this._tabList!.querySelector(`#tab-${id}`) - ) + if (this.state.withTabListOverflow) { + const tab = id + ? this._tabList!.querySelector(`#tab-${CSS.escape(id)}`) + : null + this.showActiveTabIfOverlayed(tab) + } }, 0) } diff --git a/packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx b/packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx index 3bc5a23081..75e61427f1 100644 --- a/packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx +++ b/packages/ui-top-nav-bar/src/TopNavBar/TopNavBarLayout/SmallViewportLayout/index.tsx @@ -310,7 +310,7 @@ class TopNavBarSmallViewportLayout extends Component< setTimeout(() => { const container = document.getElementById(this._trayContainerId) const firstOption = container?.querySelector( - `[id="${targetId}"]` + `[id="${CSS.escape(targetId)}"]` ) as HTMLSpanElement firstOption?.focus() if (this._drilldownRef) {