From 9659ad63349d5123ca2bd2548a43e37d5ee817e7 Mon Sep 17 00:00:00 2001 From: Brandy Carney Date: Wed, 4 Nov 2020 16:19:38 -0500 Subject: [PATCH] fix(item): only add click event listener to items with inputs (#22352) This stops screen readers, such as NVDA, from reading every item as clickable even when it is text only. fixes #22011 --- core/src/components/alert/alert.tsx | 5 +-- core/src/components/datetime/datetime.tsx | 2 +- core/src/components/item/item.tsx | 42 ++++++++++++++++--- .../components/item/test/inputs/index.html | 13 ++++++ .../components/radio-group/radio-group.tsx | 2 +- core/src/utils/overlays.ts | 2 +- packages/react-router/package.json | 3 +- packages/react/package.json | 3 +- 8 files changed, 58 insertions(+), 14 deletions(-) diff --git a/core/src/components/alert/alert.tsx b/core/src/components/alert/alert.tsx index 89e1468477..bfce34f037 100644 --- a/core/src/components/alert/alert.tsx +++ b/core/src/components/alert/alert.tsx @@ -139,8 +139,7 @@ export class Alert implements ComponentInterface, OverlayInterface { if ( !inputTypes.has('radio') || (ev.target && !this.el.contains(ev.target)) - || ev.target.classList.contains('alert-button')) - { + || ev.target.classList.contains('alert-button')) { return; } @@ -170,7 +169,7 @@ export class Alert implements ComponentInterface, OverlayInterface { if (['ArrowUp', 'ArrowLeft'].includes(ev.key)) { nextEl = (index === 0) ? radios[radios.length - 1] - : radios[index - 1] + : radios[index - 1]; } if (nextEl && radios.includes(nextEl)) { diff --git a/core/src/components/datetime/datetime.tsx b/core/src/components/datetime/datetime.tsx index 0b8788aee9..eb6a1e9d09 100644 --- a/core/src/components/datetime/datetime.tsx +++ b/core/src/components/datetime/datetime.tsx @@ -285,7 +285,7 @@ export class Datetime implements ComponentInterface { if (data.name !== 'ampm' && this.datetimeValue.ampm !== undefined) { changeData['ampm'] = { value: this.datetimeValue.ampm - } + }; } this.updateDatetimeValue(changeData); diff --git a/core/src/components/item/item.tsx b/core/src/components/item/item.tsx index 179c34d845..67a32dab0d 100644 --- a/core/src/components/item/item.tsx +++ b/core/src/components/item/item.tsx @@ -30,6 +30,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac private labelColorStyles = {}; private itemStyles = new Map(); + private clickListener?: (ev: Event) => void; @Element() el!: HTMLIonItemElement; @@ -152,7 +153,33 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac } } + componentDidUpdate() { + // Do not use @Listen here to avoid making all items + // appear as clickable to screen readers + // https://github.com/ionic-team/ionic-framework/issues/22011 + const input = this.getFirstInput(); + if (input && !this.clickListener) { + this.clickListener = (ev: Event) => this.delegateFocus(ev, input); + this.el.addEventListener('click', this.clickListener); + } + } + + disconnectedCallback() { + const input = this.getFirstInput(); + if (input && this.clickListener) { + this.el.removeEventListener('click', this.clickListener); + this.clickListener = undefined; + } + } + componentDidLoad() { + this.setMultipleInputs(); + } + + // If the item contains multiple clickable elements and/or inputs, then the item + // should not have a clickable input cover over the entire item to prevent + // interfering with their individual click events + private setMultipleInputs() { // The following elements have a clickable cover that is relative to the entire item const covers = this.el.querySelectorAll('ion-checkbox, ion-datetime, ion-select, ion-radio'); @@ -199,22 +226,20 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac // clicking on the left padding of an item is not focusing the input // but is opening the keyboard. It will no longer be needed with // iOS 14. - @Listen('click') - delegateFocus(ev: Event) { + private delegateFocus(ev: Event, input: HTMLIonInputElement | HTMLIonTextareaElement) { const clickedItem = (ev.target as HTMLElement).tagName === 'ION-ITEM'; - const input = this.getFirstInput(); let firstActive = false; // If the first input is the same as the active element we need // to focus the first input again, but if the active element // is another input inside of the item we shouldn't switch focus - if (input && document.activeElement) { + if (document.activeElement) { firstActive = input.querySelector('input, textarea') === document.activeElement; } // Only focus the first input if we clicked on an ion-item // and the first input exists - if (clickedItem && input && firstActive) { + if (clickedItem && firstActive) { input.fireFocusEvents = false; input.setBlur(); input.setFocus(); @@ -239,6 +264,11 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac rel, target }; + // Only set onClick if the item is clickable to prevent screen + // readers from reading all items as clickable + const clickFn = clickable ? { + onClick: (ev: Event) => {openURL(href, ev, routerDirection, routerAnimation); } + } : {}; const showDetail = detail !== undefined ? detail : mode === 'ios' && clickable; this.itemStyles.forEach(value => { Object.assign(childStyles, value); @@ -267,7 +297,7 @@ export class Item implements ComponentInterface, AnchorInterface, ButtonInterfac class="item-native" part="native" disabled={disabled} - onClick={(ev: Event) => openURL(href, ev, routerDirection, routerAnimation)} + {...clickFn} >
diff --git a/core/src/components/item/test/inputs/index.html b/core/src/components/item/test/inputs/index.html index 4803f223a8..fa73204270 100644 --- a/core/src/components/item/test/inputs/index.html +++ b/core/src/components/item/test/inputs/index.html @@ -123,6 +123,10 @@ + + Delayed input + + Multiple inputs @@ -200,6 +204,15 @@ clickableItem.color = color === undefined ? 'primary' : undefined; }); + const delayedInputItem = document.querySelector('#delayedInputItem'); + const delayedInput = document.createElement('ion-input'); + delayedInput.type = 'number'; + delayedInput.value = 34; + + setTimeout(() => { + delayedInputItem.appendChild(delayedInput); + }, 3000); + const inputs = document.querySelectorAll('ion-input, ion-textarea'); for (var i = 0; i < inputs.length; i++) { diff --git a/core/src/components/radio-group/radio-group.tsx b/core/src/components/radio-group/radio-group.tsx index 17e2980662..33ee100074 100644 --- a/core/src/components/radio-group/radio-group.tsx +++ b/core/src/components/radio-group/radio-group.tsx @@ -126,7 +126,7 @@ export class RadioGroup implements ComponentInterface { if (['ArrowUp', 'ArrowLeft'].includes(ev.key)) { next = (index === 0) ? radios[radios.length - 1] - : radios[index - 1] + : radios[index - 1]; } if (next && radios.includes(next)) { diff --git a/core/src/utils/overlays.ts b/core/src/utils/overlays.ts index ef1b737bc9..f61cacd2e5 100644 --- a/core/src/utils/overlays.ts +++ b/core/src/utils/overlays.ts @@ -299,7 +299,7 @@ const focusPreviousElementOnDismiss = async (overlayEl: any) => { await overlayEl.onDidDismiss(); previousElement.focus(); -} +}; export const dismiss = async ( overlay: OverlayInterface, diff --git a/packages/react-router/package.json b/packages/react-router/package.json index 852fbb0f28..7f52f2e884 100644 --- a/packages/react-router/package.json +++ b/packages/react-router/package.json @@ -21,7 +21,7 @@ }, "scripts": { "build": "npm run clean && npm run compile", - "clean": "rm -rf dist dist-transpiled", + "clean": "rimraf dist dist-transpiled", "compile": "npm run tsc && rollup -c", "release": "np --any-branch --no-cleanup", "lint": "tslint --project .", @@ -65,6 +65,7 @@ "react-dom": "^16.9.0", "react-router": "^5.0.1", "react-router-dom": "^5.0.1", + "rimraf": "^3.0.2", "rollup": "^2.26.4", "rollup-plugin-sourcemaps": "^0.6.2", "ts-jest": "^26.1.1", diff --git a/packages/react/package.json b/packages/react/package.json index a98b6b9642..64df0dd1fe 100644 --- a/packages/react/package.json +++ b/packages/react/package.json @@ -21,7 +21,7 @@ }, "scripts": { "build": "npm run clean && npm run copy && npm run compile", - "clean": "rm -rf dist && rm -rf dist-transpiled && rm -rf routing", + "clean": "rimraf dist && rimraf dist-transpiled && rimraf routing", "compile": "npm run tsc && rollup -c", "release": "np --any-branch --yolo --no-release-draft", "lint": "tslint --project .", @@ -61,6 +61,7 @@ "np": "^6.4.0", "react": "^16.9.0", "react-dom": "^16.9.0", + "rimraf": "^3.0.2", "rollup": "^2.26.4", "rollup-plugin-sourcemaps": "^0.6.2", "ts-jest": "^26.1.1",