-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
test: add page.bringToFront test #4778
Conversation
@@ -130,5 +130,26 @@ module.exports.addTests = function({testRunner, expect, puppeteer, defaultBrowse | |||
await browser.close(); | |||
}); | |||
}); | |||
|
|||
describe('Page.bringToFront', function() { |
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.
This is cool, and looks like it works. I am concerned that it is more testing for the presence of a tab strip than for bring-to-front to be actually working properly. Browsers don't need to have a tab strip that behaves this way, with only one web contents visible at a time, nor do they need to make the page created by browser.newPage() visible. But maybe that's fine, and we can cross that bridge when we get to it. @aslushnikov wdyt?
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.
Browsers don't need to have a tab strip that behaves this way
Yeah, but I'd speculate that browsers are unlikely to change here in the foreseeable future.
nor do they need to make the page created by browser.newPage() visible
I don't really think we've relied on this anywhere so far. Let's not use this assumption in the test:
const page1 = await browser.newPage();
const page2 = await browser.newPage();
await page1.bringToFront();
expect(await page1.evaluate(() => document.visibilityState)).toBe('visible');
expect(await page2.evaluate(() => document.visibilityState)).toBe('hidden');
await page2.bringToFront();
expect(await page1.evaluate(() => document.visibilityState)).toBe('hidden');
expect(await page2.evaluate(() => document.visibilityState)).toBe('visible');
But maybe that's fine, and we can cross that bridge when we get to it
Yeah I like it! Let's land this!
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.
Done!
Test `page.bringToFront` in headful.
I think we can test
page.bringToFront
in headful.