Fix Term Widget not registering pointer events #1614
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces changes to the terminal view's scrollbar visibility management in both the SCSS styling ( In the SCSS file, two new CSS classes are added: The corresponding TypeScript file introduces callback functions 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app/view/term/term.tsx (2)
852-857: Remove or refine console debugging statements.While logging is useful for debugging, consider removing or converting console.log statements to a logging utility with appropriate log levels in production code. This helps maintain a clean console output in production.
865-865: Use descriptive naming for stickerConfig.While the sticker configuration object is static, renaming it to something like "terminalStickerConfig" may enhance clarity and reduce confusion if there are multiple “config” objects in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/term/term.scss(2 hunks)frontend/app/view/term/term.tsx(2 hunks)
🔇 Additional comments (5)
frontend/app/view/term/term.tsx (3)
851-851: Check for null references before usage.
Since you are capturing references to DOM elements (e.g., scrollbarHideObserverRef), consider guarding against scenarios where the element might be null. Adding a simple conditional check can help avoid runtime errors if the element hasn't mounted yet.
858-863: Double-check fallback styling for termViewport.
When resetting the z-index to "auto" on pointer out, ensure there is no scenario where the terminal requires a specific default z-index other than "auto." Verify that no other elements rely on the old z-index or that "auto" doesn't inadvertently hide or surface the element in unexpected ways.
879-886: Verify observer element widths match actual scrollbar sizes.
Both observers use fixed widths (12px). If the scrollbar width changes in future design updates, these fixed values might cause a mismatch. You could dynamically calculate the observer widths or at least maintain them as SASS variables to ensure consistency.
frontend/app/view/term/term.scss (2)
39-55: Styling looks consistent.
No functional changes are introduced, and the usage of flex layout with overflow handling is consistent with other toolbar definitions.
129-146: Confirm existence of --zindex-xterm-viewport-overlay.
Ensure the custom property --zindex-xterm-viewport-overlay is defined somewhere in the codebase and has sensible defaults, especially for browsers that might not support CSS variables.
Generate this script to confirm the variable definition:
✅ Verification successful
The CSS variable --zindex-xterm-viewport-overlay is properly defined with a value of 5
The variable is defined in the codebase with a clear value and purpose (for viewport containing the scrollbar), making it safe to use in z-index calculations. The implementation in the reviewed code correctly uses this variable for layering the scrollbar observers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the definition of --zindex-xterm-viewport-overlay across SCSS/CSS files
fd -e scss -e css --exec rg --context 5 'zindex-xterm-viewport-overlay'
Length of output: 951
This is a bit janky. The problem is that we were placing the
xterm-viewportdiv, which contains the scroll observer for the xterm contents, at a higher z-index than the xterm contents, meaning that the contents couldn't register any pointer events. If we don't put a z-index, though, the scroll bar can't accept pointer events. To get around this, I've added two observer divs, which control whether the contents or the viewport have pointer event priority. The first div, theterm-scrollbar-show-observer, sits above where the scrollbar will be rendered. When the user hovers over it, it will cause the viewport div to move to a z-index above the contents. It will also enable a second div, theterm-scrollbar-hide-observer, which sits above the viewport and the term contents, but not blocking the scrollbar. When the user hovers over this div (indicating their mouse has left the scrollbar), the viewport div is moved back to its original z-index and the hide observer is set todisplay: none. This gives pointer event priority back to the contents div.This resolves an issue where the user could not click links in the terminal output.
Resolves #1357