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

chore: WebKit support (development-only) #15533

Merged
merged 28 commits into from
Aug 15, 2022
Merged

chore: WebKit support (development-only) #15533

merged 28 commits into from
Aug 15, 2022

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Mar 16, 2021

User facing changelog

n/a - does not expose WebKit in production yet, only in development

Additional details

  • This PR adds development-only support for the WebKit browser. It will detect playwright-webkit installed in a project and if it is found, it will be made available in Cypress as --browser webkit
  • See Safari (WebKit) Support #6422 for remaining work - trying to keep PRs in this epic small since there's a lot to parse

Pre-merge tasks:

  • Merge with latest develop
  • Add driver test CI job
  • Double-check that --headless works w/o Xvfb

How has the user experience changed?

In development only, WebKit will be detected in run/open mode if playwright-webkit is installed.

image

PR Tasks

  • Have tests been added/updated?
    • Only tests relating to browser detection have been added/updated. Future test updates will come as remaining functionality is fixed, see Safari (WebKit) Support #6422 for details.
  • [na] Has the original issue or this PR been tagged with a release in ZenHub?
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

Co-authored-by: Weyert de Boer [email protected]

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 16, 2021

Thanks for taking the time to open a PR!

@jennifer-shehane jennifer-shehane mentioned this pull request Mar 27, 2021
21 tasks
@weyert weyert mentioned this pull request Aug 16, 2021
5 tasks
@jonatan-mobal
Copy link

Any updates on this?

@flotwig flotwig requested review from tbiethman and BlueWinds August 8, 2022 15:38
@cypress
Copy link

cypress bot commented Aug 8, 2022



Test summary

37828 0 615 0Flakiness 7


Run details

Project cypress
Status Passed
Commit ac70fc8
Started Aug 15, 2022 10:39 PM
Ended Aug 15, 2022 11:01 PM
Duration 21:41 💡
OS Linux Debian - 11.3
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay with deprecated delayMs param
2 network stubbing > intercepting request > can delay with deprecated delayMs param
3 network stubbing > intercepting request > can delay with deprecated delayMs param
4 network stubbing > intercepting request > can delay with deprecated delayMs param
5 network stubbing > intercepting request > can delay with deprecated delayMs param
This comment includes only the first 5 flaky tests. See all 7 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@flotwig flotwig marked this pull request as ready for review August 8, 2022 20:11
@flotwig flotwig requested review from a team as code owners August 8, 2022 20:11
@lmiller1990
Copy link
Contributor

lmiller1990 commented Aug 10, 2022

Still going over the code, but I took it for a test drive. It functions like it should, but I am noticing everything in the webkit driver is kind of blurry.

Compare the text from GH (left) and the spec list (right). I am guessing this isn't on our end, but the webkit-playwright code? Either way, users might notice this - we might want to include a note in the experimental release (unless the bug is localised to my machine?)

I am on an M1 running under arm64 if that is relevant.

I will continue the code review tomorrow. I am excited for WebKit support!

Image: text in webkit browser is a bit blurry (compare to address bar or GH text in background).

image

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

I'm still working through the code - posted my finding so far, but functionality seems to work as expected.

@lmiller1990 lmiller1990 requested a review from a team August 10, 2022 10:42
@tbiethman
Copy link
Contributor

@lmiller1990 I can confirm I see the same blurry rendering on my Macbook display, but it looks crisp on my external display (with a much lower pixel density).

@flotwig
Copy link
Contributor Author

flotwig commented Aug 10, 2022

@lmiller1990 @tbiethman We check and set 2x pixel density on other browsers but not in WebKit. I can add it to the list of things to fix in the parent epic, but in the spirit of keeping this PR minimal let's not worry about it here. It seems like we can set devicePixelRatio, but not in browserType.launch, so it requires some further investigation as to how to do this with WebKit: https://playwright.dev/docs/api/class-browsertype#browser-type-launch-persistent-context

@tbiethman
Copy link
Contributor

tbiethman commented Aug 10, 2022

Code looks good in this initial state. Here are a couple functional findings that don't necessarily need to hold up this initial PR, but I'll let you be the judge:

