From ff05dbe50d7ccce020f0779c6fbf3ce6578745ef Mon Sep 17 00:00:00 2001 From: Chris Thielen Date: Tue, 22 Dec 2020 12:03:10 -0800 Subject: [PATCH] fix(sref): do not handle clicks if the dom element has a 'target' attribute Also do not handle clicks if shift or alt is being held down. Closes #786 --- src/components/__tests__/UISref.test.tsx | 41 ++++++++++++- src/hooks/__tests__/useSref.test.tsx | 74 +++++++++++++++++------- src/hooks/useSref.ts | 6 +- 3 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/components/__tests__/UISref.test.tsx b/src/components/__tests__/UISref.test.tsx index 8badeaa4..9929626f 100644 --- a/src/components/__tests__/UISref.test.tsx +++ b/src/components/__tests__/UISref.test.tsx @@ -94,7 +94,7 @@ describe('', () => { it('calls the child elements onClick function and honors e.preventDefault()', async () => { const goSpy = jest.spyOn(router.stateService, 'go'); - const onClickSpy = jest.fn(e => e.preventDefault()); + const onClickSpy = jest.fn((e) => e.preventDefault()); const wrapper = mountInRouter( state2 @@ -106,7 +106,7 @@ describe('', () => { expect(goSpy).not.toHaveBeenCalled(); }); - it("doesn't trigger a transition when middle-clicked/ctrl+clicked", async () => { + it("doesn't trigger a transition when middle-clicked", async () => { const stateServiceGoSpy = jest.spyOn(router.stateService, 'go'); const wrapper = mountInRouter( @@ -119,9 +119,44 @@ describe('', () => { expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); link.simulate('click', { button: 1 }); - link.simulate('click', { metaKey: true }); + expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); + }); + + it("doesn't trigger a transition when ctrl/meta/shift/alt+clicked", async () => { + const stateServiceGoSpy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter( + + state2 + + ); + + const link = wrapper.find('a'); + link.simulate('click'); + expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); + link.simulate('click', { ctrlKey: true }); + expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); + link.simulate('click', { metaKey: true }); + expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); + + link.simulate('click', { shiftKey: true }); + expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); + + link.simulate('click', { altKey: true }); expect(stateServiceGoSpy).toHaveBeenCalledTimes(1); }); + + it("doesn't trigger a transition when the anchor has a 'target' attribute", async () => { + const stateServiceGoSpy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter( + + state2 + + ); + + const link = wrapper.find('a'); + link.simulate('click'); + expect(stateServiceGoSpy).not.toHaveBeenCalled(); + }); }); diff --git a/src/hooks/__tests__/useSref.test.tsx b/src/hooks/__tests__/useSref.test.tsx index 48c37a43..c7d9239e 100644 --- a/src/hooks/__tests__/useSref.test.tsx +++ b/src/hooks/__tests__/useSref.test.tsx @@ -10,12 +10,16 @@ const state = { name: 'state', url: '/state' }; const state2 = { name: 'state2', url: '/state2' }; const state3 = { name: 'state3', url: '/state3/:param' }; -const Link = ({ to, params = undefined, children = undefined }) => { +const Link = ({ to, params = undefined, children = undefined, target = undefined }) => { const sref = useSref(to, params); - return {children}; + return ( + + {children} + + ); }; -describe('useUiSref', () => { +describe('useSref', () => { let { router, routerGo, mountInRouter } = makeTestRouter([]); beforeEach(() => ({ router, routerGo, mountInRouter } = makeTestRouter([state, state2, state3]))); @@ -29,21 +33,51 @@ describe('useUiSref', () => { expect(wrapper.html()).toBe('state2'); }); - it('returns an onClick function which activates the target state', () => { - const spy = jest.spyOn(router.stateService, 'go'); - const wrapper = mountInRouter(); - wrapper.simulate('click'); + describe('onClick function', () => { + it('activates the target state', () => { + const spy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter(); + wrapper.simulate('click'); - expect(spy).toBeCalledTimes(1); - expect(spy).toBeCalledWith('state', expect.anything(), expect.anything()); - }); + expect(spy).toBeCalledTimes(1); + expect(spy).toBeCalledWith('state', expect.anything(), expect.anything()); + }); - it('returns an onClick function which respects defaultPrevented', () => { - const spy = jest.spyOn(router.stateService, 'go'); - const wrapper = mountInRouter(); - wrapper.simulate('click', { defaultPrevented: true }); + it('respects defaultPrevented', () => { + const spy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter(); + wrapper.simulate('click', { defaultPrevented: true }); + + expect(spy).not.toHaveBeenCalled(); + }); + + it('does not get called when middle or right clicked', () => { + const spy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter(); - expect(spy).not.toHaveBeenCalled(); + wrapper.simulate('click', { button: 1 }); + expect(spy).not.toHaveBeenCalled(); + }); + + it('does not get called if any of these modifiers are present: ctrl, meta, shift, alt', () => { + const spy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter(); + + wrapper.simulate('click', { ctrlKey: true }); + wrapper.simulate('click', { metaKey: true }); + wrapper.simulate('click', { shiftKey: true }); + wrapper.simulate('click', { altKey: true }); + + expect(spy).not.toHaveBeenCalled(); + }); + + it('does not get called if the underlying DOM element has a "target" attribute', () => { + const spy = jest.spyOn(router.stateService, 'go'); + const wrapper = mountInRouter(); + + wrapper.simulate('click'); + expect(spy).not.toHaveBeenCalled(); + }); }); it('updates the href when the stateName changes', () => { @@ -96,7 +130,7 @@ describe('useUiSref', () => { it('calls go() with the actual string provided (not with the name of the matched future state)', async () => { router.stateRegistry.register({ name: 'future.**', url: '/future' }); - const Link = props => { + const Link = (props) => { const sref = useSref('future.child', { param: props.param }); return ; }; @@ -112,7 +146,7 @@ describe('useUiSref', () => { it('participates in parent UISrefActive component active state', async () => { await routerGo('state2'); - const State2Link = props => { + const State2Link = (props) => { const sref = useSref('state2'); return ( @@ -150,7 +184,7 @@ describe('useUiSref', () => { it('participates in grandparent UISrefActive component active state', async () => { await routerGo('state2'); - const State2Link = props => { + const State2Link = (props) => { const sref = useSref('state2'); return ( @@ -178,7 +212,7 @@ describe('useUiSref', () => { it('stops participating in parent/grandparent UISrefActive component active state when unmounted', async () => { await routerGo('state2'); - const State2Link = props => { + const State2Link = (props) => { const sref = useSref('state2'); return ( @@ -187,7 +221,7 @@ describe('useUiSref', () => { ); }; - const Component = props => { + const Component = (props) => { return (
diff --git a/src/hooks/useSref.ts b/src/hooks/useSref.ts index 0421ff2d..0b4084b9 100644 --- a/src/hooks/useSref.ts +++ b/src/hooks/useSref.ts @@ -1,7 +1,7 @@ /** @packageDocumentation @reactapi @module react_hooks */ import * as React from 'react'; -import { useCallback, useContext, useEffect, useMemo, useState } from 'react'; +import { ReactHTMLElement, useCallback, useContext, useEffect, useMemo, useState } from 'react'; import { isString, StateDeclaration, TransitionOptions, UIRouter } from '@uirouter/core'; import { UISrefActiveContext } from '../components'; import { useDeepObjectDiff } from './useDeepObjectDiff'; @@ -84,7 +84,9 @@ export function useSref(stateName: string, params: object = {}, options: Transit const onClick = useCallback( (e: React.MouseEvent) => { - if (!e.defaultPrevented && !(e.button == 1 || e.metaKey || e.ctrlKey)) { + const targetAttr = (e.target as any)?.attributes?.target; + const modifierKey = e.button >= 1 || e.ctrlKey || e.metaKey || e.shiftKey || e.altKey; + if (!e.defaultPrevented && targetAttr == null && !modifierKey) { e.preventDefault(); router.stateService.go(stateName, paramsMemo, optionsMemo); }