Skip to content

Commit

Permalink
fix(screenshot): wait for stable position before taking element scree…
Browse files Browse the repository at this point in the history
…nshot (#3216)

Same goes for scrollIntoViewIfNeeded.
  • Loading branch information
dgozman authored Jul 29, 2020
1 parent c6180ed commit 9132d23
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 11 deletions.
3 changes: 2 additions & 1 deletion docs/actionability.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ Some actions like `page.click()` support `{force: true}` option that disable non
| `check()`<br>`click()`<br>`dblclick()`<br>`hover()`<br>`uncheck()` | [Visible]<br>[Stable]<br>[Enabled]<br>[Receiving Events]<br>[Attached] |
| `fill()` | [Visible]<br>[Enabled]<br>[Editable]<br>[Attached] |
| `dispatchEvent()`<br>`focus()`<br>`press()`<br>`setInputFiles()`<br>`selectOption()`<br>`type()` | [Attached] |
| `selectText()`<br>`scrollIntoViewIfNeeded()`<br>`screenshot()` | [Visible]<br>[Attached] |
| `scrollIntoViewIfNeeded()`<br>`screenshot()` | [Visible]<br>[Stable]<br>[Attached] |
| `selectText()` | [Visible]<br>[Attached] |
| `getAttribute()`<br>`innerText()`<br>`innerHTML()`<br>`textContent()` | [Attached] |

### Visible
Expand Down
22 changes: 14 additions & 8 deletions src/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {

async _waitAndScrollIntoViewIfNeeded(progress: Progress): Promise<void> {
while (progress.isRunning()) {
assertDone(throwRetargetableDOMError(await this._waitForVisible(progress)));
assertDone(throwRetargetableDOMError(await this._waitForDisplayedAtStablePosition(progress, false /* waitForEnabled */)));

progress.throwIfAborted(); // Avoid action that has side-effects.
const result = throwRetargetableDOMError(await this._scrollRectIntoViewIfNeeded());
Expand Down Expand Up @@ -321,7 +321,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
if ((options as any).__testHookBeforeStable)
await (options as any).__testHookBeforeStable();
if (!force) {
const result = await this._waitForDisplayedAtStablePositionAndEnabled(progress);
const result = await this._waitForDisplayedAtStablePosition(progress, true /* waitForEnabled */);
if (result !== 'done')
return result;
}
Expand Down Expand Up @@ -636,15 +636,21 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
return result;
}

async _waitForDisplayedAtStablePositionAndEnabled(progress: Progress): Promise<'error:notconnected' | 'done'> {
progress.logger.info(' waiting for element to be visible, enabled and not moving');
async _waitForDisplayedAtStablePosition(progress: Progress, waitForEnabled: boolean): Promise<'error:notconnected' | 'done'> {
if (waitForEnabled)
progress.logger.info(` waiting for element to be visible, enabled and not moving`);
else
progress.logger.info(` waiting for element to be visible and not moving`);
const rafCount = this._page._delegate.rafCountForStablePosition();
const poll = this._evaluateHandleInUtility(([injected, node, rafCount]) => {
return injected.waitForDisplayedAtStablePositionAndEnabled(node, rafCount);
}, rafCount);
const poll = this._evaluateHandleInUtility(([injected, node, { rafCount, waitForEnabled }]) => {
return injected.waitForDisplayedAtStablePosition(node, rafCount, waitForEnabled);
}, { rafCount, waitForEnabled });
const pollHandler = new InjectedScriptPollHandler(progress, await poll);
const result = await pollHandler.finish();
progress.logger.info(' element is visible, enabled and does not move');
if (waitForEnabled)
progress.logger.info(' element is visible, enabled and does not move');
else
progress.logger.info(' element is visible and does not move');
return result;
}

Expand Down
4 changes: 2 additions & 2 deletions src/injected/injectedScript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ export default class InjectedScript {
input.dispatchEvent(new Event('change', { 'bubbles': true }));
}

waitForDisplayedAtStablePositionAndEnabled(node: Node, rafCount: number): types.InjectedScriptPoll<'error:notconnected' | 'done'> {
waitForDisplayedAtStablePosition(node: Node, rafCount: number, waitForEnabled: boolean): types.InjectedScriptPoll<'error:notconnected' | 'done'> {
let lastRect: types.Rect | undefined;
let counter = 0;
let samePositionCounter = 0;
Expand Down Expand Up @@ -464,7 +464,7 @@ export default class InjectedScript {
const isVisible = !!style && style.visibility !== 'hidden';

const elementOrButton = element.closest('button, [role=button]') || element;
const isDisabled = ['BUTTON', 'INPUT', 'SELECT'].includes(elementOrButton.nodeName) && elementOrButton.hasAttribute('disabled');
const isDisabled = waitForEnabled && ['BUTTON', 'INPUT', 'SELECT'].includes(elementOrButton.nodeName) && elementOrButton.hasAttribute('disabled');

if (isDisplayed && isStable && isVisible && !isDisabled)
return 'done';
Expand Down
10 changes: 10 additions & 0 deletions test/assets/grid.html
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,14 @@
::-webkit-scrollbar {
display: none;
}

@keyframes move {
from { left: -500px; background-color: cyan; }
to { left: 0; background-color: rgb(255, 210, 204); }
}
.box.animation {
position: relative;
animation: 2s linear 0s move forwards;
}

</style>
15 changes: 15 additions & 0 deletions test/elementhandle.jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,21 @@ describe('ElementHandle.scrollIntoViewIfNeeded', function() {
await page.setContent('<span style="display:none"><div>Hello</div></span>');
await testWaiting(page, div => div.parentElement.style.display = 'block');
});
it('should wait for element to stop moving', async({page, server}) => {
await page.setContent(`
<style>
@keyframes move {
from { margin-left: 0; }
to { margin-left: 200px; }
}
div.animated {
animation: 2s linear 0s infinite alternate move;
}
</style>
<div class=animated>moving</div>
`);
await testWaiting(page, div => div.classList.remove('animated'));
});

it('should timeout waiting for visible', async({page, server}) => {
await page.setContent('<div style="display:none">Hello</div>');
Expand Down
15 changes: 15 additions & 0 deletions test/screenshot.jest.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,4 +537,19 @@ describe.skip(ffheadful)('ElementHandle.screenshot', function() {
await utils.verifyViewport(page, 350, 360);
await context.close();
});
it('should wait for element to stop moving', async({page, server}) => {
await page.setViewportSize({ width: 500, height: 500 });
await page.goto(server.PREFIX + '/grid.html');
const elementHandle = await page.$('.box:nth-of-type(3)');
await elementHandle.evaluate(e => e.classList.add('animation'));
const screenshot = await elementHandle.screenshot();
expect(screenshot).toBeGolden('screenshot-element-bounding-box.png');
});
it('should take screenshot of disabled button', async({page}) => {
await page.setViewportSize({ width: 500, height: 500 });
await page.setContent(`<button disabled>Click me</button>`);
const button = await page.$('button');
const screenshot = await button.screenshot();
expect(screenshot).toBeInstanceOf(Buffer);
});
});

0 comments on commit 9132d23

Please sign in to comment.