diff --git a/superset/utils/screenshot_utils.py b/superset/utils/screenshot_utils.py index 7c996e606fe9..a29e32521aa7 100644 --- a/superset/utils/screenshot_utils.py +++ b/superset/utils/screenshot_utils.py @@ -156,12 +156,28 @@ def take_tiled_screenshot( remaining_content = dashboard_height - tile_start_in_element tile_content_height = min(viewport_height, remaining_content) + # Determine clip height (use visible element height in viewport) + clip_height = min(tile_content_height, current_element_box["height"]) + + # Skip tile if dimensions are invalid (width or height <= 0) + # This can happen if element is completely scrolled out of viewport + if clip_height <= 0: + logger.warning( + "Skipping tile %s/%s due to invalid clip dimensions: " + "width=%s, height=%s (element may be scrolled out of viewport)", + i + 1, + num_tiles, + current_element_box["width"], + clip_height, + ) + continue + # Clip to capture only the current tile portion of the element clip = { "x": current_element_box["x"], "y": current_element_box["y"], "width": current_element_box["width"], - "height": min(tile_content_height, current_element_box["height"]), + "height": clip_height, } # Take screenshot with clipping to capture only this tile's content diff --git a/tests/unit_tests/utils/test_screenshot_utils.py b/tests/unit_tests/utils/test_screenshot_utils.py index 67d4d638e999..a8106e5c9202 100644 --- a/tests/unit_tests/utils/test_screenshot_utils.py +++ b/tests/unit_tests/utils/test_screenshot_utils.py @@ -315,3 +315,58 @@ def test_screenshot_clip_parameters(self, mock_page): assert clip["width"] == 800 # Height should be min of viewport_height and remaining content assert clip["height"] <= 600 # Element height from mock + + def test_skips_tiles_with_zero_height(self): + """Test that tiles with zero height are skipped.""" + mock_page = MagicMock() + + # Mock element locator + element = MagicMock() + mock_page.locator.return_value = element + + # Mock element info - 4000px tall dashboard + element_info = {"height": 4000, "top": 100, "left": 50, "width": 800} + + # First tile: valid clip region + valid_element_box = {"x": 50, "y": 200, "width": 800, "height": 600} + + # Second tile: element scrolled completely out (zero height) + invalid_element_box = {"x": 50, "y": -100, "width": 800, "height": 0} + + # For 2 tiles (4000px / 2000px = 2): + # 1 initial + 2 scroll + 2 element box + 1 reset = 6 calls + mock_page.evaluate.side_effect = [ + element_info, # Initial call for dashboard dimensions + None, # First scroll call + valid_element_box, # First element box (valid) + None, # Second scroll call + invalid_element_box, # Second element box (zero height) + None, # Reset scroll call + ] + + mock_page.screenshot.return_value = b"fake_screenshot" + + with patch("superset.utils.screenshot_utils.logger") as mock_logger: + with patch( + "superset.utils.screenshot_utils.combine_screenshot_tiles" + ) as mock_combine: + mock_combine.return_value = b"combined" + + result = take_tiled_screenshot( + mock_page, "dashboard", viewport_height=2000 + ) + + # Should still succeed with partial tiles + assert result == b"combined" + + # Should only take 1 screenshot (second tile skipped) + assert mock_page.screenshot.call_count == 1 + + # Should log warning about skipped tile + mock_logger.warning.assert_called_once() + warning_call = mock_logger.warning.call_args + # Check the format string + assert "invalid clip dimensions" in warning_call[0][0] + # Check the arguments (tile 2/2) + assert warning_call[0][1] == 2 # tile number (i + 1) + assert warning_call[0][2] == 2 # num_tiles