eschutho opened a new pull request, #35806:
URL: https://github.com/apache/superset/pull/35806

   ### SUMMARY
   
   Fixes the "Clipped area is either empty or outside the resulting image" 
error that occurs when generating tiled screenshots for large dashboards (50+ 
charts).
   
   **Root Cause:**
   When scrolling through tall dashboards to capture tiles, 
`getBoundingClientRect()` returns negative Y coordinates for elements scrolled 
above the viewport. Playwright rejects these invalid clip coordinates, causing 
the screenshot to fail.
   
   **Solution:**
   - Add comprehensive viewport bounds tracking and validation
   - Clamp clip coordinates to always be within viewport (≥0)
   - Calculate visible portions when elements are partially scrolled off-screen
   - Skip invalid tiles with warnings instead of crashing
   - Validate all clip dimensions before calling `page.screenshot()`
   
   **Changes:**
   - `superset/utils/screenshot_utils.py`: Enhanced clip validation logic 
(lines 142-205)
   - `tests/unit_tests/utils/test_screenshot_utils.py`: Added 6 new edge case 
tests
   
   **Test Coverage:**
   All 21 unit tests passing, including new tests for:
   - Negative element positions (scrolled above viewport)
   - Elements extending beyond viewport boundaries
   - Invalid clip dimensions (zero/negative width/height)
   - Elements completely scrolled out of view
   - Zero-width elements
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   **BEFORE:**
   ```
   playwright._impl._errors.Error: Page.screenshot: Clipped area is either 
empty or outside the resulting image
   Call log:
   taking page screenshot
     - waiting for fonts to load...
     - fonts loaded
   ```
   Tiled screenshot fails → Falls back to single screenshot → Empty PDF for 
large dashboards
   
   **AFTER:**
   - Tiled screenshots complete successfully for dashboards with 50+ charts
   - No more clip validation errors
   - Invalid tiles are gracefully skipped with warnings
   
   ### TESTING INSTRUCTIONS
   
   1. **Unit Tests:**
      ```bash
      pytest tests/unit_tests/utils/test_screenshot_utils.py -v
      ```
      All 21 tests should pass
   
   2. **Manual Testing (requires large dashboard):**
      - Create a dashboard with 50+ charts (one per row)
      - Enable tiled screenshots:
        ```python
        SCREENSHOT_TILED_ENABLED = True
        SCREENSHOT_TILED_CHART_THRESHOLD = 20
        ```
      - Generate a screenshot/report for the dashboard
      - Verify: No "Clipped area" errors in logs
      - Verify: PDF/image is generated correctly (not empty)
   
   3. **Verify Logs:**
      - Should see "Large dashboard detected" and "Using tiled screenshots" 
messages
      - Should NOT see clip validation errors
      - May see "Skipping tile X/Y - invalid clip dimensions" warnings 
(expected for edge cases)
   
   ### ADDITIONAL INFORMATION
   
   - [ ] Has associated issue:
   - [ ] Required feature flags: `SCREENSHOT_TILED_ENABLED=True` (optional, for 
testing)
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in 
[SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to