fix(forms): allow hash-based field focus#108376
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
… behavior" This reverts commit 20c06c2.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return; | ||
| } | ||
| if (hash === node.dataset.field) { | ||
| node.scrollIntoView({block: 'center', behavior: 'smooth'}); |
There was a problem hiding this comment.
Bug: The scrollIntoView call inside the ref callback executes before browser layout is complete, which can lead to scrolling to an incorrect position when navigating to a field.
Severity: MEDIUM
Suggested Fix
Defer the scrollIntoView call to ensure it executes after the browser has completed layout calculations. This can be achieved by wrapping the call in requestAnimationFrame, as was done in the previous implementation, or by using a setTimeout with a small delay.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/core/form/field/meta.tsx#L42
Potential issue: The `scrollToFieldRef` function calls `node.scrollIntoView` immediately
within a ref callback. Ref callbacks in React execute during the synchronous commit
phase, which occurs before the browser has performed layout calculations. Calling
`scrollIntoView` on an element whose final position has not yet been determined by the
browser can cause the page to scroll to an incorrect or inaccurate position. This is a
regression, as the previous implementation used `requestAnimationFrame` to defer the
scroll until after layout was complete. A similar pattern in `FoldSection.tsx` uses
`setTimeout` to explicitly delay scrolling for the same reason.
| let hash: string; | ||
| try { | ||
| hash = decodeURIComponent(window.location.hash.slice(1)); | ||
| } catch { | ||
| return; | ||
| } | ||
| if (hash === node.dataset.field) { | ||
| node.scrollIntoView({block: 'center', behavior: 'smooth'}); | ||
| node.control?.focus({focusVisible: true}); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug: A race condition in the scrollToFieldRef callback prevents field focusing. The label's ref executes before the associated input control is mounted, causing node.control to be null.
Severity: MEDIUM
Suggested Fix
Wrap the focus logic within requestAnimationFrame or setTimeout(..., 0). This will defer the execution until after the browser has painted and the input element has been mounted in the DOM, ensuring node.control is available. For example: requestAnimationFrame(() => node.control?.focus());.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/core/form/field/meta.tsx#L31-L45
Potential issue: A race condition exists in the `scrollToFieldRef` callback. The
callback is attached to a `Label` component and executes immediately when the label
mounts. However, due to React's rendering order, the associated input element has not
yet mounted in the DOM. This results in `node.control` being `null` at the time of
execution. Consequently, the `node.control?.focus()` call fails silently, and the input
field does not receive focus as intended, even though the page correctly scrolls to the
field. This issue was introduced by the removal of `requestAnimationFrame`, which
previously delayed execution until the DOM was fully rendered.
|
great use of a callback ref! ignore my comment on the original commit then, I wrote that before I saw this PR ❤️ |
Follow-up to #108376, which merged before I was able to fix the scroll behavior. Using `requestAnimationFrame` delays the `scrollIntoView()` and `focus()` until rendering is complete.
Follow-up to #107264. The previous `JSONForm` system managed automatic scrolling based on the URL hash. In #107264, we tried to replicate that but overcomplicated things with a `useEffect` call. In this PR, we introduce a ref callback on the `label` element, automatically handle hash scrolling, and call `label.control.focus()` to focus on the form field's input.
Follow-up to #108376, which merged before I was able to fix the scroll behavior. Using `requestAnimationFrame` delays the `scrollIntoView()` and `focus()` until rendering is complete.
Follow-up to #108414, #108376 Implements hash-based scroll and focus with a highlight animation for new Scraps forms Adds refs to all base form fields. https://github.com/user-attachments/assets/ac4c5dbd-72c5-4616-b46f-624f584e2872 Closes DE-954 --------- Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
3 participants
Follow-up to #107264.
The previous
JSONFormsystem managed automatic scrolling based on the URL hash.In #107264, we tried to replicate that but overcomplicated things with a
useEffectcall.In this PR, we introduce a ref callback on the
labelelement, automatically handle hash scrolling, and calllabel.control.focus()to focus on the form field's input.