-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Vitest: add WebKit browser instance #3866
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
Changes from all commits
936eceb
3a06ef8
86fde9b
0b07074
ef3f86f
5c4e3ad
6ab1bbd
215d4c7
398fe13
313bde6
c6decae
a84c72e
ea06744
a2085b6
3bfbfac
a00f60e
cecf08b
faafac2
a50bd1d
6a9e3ab
0a38ac8
2f534b5
256839e
2334d07
9880702
204591a
e2c604e
86dabf0
0ff8120
c643d53
e30e55d
196c57e
611bcdd
f78134b
39ea74a
08f0c76
6f8fd64
d420c44
b1de604
9a341b5
f1f752b
48fa0bb
6af8584
dad063b
41e0d21
fbdb6aa
9f9f9b4
732e89e
fdf411c
3aaf277
dff828b
e740506
814988a
4308ef9
b3b47fb
70f2867
8983007
d979f91
4dd636f
86c9dcb
6a260b5
63b8c8d
6d7d448
c3ddeb2
70e4b6d
a1c0c72
d13c1ae
e76cc99
441f5df
1eec276
f754599
dd44e96
f3a1cd6
a5df38a
1f3fd89
339ba17
566c5a2
c3c7bf6
aa36819
f7a9ff8
42f4ea3
d7e4606
60bd119
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -620,7 +620,8 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |
|
|
||
| if (!(target instanceof Element)) return; | ||
|
|
||
| const isCellEvent = target.closest('.rdg-cell') !== null; | ||
| const cell = target.closest('.rdg-cell'); | ||
| const isCellEvent = cell !== null; | ||
| const isRowEvent = isTreeGrid && target.role === 'row'; | ||
|
|
||
| if (!isCellEvent && !isRowEvent) return; | ||
|
|
@@ -638,7 +639,7 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |
| navigate(event); | ||
| break; | ||
| default: | ||
| handleCellInput(event); | ||
| handleCellInput(event, cell); | ||
| break; | ||
| } | ||
| } | ||
|
|
@@ -678,7 +679,7 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |
| updateRow(column, activePosition.rowIdx, updatedRow); | ||
| } | ||
|
|
||
| function handleCellInput(event: KeyboardEvent<HTMLDivElement>) { | ||
| function handleCellInput(event: KeyboardEvent<HTMLDivElement>, cell: Element | null) { | ||
| if (!activePositionIsCellInViewport) return; | ||
| const row = getActiveRow(); | ||
| const { key, shiftKey } = event; | ||
|
|
@@ -694,6 +695,9 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr | |
| } | ||
|
|
||
| if (isCellEditable(activePosition) && isDefaultCellInput(event, onCellPaste != null)) { | ||
| // ensure cell is fully visible | ||
| scrollIntoView(cell); | ||
|
Comment on lines
+698
to
+699
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a bug in safari?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and no, seems to be a different behavior in webkit when typing on a focused element I think it's like this:
Just my interpretation
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Does not hurt to keep it |
||
|
|
||
| setActivePosition(({ idx, rowIdx }) => ({ | ||
| idx, | ||
| rowIdx, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,7 +176,12 @@ test('Cell should not steal focus when the focus is outside the grid and cell is | |
|
|
||
| return ( | ||
| <> | ||
| <button type="button" onClick={onClick}> | ||
| <button | ||
| type="button" | ||
| // tabIndex needed otherwise webkit will set focus on the <body> after clicking the button | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😕 |
||
| tabIndex={0} | ||
| onClick={onClick} | ||
| > | ||
| Test | ||
| </button> | ||
| <DataGrid | ||
|
|
@@ -198,6 +203,7 @@ test('Cell should not steal focus when the focus is outside the grid and cell is | |
| const button = page.getByRole('button', { name: 'Test' }); | ||
| await expect.element(button).not.toHaveFocus(); | ||
| await userEvent.click(button); | ||
| await expect.element(button).toHaveFocus(); | ||
| await expect.element(cell).not.toHaveFocus(); | ||
| await expect.element(button).toHaveFocus(); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| import { page, userEvent, type Locator } from 'vitest/browser'; | ||
| import { page, server, userEvent, type Locator } from 'vitest/browser'; | ||
|
|
||
| import { DataGrid } from '../../src'; | ||
| import type { DataGridProps } from '../../src'; | ||
|
|
||
| // copy/paste do not work in webkit in CI | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth testing in webdriver instead of playwright?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not, we'll catch any regressions locally, and I'd rather not set up yet another provider. |
||
| export const canCopyPaste = !(import.meta.env.CI && server.browser === 'webkit'); | ||
|
|
||
| export function setup<R, SR, K extends React.Key = React.Key>(props: DataGridProps<R, SR, K>) { | ||
| return page.render(<DataGrid {...props} />); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,12 +31,21 @@ const resizeColumn: BrowserCommand<[name: string, resizeBy: number | readonly nu | |
| }; | ||
|
|
||
| // TODO: remove when `userEvent.pointer` is supported | ||
| const dragFill: BrowserCommand<[from: string, to: string]> = async ({ page, iframe }, from, to) => { | ||
| const dragFill: BrowserCommand<[from: string, to: string]> = async ( | ||
| { page, iframe, project }, | ||
| from, | ||
| to | ||
| ) => { | ||
| await iframe.getByRole('gridcell', { name: from, exact: true }).click(); | ||
| await iframe.locator('.rdg-cell-drag-handle').hover(); | ||
| await page.mouse.down(); | ||
| const toCell = iframe.getByRole('gridcell', { name: to, exact: true }); | ||
| await toCell.hover(); | ||
| await iframe.getByRole('gridcell', { name: to, exact: true }).hover(); | ||
| if (project.name.includes('webkit')) { | ||
| // let React re-render after handleDragHandlePointerMove calls setDraggedOverRowIdx() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alternative would be to use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How did you even find this bug 😄
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with tests failing locally 🙃 |
||
| await new Promise((resolve) => { | ||
| setTimeout(resolve, 20); | ||
| }); | ||
| } | ||
| await page.mouse.up(); | ||
| }; | ||
|
|
||
|
|
@@ -169,6 +178,10 @@ export default defineConfig(({ isPreview }): ViteUserConfig => ({ | |
| }), | ||
| // TODO: remove when FF tests are stable | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder if this is needed anymore. We can check in a separate PR
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still needed when I last tried |
||
| fileParallelism: false | ||
| }, | ||
| { | ||
| browser: 'webkit', | ||
| provider: playwright(playwrightOptions) | ||
| } | ||
| ] | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails without these commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apt-get times out without these for some reason