Skip to content

fix: Fix masking in single element screenshots. #1825

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Fix masking in single element screenshots. #1825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions playwright/src/main/java/com/microsoft/playwright/impl/ElementHandleImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.gson.JsonObject;
import com.microsoft.playwright.ElementHandle;
import com.microsoft.playwright.Frame;
import com.microsoft.playwright.Locator;
import com.microsoft.playwright.PlaywrightException;
import com.microsoft.playwright.options.BoundingBox;
import com.microsoft.playwright.options.ElementState;
Expand All @@ -44,7 +45,7 @@ public class ElementHandleImpl extends JSHandleImpl implements ElementHandle {

ElementHandleImpl(ChannelOwner parent, String type, String guid, JsonObject initializer) {
super(parent, type, guid, initializer);
this.frame = (FrameImpl)parent;
this.frame = (FrameImpl) parent;
}

@Override
Expand Down Expand Up @@ -282,8 +283,18 @@ public byte[] screenshot(ScreenshotOptions options) {
}
}
}
List<Locator> mask = options.mask;
options.mask = null;
JsonObject params = gson().toJsonTree(options).getAsJsonObject();
options.mask = mask;
params.remove("path");
if (mask != null) {
JsonArray maskArray = new JsonArray();
for (Locator locator : mask) {
maskArray.add(((LocatorImpl) locator).toProtocol());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of duplicating code with PageImpl, let's add a serializer for LocatorImpl.class into Serialization.java, that way it will automatically work for Locator.screenshot, ElementHandle.screenshot and Page.screenshot.

}
params.add("mask", maskArray);
}
JsonObject json = sendMessage("screenshot", params, frame.timeout(options.timeout)).getAsJsonObject();

