Skip to content
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

Puppeteer E2E test: Use new headless mode #25979

Closed
wants to merge 7 commits into from

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented May 3, 2023

Related issue:

  Puppeteer old Headless deprecation warning: 
    In the near feature `headless: true` will default to the new Headless mode 
    for Chrome instead of the old Headless implementation. For more 
    information, please see https://developer.chrome.com/articles/new-headless/.
    Consider opting in early by passing `headless: "new"` to `puppeteer.launch()` 
    If you encounter any bugs, please report them to https://github.com/puppeteer/puppeteer/issues/new/choose.

Description

Let's see how this works...

@LeviPesin
Copy link
Contributor Author

LeviPesin commented May 3, 2023

Seems like it also changes some font rendering... Maybe it could fix some examples in the exception list.
No, moreover, it increases differences between Windows and Linux 🤦‍♂️

@LeviPesin LeviPesin marked this pull request as ready for review May 3, 2023 15:46
@LeviPesin
Copy link
Contributor Author

I think this is ready to merge now.

@mrdoob
Copy link
Owner

mrdoob commented May 4, 2023

Are you sure?

Screenshot 2023-05-04 10 34 52

@LeviPesin
Copy link
Contributor Author

I'm afraid this is the only way to fix the text rendering inconsistences without adding like 20 more examples to the exception list -- in addition to those before this PR changing to the new headless mode adds like 5 more.

@mrdoob
Copy link
Owner

mrdoob commented May 4, 2023

Uhh... Is there any other side effect you have not disclosed in this PR?

@mrdoob
Copy link
Owner

mrdoob commented May 4, 2023

I can see many other broken screenshots...

Why do we need to support the new headless mode if it's breaking things and not fixing anything?

@LeviPesin
Copy link
Contributor Author

LeviPesin commented May 4, 2023

This PR has two things:

  1. enable new headless mode
  2. disable HTML text rendering (by simply * { color: #00000000 !important; }).

2 is so not because the new headless mode introduces the issue, but rather because it makes it more severe.

I can revert the text-related changes and just add more examples to the exceptions then.

@mrdoob
Copy link
Owner

mrdoob commented May 4, 2023

Do all the OSes have the text rendering issue?

@LeviPesin
Copy link
Contributor Author

This commit didn't revert the previous ones 🤦‍♂️

@LeviPesin
Copy link
Contributor Author

Do all the OSes have the text rendering issue?

The issue is the difference of text rendering between all three OSes.

@LeviPesin
Copy link
Contributor Author

This commit didn't revert the previous ones 🤦‍♂️

#25982

@LeviPesin LeviPesin closed this May 4, 2023
@LeviPesin LeviPesin deleted the patch-3 branch May 4, 2023 02:32
@mrdoob
Copy link
Owner

mrdoob commented May 4, 2023

The issue is the difference of text rendering between all three OSes.

Then... How about we just test in one OS as we used to?

I have yet to see any benefit in testing on all OSes...

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.

None yet

2 participants