-
Notifications
You must be signed in to change notification settings - Fork 16.7k
fix: Ensure that Playwright tile height is always positive #36027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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: | ||||||
|
||||||
| if clip_height <= 0: | |
| if clip_height <= 0 or current_element_box["width"] <= 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete validation of clip dimensions 
Tell me more
What is the issue?
The code only checks for clip_height <= 0 but doesn't validate that current_element_box["width"] > 0, yet it skips the tile claiming "invalid clip dimensions" for both width and height.
Why this matters
If the element width becomes 0 or negative (which can happen when elements are completely out of viewport), the screenshot will still be attempted with invalid width dimensions, potentially causing crashes or unexpected behavior in the screenshot API.
Suggested change ∙ Feature Preview
Add validation for both width and height dimensions:
if clip_height <= 0 or current_element_box["width"] <= 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,
)
continueProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||
|
||||||
| # Check the format string | |
| # Check the warning message content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation added to skip tiles with invalid clip dimensions only checks if
clip_height <= 0, but Playwright'spage.screenshot()requires both width and height to be greater than zero. Ifcurrent_element_box["width"]is less than or equal to zero, the code will proceed and attempt to take a screenshot with an invalid clip, causing a failure. This impacts thetake_tiled_screenshotfunction used inwebdriver.pyfor dashboard screenshots and could lead to incomplete or failed screenshot generation in production. Add a check for width in the condition to ensure both dimensions are validated.Code suggestion
Code Review Run #2fd6a4
Should Bito avoid suggestions like this for future reviews? (Manage Rules)