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

Regression with setting focus in test environment #7740

Open
2 of 3 tasks
nwhittaker opened this issue Sep 13, 2023 · 3 comments
Open
2 of 3 tasks

Regression with setting focus in test environment #7740

nwhittaker opened this issue Sep 13, 2023 · 3 comments
Assignees
Labels
1 - assigned Issues that are assigned to a sprint and a team member. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. has workaround Issues have a workaround available in the meantime. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.

Comments

@nwhittaker
Copy link
Contributor

nwhittaker commented Sep 13, 2023

Check existing issues

Actual Behavior

In an automated test, if multiple components are given focus in quick succession, there's a race condition as to which one will actually retain focus.

Expected Behavior

In an automated test, if multiple components are given focus in quick succession, the last component to gain focus retains it.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/eYbRGqP

Reproduction Steps

  1. Visit code sample
  2. Click the Set focus button and see the first input retains focus despite the 2nd input being focused after.

Reproduction Version

1.8.0

Relevant Info

The code sample aims to mimic how an automated test interacts with components. Specifically via dispatched events instead of calling and awaiting setFocus() directly.

The issue appears to stem from changes in #7255 and #7277.

Regression?

1.4.3

Priority impact

p2 - want for current milestone

Impact

This issue represents a significant amount of effort for the Field Maps team to debug and rewrite tests in order to upgrade beyond Calcite 1.4.3.

Calcite package

  • @esri/calcite-components
  • @esri/calcite-components-react

Esri team

ArcGIS Field Apps

@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Sep 13, 2023
@github-actions github-actions bot added ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone calcite-components Issues specific to the @esri/calcite-components package. labels Sep 13, 2023
@jcfranco
Copy link
Member

We'll have to do some exploratory work here first, but we might be able to help here by avoiding calls to setFocus internally when the host is clicked in some cases. As you probably saw in the issues you referenced, our component's rely on focus being async to make sure the component is rendered before we can do any reliable internal focusing.

For your use case, is it mainly calcite-input wreaking havoc in tests or are there other components that are causing issues? Would it also be possible to see one of your example tests? Also, what's a reasonable timeline for a fix? We can do this via chat or a call too if it makes it easier. 😀

@geospatialem geospatialem added the need more info Issues that are missing information and/or a clear, actionable description. label Sep 18, 2023
@nwhittaker
Copy link
Contributor Author

…but we might be able to help here by avoiding calls to setFocus internally when the host is clicked in some cases.

At minimum it'd be nice to bail early if the element already has focus (if possible). Otherwise we don't really have any way to determine when setFocus finishes in that case since we can't wait for an update to document.activeElement.


For your use case, is it mainly calcite-input wreaking havoc in tests or are there other components that are causing issues?

calcite-input was convenient for the test case, but I primarily had problems testing keyboard navigation amongst a collection of calcite-radio-button elements (within a custom group element). I suspect it's a potential issue for any component with a setFocus implementation, though, due to its waiting for the next animation frame in componentFocusable.


Would it also be possible to see one of your example tests?

Most of the test failures revolve around testing keyboard navigation since the browser's logic for determining the next element is dependent on the current activeElement.

In this example, the keyboard events were sometimes dispatched to the same element instead of the next one, if I recall correctly. The waitForFocus calls were added to work around the regression.


expect(radioA.checked).toBeTruthy()
expect(radioA.tabIndex).toBe(0)

userEvent.keyboard('[ArrowDown]')
await waitForFocus(radioB)
expect(radioA.checked).toBeFalsy()
expect(radioA.tabIndex).toBe(-1)
expect(radioB.checked).toBeTruthy()
expect(radioB.tabIndex).toBe(0)

userEvent.keyboard('[ArrowDown]')
await waitForFocus(radioC)


async function waitForFocus(element: Element) {
  return waitFor(() => {
    if (document.activeElement === element) return
    throw new Error()
  })
}

Also, what's a reasonable timeline for a fix?

We have some workarounds in place now, so we're no longer blocked -- but I could see the issue being a gotcha with new features going forward. Happy to discuss more on Teams, if needed -- but I imagine a phased approach could be worth considering:

  1. Higher priority issue to bail setFocus early in cases where focus is delegated(?) or the component already has focus.
  2. Lower priority issue to explore eliminating animation frame waiting -- or clarify why/when it's necessary and possibly limiting it to those specific cases.

@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library has workaround Issues have a workaround available in the meantime. spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. and removed needs triage Planning workflow - pending design/dev review. need more info Issues that are missing information and/or a clear, actionable description. labels Nov 1, 2023
@geospatialem
Copy link
Member

A spike effort will be conducted early in 2024 to help determine a timing estimate, with the goal to include prior to the February schedule if in alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. has workaround Issues have a workaround available in the meantime. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.
Projects
None yet
Development

No branches or pull requests

4 participants