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

fix: AUT Redirect bug on form / anchor target #16394

Merged
merged 34 commits into from
May 10, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
72a9afb
Add failing test for #3975
tgriesser Apr 14, 2021
d2d8f21
Proposed fix for #3975
tgriesser Apr 14, 2021
5be3049
Pass a function wrapper rather than new Function
tgriesser Apr 15, 2021
379eaba
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser Apr 15, 2021
fbf8e54
Add eventHandlerWrap to XHR
tgriesser Apr 15, 2021
97073d0
Merge branch 'tgriesser/fix/3975-redirect-bug' of github.com:cypress-…
tgriesser Apr 15, 2021
5d954b1
Change impl to set bridge on window, fallback to Fn ctor
tgriesser Apr 15, 2021
86c3212
Fix object lookup syntax
tgriesser Apr 15, 2021
135b55b
Fix test
tgriesser Apr 16, 2021
94f1fa4
Pairing with @brian-mann
tgriesser Apr 16, 2021
7609f85
Fix tests
tgriesser Apr 26, 2021
88b2589
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser Apr 26, 2021
f2c0ea0
Ensure we have contentWindowListener
tgriesser Apr 26, 2021
341e745
Consolidate & document impl
tgriesser Apr 26, 2021
dfb5b57
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser Apr 26, 2021
9fdc0e2
Split out tests for #1244
tgriesser Apr 28, 2021
267c8ba
Cleanup implementation, wrapping only needed on direct patches
tgriesser Apr 28, 2021
b8e0cee
Give a more descriptive bridge fn name for the debugger
tgriesser Apr 28, 2021
2d06c65
Forgot to save file
tgriesser Apr 28, 2021
8fd4a6c
Fixes
tgriesser Apr 28, 2021
447c0ad
Guard against event already having target descriptor
tgriesser Apr 28, 2021
bc722aa
Fix window stub
tgriesser Apr 29, 2021
745667c
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser Apr 29, 2021
dc956eb
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser Apr 29, 2021
3d0a5e6
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser May 3, 2021
23d86f7
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
tgriesser May 4, 2021
42358a9
improve tests for 1244
tgriesser May 4, 2021
d1b31bf
Adding guards for anchor elements as well as forms
tgriesser May 7, 2021
5af4776
Add tests for _blank
tgriesser May 7, 2021
09abfde
Merge branch 'develop' into tgriesser/fix/1244-redirect-bug
tgriesser May 7, 2021
aac40ed
Capture phase needs to be taken into account for removeEventListener
tgriesser May 7, 2021
43db8be
Add more tests, only guard against _top / _parent
tgriesser May 7, 2021
3963773
Merge branch 'develop' into tgriesser/fix/1244-redirect-bug
tgriesser May 10, 2021
23e0241
Explicitly test to ensure _parent is not stripped in nested frames
tgriesser May 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Pass a function wrapper rather than new Function
  • Loading branch information
