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
fix: ensure that chromium based browsers do not send out a lot of font requests when global styles change #28217
Conversation
3 flaky tests on run #52117 ↗︎
Details:
cypress/e2e/cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox
Review all test suite changes for PR #28217 ↗︎ |
@@ -53,7 +53,7 @@ describe('src/cypress/dom/visibility', () => { | |||
expect(fn()).to.be.true | |||
}) | |||
|
|||
it('returns false if window and body < window height', () => { |
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.
why are the visibility tests removed?
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.
@@ -427,6 +427,9 @@ export = { | |||
args.push(`--remote-debugging-port=${port}`) | |||
args.push('--remote-debugging-address=127.0.0.1') | |||
|
|||
// control memory caching per execution context so that font flooding does not occur: https://github.com/cypress-io/cypress/issues/28215 | |||
args.push('--enable-features=ScopeMemoryCachePerContext') |
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.
Is this only impacting replay & chrome/electron browsers?
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 issue is very specific to chrome/electron in terms of how they handle memory caches, etc. There may be additional nuances to firefox and webkit but I have not noticed the exact same behavior. I can clarify that this fix is only for chrome/electron though.
@@ -1678,16 +1678,12 @@ describe('src/cy/commands/actions/click', () => { | |||
it('can scroll to and click elements in html with scroll-behavior: smooth', () => { |
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 file's changes involve undoing #27860
@@ -53,7 +53,7 @@ describe('src/cypress/dom/visibility', () => { | |||
expect(fn()).to.be.true |
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 file's changes involve undoing #27860
@@ -8,7 +8,6 @@ import $utils from './../cypress/utils' | |||
import type { ElWindowPostion, ElViewportPostion, ElementPositioning } from '../dom/coordinates' |
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 file's changes involve undoing #27860
@@ -290,12 +290,6 @@ export const isScrollable = ($el) => { | |||
return false |
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 file's changes involve undoing #27860
@@ -207,7 +207,6 @@ const nativeGetters = { | |||
body: descriptor('Document', 'body').get, |
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 file's changes involve undoing #27860
style.innerHTML = '* { scroll-behavior: inherit !important; }' | ||
// there's guaranteed to be a <script> tag, so that's the safest thing | ||
// to query for and add the style tag after | ||
doc.querySelector('script').after(style) |
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.
Could we use adopted stylesheets to insert this rule without modifying the DOM itself?
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 was existing code that was reverted. I think this is something we could explore, but I'd rather revert the code to where it was previously and maybe explore this separately.
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
We are seeing issues where fonts flood as a result of various things that are either done by an application or by Cypress.
Chrome has some behavior that triggers reloading of in-memory font assets when global style sheets are updated. This has the effect of triggering all font requests to reload. Even though they are in-memory, this produces a flood of CDP events which backs up proxy correlations and bloats the test replay data being captured.
We can fix this by ensuring that the in memory cache is only used in a given context via the ScopeMemoryCachePerContext feature. This will ensure that the resource is in a local cache and it won't replay the CDP events (https://github.com/chromium/chromium/blob/118.0.5993.117/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc#L2650-L2652).
Note that we initially tried to fix this issue via #27860 when we thought it was something that Cypress was inherently doing as part of actionability. This did not resolve the issue as frameworks in the apps themselves can be causing the problem as well. As a result as a part of fixing this, we should undo: #27860
Steps to test
The additional system test covers this scenario
How has the user experience changed?
You will no longer be seeing large #'s of font requests in dev tools or in Test Replay when global styles are messed with.
PR Tasks
cypress-documentation
?type definitions
?