Fix #12865: prevent toolbar clipping in containers#14471
Fix #12865: prevent toolbar clipping in containers#14471TiagoLouro14 wants to merge 5 commits intostreamlit:developfrom
Conversation
Implements a fixed-position overlay strategy for the toolbar to ensure visibility inside restricted containers. This fix allows the menu to overlap borders by using a portal-like approach with ResizeObserver for synchronization. Signed-off-by: Tiago Louro <tiago.louro@tecnico.ulisboa.pt>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thanks for contributing to Streamlit! 🎈 Please make sure you have read our Contributing Guide. You can find additional information about Streamlit development in the wiki. The review process:
We're receiving many contributions and have limited review bandwidth — please expect some delay. We appreciate your patience! 🙏 |
lukasmasuch
added
change:bugfix
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR fixes an issue (#12865) where the element toolbar is clipped when rendered inside a fixed-height container with overflow: hidden. The fix renders the toolbar within a position: fixed wrapper that mirrors the parent element's bounding box, kept in sync via scroll, resize, and ResizeObserver listeners. It includes a new Playwright e2e test and a frontend unit test for the fix.
Code Quality
The approach of using a fixed-position overlay to escape overflow: hidden clipping is sound in principle, but the current implementation has a significant performance concern. The Toolbar component is used by many element types (DataFrames, images, maps, charts, etc.), and the current design attaches global scroll and resize event listeners per Toolbar instance. On every scroll event, getBoundingClientRect() is called (forcing synchronous layout) and setParentRect creates a new DOMRect object. Because React's useState uses strict equality, this triggers a re-render for every visible Toolbar on every scroll frame — a pattern that will cause noticeable scroll jank on pages with multiple elements. Both reviewers flagged concerns about the robustness and performance of the synchronization logic. This is the primary blocking issue.
The code is otherwise cleanly scoped, follows existing component patterns, and properly handles the fullscreen vs. standard mode distinction. Cleanup for listeners and the ResizeObserver is correctly implemented.
Test Coverage
Test coverage is directionally good:
- A new frontend unit test validates the fixed-wrapper positioning behavior in non-fullscreen mode.
- A new Playwright e2e test reproduces the clipped-toolbar scenario with a constrained container.
However, the e2e test assertion (expect(wrapper).to_have_css("position", "fixed")) is implementation-oriented rather than behavior-oriented. A stronger assertion would verify that the toolbar is actually visible/unclipped from the user's perspective.
Backwards Compatibility
No API or protobuf surface changes. The change is internal to the shared Toolbar UI component. The primary regression risk is visual/positioning issues in existing toolbar consumers, and performance degradation on scroll-heavy pages.
Security & Risk
No security-sensitive areas (auth, WebSocket, CORS, CSP, file handling, etc.) are affected. No new dependencies or external network calls introduced. The risk is confined to UI performance and visual correctness.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence: UI-only change for toolbar layout positioning. No routing, auth, WebSocket, asset handlers, CORS, CSP, storage, embedding, or SiS/runtime changes.
- Suggested external_test focus areas: None required. Optional confidence check: verify toolbar placement inside an iframe-hosted app with nested scroll containers.
- Confidence: High
- Assumptions and gaps: Assessment is based on the diff alone.
Accessibility
No accessibility regressions introduced. The toolbar retains its existing structure, interactive elements, and accessible labels. No new interactive controls or semantic-role changes.
Recommendations
- Critical — Avoid React state for scroll-driven positioning: Refactor the position synchronization to use a
reffor the fixed-wrapperdivand updatestyle.top,style.left,style.width,style.heightdirectly, bypassing React re-renders. UserequestAnimationFrameto batch DOM reads/writes. - Important — Lazy listener attachment: Only attach global
scrollandresizelisteners when the parent element is hovered or the toolbar islocked. This avoids tracking and layout calculations for toolbars that are invisible. - Minor — Guard against stale coordinates on content reflow: Recompute position on
mouseenterof the parent element (or another layout-shift-safe trigger) to cover cases where content reflows without triggering scroll/resize. - Minor — Strengthen e2e assertion: Assert user-visible behavior (e.g., toolbar visibility or bounding-box geometry) rather than only CSS
position: fixed.
Verdict
CHANGES REQUESTED: The approach correctly solves the toolbar-clipping problem, but the scroll-driven React state updates introduce a significant performance regression that must be addressed before merge. Both reviewers independently flagged this or related robustness concerns. The fix should use direct DOM style manipulation via refs instead of React state for high-frequency position synchronization.
Reviewer agreement:
- All reviewers (gemini-3.1-pro, gpt-5.3-codex-high) agree on CHANGES_REQUESTED.
- Both agree the fix is sound in principle but needs implementation hardening.
- Both agree there are no security, accessibility, or backwards compatibility concerns.
- Gemini focused primarily on the performance regression; GPT focused on stale-position robustness and e2e test quality. Both concerns are valid and complementary.
Missing reviews: None. All expected models (gpt-5.3-codex-high, gemini-3.1-pro) submitted reviews. claude-4.6-opus-high-thinking served as the consolidation model.
Consolidated AI review by claude-4.6-opus-high-thinking. Please verify the feedback and use your judgment.
This review also includes 3 inline comment(s) on specific code lines.
| const updatePosition = (): void => { | ||
| if (anchorRef.current?.parentElement) { | ||
| // eslint-disable-next-line streamlit-custom/no-force-reflow-access | ||
| setParentRect(anchorRef.current.parentElement.getBoundingClientRect()) |
There was a problem hiding this comment.
issue: Calling getBoundingClientRect() and setParentRect() with a new DOMRect on every scroll event triggers a React re-render for every Toolbar instance on the page (since useState uses === and each DOMRect is a new object). With many elements (DataFrames, images, maps, etc.), this causes severe scroll jank.
Suggestion: Use a useRef for the fixed-wrapper div and update its style.top/left/width/height directly (bypassing React state). Additionally, throttle updates with requestAnimationFrame, and consider only attaching scroll/resize listeners when the parent is hovered or the toolbar is locked, so invisible toolbars don't incur tracking cost.
| updatePosition() | ||
|
|
||
| window.addEventListener("resize", updatePosition) | ||
| window.addEventListener("scroll", updatePosition, true) |
There was a problem hiding this comment.
issue: The fixed-overlay position is only recomputed on resize, scroll, and parent ResizeObserver events. If the element moves because surrounding content reflows (e.g., content expansion above the element) without triggering scroll or resize, the toolbar stays at stale coordinates. Consider recomputing position right before showing the toolbar (e.g., on mouseenter of the parent) as an additional trigger to avoid misalignment.
| wrapper = toolbar.locator("xpath=..") | ||
|
|
||
| # Assert the fix: without position: fixed, this would be invisible/clipped | ||
| expect(wrapper).to_have_css("position", "fixed", timeout=10000) |
There was a problem hiding this comment.
suggestion: Asserting only position: fixed verifies the implementation detail but can still pass when the toolbar is clipped or offscreen. Consider also asserting visible geometry (e.g., that the toolbar's bounding box is within the viewport or that it is actually visible via expect(toolbar).to_be_visible()) so the test validates user-visible behavior.
|
@TiagoLouro14 this is something I already have a draft PR for taking a different approach via portals. That's why I had the issue assigned to myself. |
Sorry about that, I missed the assignment notice and jumped the gun. I’m doing this for a university project and just wanted to contribute to anything streamlit relaated. I’m happy to close this since you’ve already got a draft with portals in the works. Let me know if I can help with anything else! ;) |
|
@TiagoLouro14 no problem, sometimes assigned issues can be re-assigned. Just ping the person to check. One thing we are really encouraging people to take a look at is implementing a custom component that they design. We have recently rolled out V2 and a new gallery. https://docs.streamlit.io/develop/concepts/custom-components I am wondering actually, we get a lot of contributions from your University, could you let me know who your instructor is? I want to talk to them about custom components. One issue with submitting bug fixes or features is that our team is handling these much faster with agents so there are not a lot of available issues that are good to hand off to new contributors. |
|
@sfc-gh-lwilby, Thanks for the heads up! Since you already have a draft PR, does it make sense for me to hand this over to you? I don't want to get in your way if your progress is really substantial. I'll definitely give the custom components V2 a serious look for my project. About the instructor and other details is there a way I can send you more details or his contact in private? |
|
@TiagoLouro14 the approach here has some performance implications that my portal approach does not: #11748 My PR is a bit old, if you want, you could re-make it against the current develop, there are some review suggestions from Bob that also need to be addressed. But keep in mind this one will require some manual testing and there are some mobile concerns raised by Bob so ideally we could verify the fix works in mobile as well. I am not working on it for a few weeks, so let me know if you want to do this, ping me on your PR and I will take a look (I will be on vacation until after Easter). Could you send me a message on LinkedIn? Seems like the best contact to link here https://www.linkedin.com/in/laura-wilby-03508316/ |
|
Sure, I'll give it a go. |
3 participants
Describe your changes
Fixes the toolbar being clipped inside fixed-height containers (
st.containerwithheight). Elements likest.dataframe,st.image, andst.maprender a hover toolbar that was being cut off by the container'soverflow: hidden.The fix renders the toolbar in a
position: fixedoverlay that mirrors the parent element's bounding box, bypassing the overflow constraint. AResizeObserverand scroll/resize listeners keep the overlay synchronized with the parent.Screenshot or video (only for visual changes)
GitHub Issue Link (if applicable)
Fixes #12865
Testing Plan
"uses fixed positioning outside of fullscreen mode"toToolbar.test.tsx, verifying that the wrapper usesposition: fixedandpointer-events: noneoutside fullscreen mode.st_toolbar_overflow.py(app) andst_toolbar_overflow_test.py(Playwright test) that reproduce the clipping scenario and assertposition: fixedon the toolbar wrapper.st.container— toolbar is now fully visible.Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.