tgriesser committed Apr 15, 2021
commit 5be3049eb6ebbdf3bcea21b3940896cedce016a6
8 changes: 7 additions & 1 deletion cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5166,6 +5166,12 @@ declare namespace Cypress {
*/
type Task = (value: any) => any

/**
* Takes an event handler, and returns a new event handler defined in the
* AUT, ensuring the event is associated with the correct window.
*/
type EventHandlerWrap = <T extends Function>(fn: T) => T

interface Tasks {
[key: string]: Task
}
Expand Down Expand Up @@ -5284,7 +5290,7 @@ declare namespace Cypress {
* Useful to modify the window on a page transition.
* @see https://on.cypress.io/catalog-of-events#App-Events
*/
(action: 'window:before:load', fn: (win: AUTWindow) => void): Cypress
(action: 'window:before:load', fn: (win: AUTWindow, eventHandlerWrap?: EventHandlerWrap | undefined) => void): Cypress
/**
* Fires after all your resources have finished loading after a page transition.
* This fires at the exact same time as a `cy.visit()` `onLoad` callback.
Expand Down
3 changes: 2 additions & 1 deletion cli/types/tests/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ Cypress.on('window:alert', (text) => {
text // $ExpectType string
})

Cypress.on('window:before:load', (win) => {
Cypress.on('window:before:load', (win, fn) => {
win // $ExpectType AUTWindow
fn // $ExpectType Function | undefined
})

Cypress.on('window:load', (win) => {
Expand Down
4 changes: 2 additions & 2 deletions packages/driver/src/cy/commands/xhr.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,10 +330,10 @@ module.exports = (Commands, Cypress, cy, state, config) => {
return null
})

Cypress.on('window:before:load', (contentWindow) => {
Cypress.on('window:before:load', (contentWindow, eventHandlerWrap = (fn) => fn) => {
if (server) {
// dynamically bind the server to whatever is currently running
return server.bindTo(contentWindow)
return server.bindTo(contentWindow, eventHandlerWrap)
}

server = startXhrServer(cy, state, config)
Expand Down
37 changes: 15 additions & 22 deletions packages/driver/src/cy/listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,9 @@ const removeAllListeners = () => {
}

const addListener = (win, event, cb) => {
const id = Math.random()
events.push([win, event, cb])

win.__cypressEventMapRPC = win.__cypressEventMapRPC || {}
win.__cypressEventMapRPC[id] = cb

const fn = new win.Function('evt', `window.__cypressEventMapRPC[${id}].call(this, evt)`)

events.push([win, event, fn])

win.addEventListener(event, fn)
win.addEventListener(event, cb)
}

const eventHasReturnValue = (e) => {
Expand All @@ -47,7 +40,7 @@ const eventHasReturnValue = (e) => {
}

module.exports = {
bindTo (contentWindow, callbacks = {}) {
bindTo (contentWindow, eventHandlerWrap = (fn) => fn, callbacks = {}) {
if (listenersAdded) {
return
}
Expand All @@ -56,30 +49,30 @@ module.exports = {

listenersAdded = true

addListener(contentWindow, 'error', callbacks.onError('error'))
addListener(contentWindow, 'unhandledrejection', callbacks.onError('unhandledrejection'))
addListener(contentWindow, 'error', eventHandlerWrap(callbacks.onError('error')))
addListener(contentWindow, 'unhandledrejection', eventHandlerWrap(callbacks.onError('unhandledrejection')))

addListener(contentWindow, 'beforeunload', (e) => {
addListener(contentWindow, 'beforeunload', eventHandlerWrap((e) => {
// bail if we've canceled this event (from another source)
// or we've set a returnValue on the original event
if (e.defaultPrevented || eventHasReturnValue(e)) {
return
}

callbacks.onBeforeUnload(e)
})
}))

addListener(contentWindow, 'unload', (e) => {
addListener(contentWindow, 'unload', eventHandlerWrap((e) => {
// when we unload we need to remove all of the event listeners
removeAllListeners()

// else we know to proceed onwards!
callbacks.onUnload(e)
})
}))

addListener(contentWindow, 'hashchange', (e) => {
addListener(contentWindow, 'hashchange', eventHandlerWrap((e) => {
callbacks.onNavigation('hashchange', e)
})
}))

for (let attr of HISTORY_ATTRS) {
const orig = contentWindow.history?.[attr]
Expand All @@ -95,7 +88,7 @@ module.exports = {
}
}

addListener(contentWindow, 'submit', (e) => {
addListener(contentWindow, 'submit', eventHandlerWrap((e) => {
// if we've prevented the default submit action
// without stopping propagation, we will still
// receive this event even though the form
Expand All @@ -106,9 +99,9 @@ module.exports = {

// else we know to proceed onwards!
return callbacks.onSubmit(e)
})
}))

contentWindow.alert = callbacks.onAlert
contentWindow.confirm = callbacks.onConfirm
contentWindow.alert = eventHandlerWrap(callbacks.onAlert)
contentWindow.confirm = eventHandlerWrap(callbacks.onConfirm)
},
}
4 changes: 2 additions & 2 deletions packages/driver/src/cypress.js
Original file line number Diff line number Diff line change
Expand Up @@ -492,9 +492,9 @@ class $Cypress {
return this.emit('page:loading', args[0])

case 'app:window:before:load':
this.cy.onBeforeAppWindowLoad(args[0])
this.cy.onBeforeAppWindowLoad(...args)

return this.emit('window:before:load', args[0])
return this.emit('window:before:load', ...args)

case 'app:navigation:changed':
return this.emit('navigation:changed', ...args)
Expand Down
8 changes: 4 additions & 4 deletions packages/driver/src/cypress/cy.js
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,8 @@ const create = function (specWindow, Cypress, Cookies, state, config, log) {
return Cypress.action('app:navigation:changed', `page navigation event (${event})`)
}

const contentWindowListeners = function (contentWindow) {
return $Listeners.bindTo(contentWindow, {
const contentWindowListeners = function (contentWindow, eventHandlerWrap = (fn) => fn) {
return $Listeners.bindTo(contentWindow, eventHandlerWrap, {
// eslint-disable-next-line @cypress/dev/arrow-body-multiline-braces
onError: (handlerType) => (event) => {
const { originalErr, err, promise } = $errUtils.errorFromUncaughtEvent(handlerType, event)
Expand Down Expand Up @@ -1281,15 +1281,15 @@ const create = function (specWindow, Cypress, Cookies, state, config, log) {
return null
},

onBeforeAppWindowLoad (contentWindow) {
onBeforeAppWindowLoad (contentWindow, eventHandlerWrap) {
// we set window / document props before the window load event
// so that we properly handle events coming from the application
// from the time that happens BEFORE the load event occurs
setWindowDocumentProps(contentWindow, state)

urlNavigationEvent('before:load')

contentWindowListeners(contentWindow)
contentWindowListeners(contentWindow, eventHandlerWrap)

wrapNativeMethods(contentWindow)

Expand Down
17 changes: 4 additions & 13 deletions packages/driver/src/cypress/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ const create = (options = {}) => {
return _.extend(options, obj)
},

bindTo (contentWindow) {
bindTo (contentWindow, eventHandlerWrap) {
restore()

const XHR = contentWindow.XMLHttpRequest
Expand Down Expand Up @@ -575,9 +575,9 @@ const create = (options = {}) => {

// bail if eventhandlers have already been called to prevent
// infinite recursion
overrides.onload = addXhrHandler(contentWindow, bailIfRecursive(onLoadFn))
overrides.onerror = addXhrHandler(contentWindow, bailIfRecursive(onErrorFn))
overrides.onreadystatechange = addXhrHandler(contentWindow, bailIfRecursive(onReadyStateFn))
overrides.onload = eventHandlerWrap(bailIfRecursive(onLoadFn))
overrides.onerror = eventHandlerWrap(bailIfRecursive(onErrorFn))
overrides.onreadystatechange = eventHandlerWrap(bailIfRecursive(onReadyStateFn))

props.forEach((prop) => {
// if we currently have one of these properties then
Expand Down Expand Up @@ -659,15 +659,6 @@ const create = (options = {}) => {
return server
}

function addXhrHandler (win, handlerFn) {
const id = Math.random()

win.__cypressEventMapRPC = win.__cypressEventMapRPC || {}
win.__cypressEventMapRPC[id] = handlerFn

return new win.Function('evt', `window.__cypressEventMapRPC[${id}].call(this, evt)`)
}

module.exports = {
defaults,

Expand Down
6 changes: 5 additions & 1 deletion packages/runner/injection/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ Cypress.on('app:timers:pause', timers.pause)

timers.wrap()

Cypress.action('app:window:before:load', window)
Cypress.action('app:window:before:load', window, function eventHandlerWrap (fn) {
return function (e) {
return fn.call(this, e)
}
})
2 changes: 1 addition & 1 deletion scripts/binary/util/testStaticAssets.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const testStaticAssets = async (buildResourcePath) => {
testPackageStaticAssets({
assetGlob: `${buildResourcePath}/packages/runner/dist/injection.js`,
goodStrings: [
'action("app:window:before:load",window)',
'action("app:window:before:load",window,(function',
],
}),
testPackageStaticAssets({
Expand Down