-
Notifications
You must be signed in to change notification settings - Fork 3.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
fix: ensure that chromium based browsers do not send out a lot of font requests when global styles change #28217
Changes from 7 commits
acc229d
71eb649
062d05f
acba6ed
cd8ad4d
9535941
48bb24e
ca48769
574331c
c36ff1c
ef4ce0b
b552e32
2666ce7
fe05080
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. This file's changes involve undoing #27860 |
||
}) | ||
|
||
it('returns false if window and body < window height', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
it('returns false window and body > window height', () => { | ||
cy.$$('body').html('<div>foo</div>') | ||
|
||
const win = cy.state('window') | ||
|
@@ -65,29 +65,6 @@ describe('src/cypress/dom/visibility', () => { | |
expect(fn()).to.be.false | ||
}) | ||
|
||
it('returns true if document element and body > window height', function () { | ||
this.add('<div style="height: 1000px; width: 10px;" />') | ||
const documentElement = Cypress.dom.wrap(cy.state('document').documentElement) | ||
|
||
const fn = () => { | ||
return dom.isScrollable(documentElement) | ||
} | ||
|
||
expect(fn()).to.be.true | ||
}) | ||
|
||
it('returns false if document element and body < window height', () => { | ||
cy.$$('body').html('<div>foo</div>') | ||
|
||
const documentElement = Cypress.dom.wrap(cy.state('document').documentElement) | ||
|
||
const fn = () => { | ||
return dom.isScrollable(documentElement) | ||
} | ||
|
||
expect(fn()).to.be.false | ||
}) | ||
|
||
it('returns false el is not scrollable', function () { | ||
const noScroll = this.add(`\ | ||
<div style="height: 100px; overflow: auto;"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. This file's changes involve undoing #27860 |
||
import $elements from '../dom/elements' | ||
import $errUtils from '../cypress/error_utils' | ||
import { callNativeMethod, getNativeProp } from '../dom/elements/nativeProps' | ||
const debug = debugFn('cypress:driver:actionability') | ||
|
||
const delay = 50 | ||
|
@@ -461,46 +460,24 @@ const verify = function (cy, $el, config, options, callbacks: VerifyCallbacks) { | |
// make scrolling occur instantly. we do this by adding a style tag | ||
// and then removing it after we finish scrolling | ||
// https://github.com/cypress-io/cypress/issues/3200 | ||
const addScrollBehaviorFix = (element: JQuery<HTMLElement>) => { | ||
const affectedParents: Map<HTMLElement, string> = new Map() | ||
const addScrollBehaviorFix = () => { | ||
let style | ||
|
||
try { | ||
let parent: JQuery<HTMLElement> | null = element | ||
const doc = $el.get(0).ownerDocument | ||
|
||
do { | ||
if ($dom.isScrollable(parent)) { | ||
const parentElement = parent[0] | ||
const style = getNativeProp(parentElement, 'style') | ||
const styles = getComputedStyle(parentElement) | ||
|
||
if (styles.scrollBehavior === 'smooth') { | ||
affectedParents.set(parentElement, callNativeMethod(style, 'getStyleProperty', 'scroll-behavior')) | ||
callNativeMethod(style, 'setStyleProperty', 'scroll-behavior', 'auto') | ||
} | ||
} | ||
|
||
parent = $dom.getFirstScrollableParent(parent) | ||
} while (parent) | ||
style = doc.createElement('style') | ||
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 commentThe 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 commentThe 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. |
||
} catch (err) { | ||
// the above shouldn't error, but out of an abundance of caution, we | ||
// ignore any errors since this fix isn't worth failing the test over | ||
} | ||
|
||
return () => { | ||
for (const [parent, value] of affectedParents) { | ||
const style = getNativeProp(parent, 'style') | ||
|
||
if (value === '') { | ||
if (callNativeMethod(style, 'getStyleProperty', 'length') === 1) { | ||
callNativeMethod(parent, 'removeAttribute', 'style') | ||
} else { | ||
callNativeMethod(style, 'removeProperty', 'scroll-behavior') | ||
} | ||
} else { | ||
callNativeMethod(style, 'setStyleProperty', 'scroll-behavior', value) | ||
} | ||
} | ||
affectedParents.clear() | ||
if (style) style.remove() | ||
} | ||
} | ||
|
||
|
@@ -523,7 +500,8 @@ const verify = function (cy, $el, config, options, callbacks: VerifyCallbacks) { | |
if (options.scrollBehavior !== false) { | ||
// scroll the element into view | ||
const scrollBehavior = scrollBehaviorOptionsMap[options.scrollBehavior] | ||
const removeScrollBehaviorFix = addScrollBehaviorFix($el) | ||
|
||
const removeScrollBehaviorFix = addScrollBehaviorFix() | ||
|
||
debug('scrollIntoView:', $el[0]) | ||
$el.get(0).scrollIntoView({ block: scrollBehavior }) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -290,12 +290,6 @@ export const isScrollable = ($el) => { | |
return false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file's changes involve undoing #27860 |
||
} | ||
|
||
const documentElement = $document.getDocumentFromElement(el).documentElement | ||
|
||
if (el === documentElement) { | ||
return checkDocumentElement($window.getWindowByElement(el), el) | ||
} | ||
|
||
// if we're any other element, we do some css calculations | ||
// to see that the overflow is correct and the scroll | ||
// area is larger than the actual height or width | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,7 +207,6 @@ const nativeGetters = { | |
body: descriptor('Document', 'body').get, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file's changes involve undoing #27860 |
||
frameElement: Object.getOwnPropertyDescriptor(window, 'frameElement')!.get, | ||
maxLength: _getMaxLength, | ||
style: descriptor('HTMLElement', 'style').get, | ||
} | ||
|
||
const nativeSetters = { | ||
|
@@ -225,16 +224,12 @@ const nativeMethods = { | |
execCommand: window.document.execCommand, | ||
getAttribute: window.Element.prototype.getAttribute, | ||
setAttribute: window.Element.prototype.setAttribute, | ||
removeAttribute: window.Element.prototype.removeAttribute, | ||
setSelectionRange: _nativeSetSelectionRange, | ||
modify: window.Selection.prototype.modify, | ||
focus: _nativeFocus, | ||
hasFocus: window.document.hasFocus, | ||
blur: _nativeBlur, | ||
select: _nativeSelect, | ||
getStyleProperty: window.CSSStyleDeclaration.prototype.getPropertyValue, | ||
setStyleProperty: window.CSSStyleDeclaration.prototype.setProperty, | ||
removeStyleProperty: window.CSSStyleDeclaration.prototype.removeProperty, | ||
} | ||
|
||
export const getNativeProp = function<T, K extends keyof T> (obj: T, prop: K): T[K] { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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. |
||
|
||
return args | ||
}, | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
import path from 'path' | ||
import fs from 'fs-extra' | ||
import type { AppCaptureProtocolInterface, ResponseEndedWithEmptyBodyOptions, ResponseStreamOptions, ResponseStreamTimedOutOptions } from '@packages/types' | ||
import type { Readable } from 'stream' | ||
|
||
const getFilePath = (filename) => { | ||
return path.join( | ||
path.resolve(__dirname), | ||
'cypress', | ||
'system-tests-protocol-dbs', | ||
`${filename}.json`, | ||
) | ||
} | ||
|
||
export class AppCaptureProtocol implements AppCaptureProtocolInterface { | ||
private filename: string | ||
private events = { | ||
numberOfFontRequests: 0, | ||
} | ||
private cdpClient: any | ||
|
||
getDbMetadata (): { offset: number, size: number } { | ||
return { | ||
offset: 0, | ||
size: 0, | ||
} | ||
} | ||
|
||
responseStreamReceived (options: ResponseStreamOptions): Readable { | ||
return options.responseStream | ||
} | ||
|
||
connectToBrowser = async (cdpClient) => { | ||
if (cdpClient) { | ||
this.cdpClient = cdpClient | ||
} | ||
|
||
this.cdpClient.on('Network.requestWillBeSent', (params) => { | ||
// For the font flooding test, we want to count the number of font requests. | ||
// There should only be 2 requests. One for each test in the spec. | ||
if (params.type === 'Font') { | ||
this.events.numberOfFontRequests += 1 | ||
} | ||
}) | ||
} | ||
|
||
addRunnables = (runnables) => { | ||
return Promise.resolve() | ||
} | ||
|
||
beforeSpec = ({ archivePath, db }) => { | ||
this.filename = getFilePath(path.basename(db.name)) | ||
|
||
if (!fs.existsSync(archivePath)) { | ||
// If a dummy file hasn't been created by the test, write a tar file so that it can be fake uploaded | ||
fs.writeFileSync(archivePath, '') | ||
} | ||
} | ||
|
||
async afterSpec (): Promise<void> { | ||
try { | ||
fs.outputFileSync(this.filename, JSON.stringify(this.events, null, 2)) | ||
} catch (e) { | ||
console.log('error writing protocol events', e) | ||
} | ||
} | ||
|
||
beforeTest = (test) => { | ||
return Promise.resolve() | ||
} | ||
|
||
commandLogAdded = (log) => { | ||
} | ||
|
||
commandLogChanged = (log) => { | ||
} | ||
|
||
viewportChanged = (input) => { | ||
} | ||
|
||
urlChanged = (input) => { | ||
} | ||
|
||
pageLoading = (input) => { | ||
} | ||
|
||
preAfterTest = (test, options) => { | ||
return Promise.resolve() | ||
} | ||
|
||
afterTest = (test) => { | ||
return Promise.resolve() | ||
} | ||
|
||
responseEndedWithEmptyBody = (options: ResponseEndedWithEmptyBodyOptions) => { | ||
} | ||
|
||
responseStreamTimedOut (options: ResponseStreamTimedOutOptions): void { | ||
} | ||
|
||
resetTest (testId: string): void { | ||
} | ||
} |
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