byte[] buffer = Base64.getDecoder().decode(json.get("binary").getAsString());
Expand All @@ -304,13 +315,13 @@ public void scrollIntoViewIfNeeded(ScrollIntoViewIfNeededOptions options) {

@Override
public List<String> selectOption(String value, SelectOptionOptions options) {
String[] values = value == null ? null : new String[]{ value };
String[] values = value == null ? null : new String[]{value};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert code formatting changes here and in other places, they just clutter the PR.

return selectOption(values, options);
}

@Override
public List<String> selectOption(ElementHandle value, SelectOptionOptions options) {
ElementHandle[] values = value == null ? null : new ElementHandle[]{ value };
ElementHandle[] values = value == null ? null : new ElementHandle[]{value};
return selectOption(values, options);
}

Expand All @@ -328,7 +339,7 @@ public List<String> selectOption(String[] values, SelectOptionOptions options) {

@Override
public List<String> selectOption(SelectOption value, SelectOptionOptions options) {
SelectOption[] values = value == null ? null : new SelectOption[]{ value };
SelectOption[] values = value == null ? null : new SelectOption[]{value};
return selectOption(values, options);
}

Expand Down
79 changes: 45 additions & 34 deletions playwright/src/test/java/com/microsoft/playwright/TestPageScreenshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ static private void rafraf(Page page) {
// Do a double raf since single raf does not
// actually guarantee a new animation frame.
page.evaluate("() => new Promise(x => {\n" +
" requestAnimationFrame(() => requestAnimationFrame(x));\n" +
" })");
" requestAnimationFrame(() => requestAnimationFrame(x));\n" +
" })");
}

@Test
Expand Down Expand Up @@ -136,7 +136,7 @@ void shouldNotCaptureInfiniteWebAnimations() {
}

@Test
void maskShouldWork() {
void maskShouldWorkForPage() {
page.setViewportSize(500, 500);
page.navigate(server.PREFIX + "/grid.html");
byte[] screenshot = page.screenshot(new Page.ScreenshotOptions()
Expand All @@ -146,6 +146,17 @@ void maskShouldWork() {
assertThrows(AssertionFailedError.class, () -> assertArrayEquals(screenshot, originalScreenshot));
}

@Test
void maskShouldWorkForLocator() {
page.navigate(server.PREFIX + "/grid.html");
Locator locatorToScreenshot = page.locator("div").first();
byte[] screenshot = locatorToScreenshot.screenshot(new Locator.ScreenshotOptions()
.setMask(asList(page.locator("img"))));
// TODO: toMatchSnapshot is not present in java, so we only checks that masked screenshot is different.
byte[] originalScreenshot = locatorToScreenshot.screenshot();
assertThrows(AssertionFailedError.class, () -> assertArrayEquals(screenshot, originalScreenshot));
}

@Test
void shouldWorkWithDeviceScaleFactorAndClip() {
try (BrowserContext context = browser.newContext(new Browser.NewContextOptions()
Expand Down Expand Up @@ -186,16 +197,16 @@ void shouldWorkWithDeviceScaleFactorAndScaleDevice() {
@Test
void shouldNotCaptureBlinkingCaretByDefault() {
page.setContent("<!-- Refer to stylesheet from other origin. Accessing this\n" +
" stylesheet rules will throw.\n" +
" -->\n" +
" <link rel=stylesheet href=\"" + server.CROSS_PROCESS_PREFIX + "/injectedstyle.css\">\n" +
" <!-- make life harder: define caret color in stylesheet -->\n" +
" <style>\n" +
" div {\n" +
" caret-color: #000 !important;\n" +
" }\n" +
" </style>\n" +
" <div contenteditable=\"true\"></div>\n");
" stylesheet rules will throw.\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the formatting changes.

" -->\n" +
" <link rel=stylesheet href=\"" + server.CROSS_PROCESS_PREFIX + "/injectedstyle.css\">\n" +
" <!-- make life harder: define caret color in stylesheet -->\n" +
" <style>\n" +
" div {\n" +
" caret-color: #000 !important;\n" +
" }\n" +
" </style>\n" +
" <div contenteditable=\"true\"></div>\n");
Locator div = page.locator("div");
div.type("foo bar");
byte[] screenshot = div.screenshot();
Expand All @@ -210,19 +221,19 @@ void shouldNotCaptureBlinkingCaretByDefault() {
}

@Test
@DisabledIf(value="com.microsoft.playwright.TestBase#isFirefox", disabledReason="fixme")
@DisabledIf(value = "com.microsoft.playwright.TestBase#isFirefox", disabledReason = "fixme")
void shouldCaptureBlinkingCaretIfExplicitlyAskedFor() {
page.setContent(" <!-- Refer to stylesheet from other origin. Accessing this\n" +
" stylesheet rules will throw.\n" +
" -->\n" +
" <link rel=stylesheet href=\"" + server.CROSS_PROCESS_PREFIX + "/injectedstyle.css'}\">\n" +
" <!-- make life harder: define caret color in stylesheet -->\n" +
" <style>\n" +
" div {\n" +
" caret-color: #000 !important;\n" +
" }\n" +
" </style>\n" +
" <div contenteditable=\"true\"></div>\n");
" stylesheet rules will throw.\n" +
" -->\n" +
" <link rel=stylesheet href=\"" + server.CROSS_PROCESS_PREFIX + "/injectedstyle.css'}\">\n" +
" <!-- make life harder: define caret color in stylesheet -->\n" +
" <style>\n" +
" div {\n" +
" caret-color: #000 !important;\n" +
" }\n" +
" </style>\n" +
" <div contenteditable=\"true\"></div>\n");
Locator div = page.locator("div");
div.type("foo bar");
byte[] screenshot = div.screenshot();
Expand Down Expand Up @@ -265,32 +276,32 @@ static boolean isScreenshotTestDisabled() {
}

@Test
@DisabledIf(value="com.microsoft.playwright.TestPageScreenshot#isScreenshotTestDisabled", disabledReason="array lengths differ")
@DisabledIf(value = "com.microsoft.playwright.TestPageScreenshot#isScreenshotTestDisabled", disabledReason = "array lengths differ")
void shouldHideElementsBasedOnAttr() throws IOException {
page.setViewportSize(500, 500);
page.navigate(server.PREFIX + "/grid.html");
page.locator("div").nth(5).evaluate("element => {\n" +
" element.setAttribute('data-test-screenshot', 'hide');\n" +
"}");
" element.setAttribute('data-test-screenshot', 'hide');\n" +
"}");
byte[] actual = page.screenshot(new Page.ScreenshotOptions().setStyle("[data-test-screenshot=\"hide\"] {\n" +
" visibility: hidden;\n" +
" }"));
" visibility: hidden;\n" +
" }"));
assertArrayEquals(expectedScreenshot("hide-should-work"), actual, "Screenshots should match");
Object visibility = page.locator("div").nth(5).evaluate("element => element.style.visibility");
assertEquals("", visibility);
}

@Test
@DisabledIf(value="com.microsoft.playwright.TestPageScreenshot#isScreenshotTestDisabled", disabledReason="array lengths differ")
@DisabledIf(value = "com.microsoft.playwright.TestPageScreenshot#isScreenshotTestDisabled", disabledReason = "array lengths differ")
void shouldRemoveElementsBasedOnAttr() throws IOException {
page.setViewportSize(500, 500);
page.navigate(server.PREFIX + "/grid.html");
page.locator("div").nth(5).evaluate("element => {\n" +
" element.setAttribute('data-test-screenshot', 'remove');\n" +
" }");
" element.setAttribute('data-test-screenshot', 'remove');\n" +
" }");
byte[] actual = page.screenshot(new Page.ScreenshotOptions().setStyle("[data-test-screenshot=\"remove\"] {\n" +
" display: none;\n" +
" }"));
" display: none;\n" +
" }"));
assertArrayEquals(expectedScreenshot("remove-should-work"), actual, "Screenshots should match");
Object display = page.locator("div").nth(5).evaluate("element => element.style.display");
assertEquals("", display);
Expand Down
Loading