macOS

  • Focus button in launchpad does not focus Webkit browser.
  • I'm seeing GraphQL errors that are only present within the Webkit browser. Both e2e/component testing projects seem to be effected, but it can take longer for the error to be presented in component tests.

Screen Shot 2022-08-10 at 10 03 36 AM

  • Using the Tab key only seems to focus certain elements within the Webkit browser. I imagine this is related to the Safari setting to "Press Tab to highlight each item on a webpage", which must be enabled to behave similar to Chrome/Firefox. Not sure if there's a corresponding launch flag (or if this has any impact on our .focus() command)?

Screen Shot 2022-08-10 at 10 29 26 AM

Windows

  • Seeing various failures in both e2e and component tests in our internal projects. I'm sure there are incompatibilities we'll want to document, not sure if these are known about or not:

Screen Shot 2022-08-10 at 10 57 09 AM

Screen Shot 2022-08-10 at 10 58 01 AM

@flotwig
Copy link
Contributor Author

flotwig commented Aug 10, 2022

@tbiethman Thanks for the detailed feedback!

* `Focus` button in launchpad does not focus Webkit browser.

Marked it as "less critical" here: #6422 I'm using page.bringToFront to focus but it seems to not behave the same as Chrome.

* I'm seeing GraphQL errors that are only present within the Webkit browser. Both e2e/component testing projects seem to be effected, but it can take longer for the error to be presented in component tests.

Added as a must-fix to #6422

* Using the Tab key only seems to focus certain elements within the Webkit browser. I imagine this is related to the Safari setting to "Press Tab to highlight each item on a webpage", which must be enabled to behave similar to Chrome/Firefox. Not sure if there's a corresponding launch flag (or if this has any impact on our `.focus()` command)?

I have noticed some failures in the driver tests related to focus, thanks for the tip on this. I'll keep this in mind when looking at those.

* Seeing various failures in both e2e and component tests in our internal projects. I'm sure there are incompatibilities we'll want to document, not sure if these are known about or not:

Not known to me yet - I've been working on fixing the driver tests first before checking out other projects, since that should resolve most common issues. I plan on also adding certain system-tests. Limitations that come from those will be documented.

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Left some comments, they are mainly related to style, though - still trying the functionality out.

@lmiller1990
Copy link
Contributor

I found something strange. I don't think it needs to block this, but if I start run mode, then cancel it mid-run, global mode opens after a few seconds.

It's also kind of slow (I guess this is just how it is, at least for now). Since this is development only, I think it's fine to merge up, I'll keep posting things I notice as I continue to play around with it (which will be easier once it's merged).

bug.mov

@lmiller1990 lmiller1990 self-requested a review August 11, 2022 03:04
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Left some potential improvements to code and noted a weird bug I encountered.

@flotwig
Copy link
Contributor Author

flotwig commented Aug 15, 2022

@lmiller1990 I've noticed that too. Haven't been able to track it down yet, but it's super annoying. I have it captured as a must-fix here: #6422

@flotwig flotwig merged commit 4580330 into develop Aug 15, 2022
@flotwig flotwig deleted the cy-webkit branch August 15, 2022 22:53
@weyert
Copy link

weyert commented Aug 22, 2022

Any plans to mention my name when release this feature?

@BlueWinds
Copy link
Contributor

@weyert - Traditionally we'd do something like this: https://docs.cypress.io/api/commands/selectFile#Community-Recognition. @flotwig - I don't see that we have a docs PR for this, just a reasonable reminder to make sure we mention community contributions when it's ready!

@flotwig
Copy link
Contributor Author

flotwig commented Aug 22, 2022

Any plans to mention my name when release this feature?

@weyert I was planning on it, since this does use your initial approach of driving PW. I actually meant to have you as a Co-authored-by on this PR but I guess I managed to merge without including the PR message in the commit message - oops. I'll credit you in the release announcement and docs, unless you don't want to be recognized.

@weyert
Copy link

weyert commented Aug 22, 2022

I would greatly appreciate that, if you would do that. Thank you 🤗❤️

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.

7 participants