fix: implement native browser fullscreen for dashboard charts#38819
fix: implement native browser fullscreen for dashboard charts#38819venkateshwaracholan wants to merge 26 commits intoapache:masterfrom
Conversation
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
…visibility Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
…visibility Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
…n SliceHeaderControls Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Code Review Agent Run #a19fedActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Sequence DiagramThis PR replaces CSS-only chart expansion with native browser fullscreen on the chart holder element. It also updates portal and tooltip behavior so dropdowns and tooltips render correctly inside the fullscreen layer and keeps UI state synced when users exit with Esc. sequenceDiagram
participant User
participant SliceHeaderControls
participant BrowserFullscreen
participant ChartHolder
participant ChartUI
User->>SliceHeaderControls: Choose Enter fullscreen
SliceHeaderControls->>BrowserFullscreen: Request fullscreen for chart holder
BrowserFullscreen-->>ChartHolder: Activate fullscreen element
SliceHeaderControls->>ChartHolder: Toggle fullsize state and trigger resize
ChartHolder->>ChartUI: Render popups and menus in chart holder when fullscreen
ChartUI->>ChartUI: Use fullscreen aware tooltip container
User->>BrowserFullscreen: Press Esc to exit
BrowserFullscreen-->>SliceHeaderControls: Fire fullscreen change event
SliceHeaderControls->>ChartHolder: Sync fullsize state to normal mode
Generated by CodeAnt AI |
|
|
||
| export const fullscreenStyles = (theme: SupersetTheme) => css` | ||
| /* Using && to increase specificity without needing !important as often */ | ||
| &&[data-test='dashboard-component-chart-holder']:fullscreen { |
There was a problem hiding this comment.
Suggestion: The selector starts with && inside a Global style block, which has no parent selector to expand &. That makes the fullscreen rule invalid and causes the main fullscreen layout styles to be skipped. Use a normal explicit selector so the rule actually matches the fullscreen chart holder. [logic error]
Severity Level: Critical 🚨
- ⚠️ Fullscreen chart may not get intended 100vw/100vh layout.
- ⚠️ New fullscreen feature can appear partially broken.| &&[data-test='dashboard-component-chart-holder']:fullscreen { | |
| [data-test='dashboard-component-chart-holder']:fullscreen { |
Steps of Reproduction ✅
1. Open a dashboard chart menu and click fullscreen; `MenuKeys.Fullscreen` in
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:239-249` calls
`el.requestFullscreen()` then `props.handleToggleFullSize()`.
2. `handleToggleFullSize` in
`superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:232-234`
sets fullsize state, making `isFullSize` true for that chart.
3. With `isFullSize === true`, `SliceHeaderControls` renders `<Global
styles={fullscreenStyles(theme)} />` at
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:656`.
4. In `superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx:23`, the
primary fullscreen selector starts with `&&...`; in a global stylesheet there is no parent
selector context, so this selector does not reliably match, causing the main fullscreen
sizing/layout declarations in that block to be skipped.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 23:23
**Comment:**
*Logic Error: The selector starts with `&&` inside a `Global` style block, which has no parent selector to expand `&`. That makes the fullscreen rule invalid and causes the main fullscreen layout styles to be skipped. Use a normal explicit selector so the rule actually matches the fullscreen chart holder.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
| import { css, SupersetTheme } from '@apache-superset/core/theme'; | ||
|
|
||
| export const fullscreenStyles = (theme: SupersetTheme) => css` | ||
| /* Using && to increase specificity without needing !important as often */ |
There was a problem hiding this comment.
yes, removed it now, thanks
| /> | ||
| ); | ||
| }; | ||
| const FULLSCREEN_Z_INDEX = 10001; |
There was a problem hiding this comment.
is there an ant design way of doing this? Custom z indexes are a bit troublesome
There was a problem hiding this comment.
Replaced the hardcoded constant with supersetTheme.zIndexPopupBase + 1 to align with the AntD design tokens.
| overflow: visible; | ||
| position: relative; | ||
| pointer-events: auto; | ||
| z-index: 1000; |
There was a problem hiding this comment.
Let's check if there is an antd way of doing this
There was a problem hiding this comment.
Swapped the magic number for ${theme.zIndexPopupBase} to use the standardized theme token
| .ant-modal-root, | ||
| .ant-select-dropdown, | ||
| .ant-popover { | ||
| z-index: 99999; |
There was a problem hiding this comment.
Updated this to ${theme.zIndexPopupBase + 1} to ensure popups consistently layer above the fullscreen container using theme tokens.
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
| background-color: ${theme.colorBgBase}; | ||
| width: 100vw; | ||
| height: 100vh; | ||
| display: flex; |
There was a problem hiding this comment.
Suggestion: The fullscreen container sets width: 100vw and height: 100vh while also adding padding, which makes the rendered box larger than the viewport under default content-box sizing. This can cause overflow/cropping in fullscreen mode. Add box-sizing: border-box so padding is included in the fullscreen dimensions. [logic error]
Severity Level: Major ⚠️
- ⚠️ Fullscreen chart container can exceed viewport bounds.
- ⚠️ Fullscreen chart content may clip near edges.| display: flex; | |
| box-sizing: border-box; |
Steps of Reproduction ✅
1. Open a dashboard chart and trigger **Enter fullscreen** from `SliceHeaderControls`
menu; this executes `el?.requestFullscreen?.()` in
`superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:243`.
2. On success, `handleToggleFullSize()` is called (`index.tsx:245-247`), setting chart
full-size state through `ChartHolder`
(`superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:232-234`).
3. While full-size is true, `Global styles={fullscreenStyles(theme)}` is mounted at
`index.tsx:656`, applying `Styles.tsx:22-33` to
`[data-test='dashboard-component-chart-holder']:fullscreen`.
4. In that block, `width: 100vw; height: 100vh; padding: ...` (`Styles.tsx:24-28`) uses
default content-box sizing unless overridden; padding is added outside viewport-sized
content box, causing overflow/cropping risk in fullscreen layouts.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 26:26
**Comment:**
*Logic Error: The fullscreen container sets `width: 100vw` and `height: 100vh` while also adding padding, which makes the rendered box larger than the viewport under default `content-box` sizing. This can cause overflow/cropping in fullscreen mode. Add `box-sizing: border-box` so padding is included in the fullscreen dimensions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
Fixed. Added box-sizing: border-box to include padding within the viewport and prevent overflow.
Signed-off-by: Venkateshwaran Shanmugham <venkateshwaracholan@gmail.com>
Code Review Agent Run #f401b1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
@msyavuz I have addressed the review comments and made the changes, Please review. |
|
Bito Review Failed - Technical Failure Bito encountered technical difficulties while starting a code review session. To retry, type |
Code Review Agent Run #2e36c0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
| pointer-events: auto; | ||
| } | ||
|
|
||
| [data-test='dashboard-component-chart-header'] { |
There was a problem hiding this comment.
Suggestion: The header z-index fix targets a non-existent test attribute (dashboard-component-chart-header), so the style never applies and fullscreen header layering remains broken. Point the selector to the actual header attribute and scope it under the fullscreen chart holder so it applies only in fullscreen mode. [logic error]
Severity Level: Major ⚠️
- ⚠️ Fullscreen chart header z-index fix never executes.
- ⚠️ Header/menu layering can regress in fullscreen mode.| [data-test='dashboard-component-chart-header'] { | |
| [data-test='dashboard-component-chart-holder']:fullscreen [data-test='slice-header'] { |
Steps of Reproduction ✅
1. Open a dashboard chart and trigger fullscreen from menu path
`SliceHeaderControls/index.tsx:238-245` (`MenuKeys.Fullscreen` calls
`el?.requestFullscreen?.()` and then toggles full size state).
2. Confirm fullscreen global CSS is injected at `SliceHeaderControls/index.tsx:655`
(`{isFullSize && <Global styles={fullscreenStyles(theme)} />}`).
3. Inspect selector in `SliceHeaderControls/Styles.tsx:66-69`; it targets
`[data-test='dashboard-component-chart-header']`, but grep shows no such attribute exists
anywhere else in frontend code.
4. Inspect actual header markup at `SliceHeader/index.tsx:260`; header is rendered as
`data-test="slice-header"`, so the z-index rule never matches any real element.
5. Because this rule never applies, intended header layering fix is effectively dead code
during fullscreen; changing selector to existing `slice-header` under fullscreen holder
makes the rule executable.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
**Line:** 66:66
**Comment:**
*Logic Error: The header z-index fix targets a non-existent test attribute (`dashboard-component-chart-header`), so the style never applies and fullscreen header layering remains broken. Point the selector to the actual header attribute and scope it under the fullscreen chart holder so it applies only in fullscreen mode.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
superset-frontend/src/dashboard/components/SliceHeaderControls/Styles.tsx
Outdated
Show resolved
Hide resolved
…/Styles.tsx Co-authored-by: Mehmet Salih Yavuz <salih.yavuz@proton.me>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Bito Review Failed - Technical Failure |
Code Review Agent Run #298e15Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
User description
Summary
This PR implements the native Browser Fullscreen API for individual dashboard charts. Previously, "fullscreen" was a CSS-based expansion within the viewport, which caused issues with z-index, tooltip clipping, and browser chrome interference.
By leveraging the
requestFullscreenAPI on theChartHoldercomponent, charts now utilize the full hardware display, providing a more immersive experience.Key Changes
requestFullscreen()andexitFullscreen()inSliceHeaderControls.plugin-chart-echartsto toggleappendToBodybased ondocument.fullscreenElement. This ensures tooltips render inside the fullscreen DOM tree rather than behind it.chartHolderReffromChartHolder->Chart->SliceHeader->SliceHeaderControlsto allow the menu to target the container.ChartHoldercontent inAntdThemeProviderwith a dynamicgetPopupContainer. When in fullscreen, AntD Portals (Selects, Dropdowns) are attached to the chart holder instead ofdocument.body.Styles.tsxto handle the:fullscreenpseudo-class layout, ensuring the chart occupies100vw/100vhwith correct padding and background.fullscreenchangelistener to sync the internalisFullSizestate if a user exits via theEsckey.Fixes #38589
Screenshots / GIFs
Before (Viewport)
Screen.Recording.2026-03-24.at.5.44.59.PM.mov
After (Native API)
Screen.Recording.2026-03-24.at.5.41.34.PM.mov
Testing Instructions
Esckey or the "Exit Fullscreen" icon and verify the chart returns to its original grid position.Checklist
SliceHeaderControls.test.tsx.CodeAnt-AI Description
Use native fullscreen for dashboard charts and keep charts usable inside it
What Changed
Impact
✅ True full-screen chart view✅ Clearer chart tooltips in fullscreen✅ Fewer broken popups over charts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.