Skip to content

Conversation

eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Sep 2, 2025

Treats <base>, <link>, <meta>, <script>, <style> and <title> as visible if attached in waitForSelector(selector).

The default of checking for visibility is generally desired. However, when selecting these metadata elements, you don't care about visibility since the user agent will never directly render the elements. You almost always just want to assert on their properties (text content of <title> or value attributes in <meta>). E.g. in vercel/next.js we have to switch the asserted state to attached whenever we want to select metadata elements. This provides no additional clarity in the test. Worse, we had to adjust the selectors to make it clear that state: 'attached' is safe for this specific selector.

Having to include state: 'attached' for these elements is cumbersome since you have to double check that the selector is actually targeting a metadata element to not risk accidentally selecting elements that should render something but aren't visible.

I excluded <template> from this lists since that may be a footgun to assert on its contents and consider it "visible".

@eps1lon eps1lon marked this pull request as ready for review September 2, 2025 11:11

This comment has been minimized.

Copy link
Contributor

github-actions bot commented Sep 2, 2025

Test results for "tests 1"

1 failed
❌ [playwright-test] › runner.spec.ts:118 › should ignore subprocess creation error because of SIGINT @macos-latest-node18-1

2 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1079 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node18`
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104 › should work with strict CSP policy `@firefox-ubuntu-22.04-node18`

46737 passed, 817 skipped


Merge workflow run.

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

Hi, greetings from Berlin. I understand how this can be seen as a footgun, although behaviour that's conditional on where the locator resolves is a footgun of its own! I'll bring it up in our team discussion tonight.

@eps1lon
Copy link
Contributor Author

eps1lon commented Sep 3, 2025

What would be an accidental location of <script> with the new heuristics? In our tests, all of the locators on these elements want attached instead of visible.

The point being that display: none or visibility: hidden have no effect on the functionality of these elements. An invisible <script> or <title> would still affect the document. So waitForSelector would actually be misleading you into thinking the element has no effect with the old heuristics when it does.

@Skn0tt
Copy link
Member

Skn0tt commented Sep 3, 2025

I'm mostly worried that the list of elements we deem "metadata" isn't the right one for some users. For example, should <head> and <source> be in the list? Also, what if the page is https://secretgeek.github.io/html_wysiwyg/html.html?

You're the first one to report this, so it's probably not a big issue outside of framework testing. Do we want to make the vast majority of Playwright users read through "Metadata elements are always considered visible"? The average reader is not asserting on their metadata elements because they don't work on a framework; but we still made them read and understand this new complexity. And it might throw them off while they're debugging something.

So essentially it's a tradeoff between complexity beared by the average Playwright user, and complexity beared by framework authors like yourself. And since framework authors tend to have an excellent understanding of the DOM, we need to be especially mindful of the average Playwright user here.

@yury-s
Copy link
Member

yury-s commented Sep 12, 2025

Closing per the comment above, looks like there is no consensus on this change. Feel free to file an issue if you disagree and we can continue the discussion there.

@yury-s yury-s closed this Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants