From bb80285465bbe150ac216584d1d5d781a5f4b7a0 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 12 May 2022 12:33:49 +0100 Subject: [PATCH 1/7] Use FocusEvent API to deactivate content To avoid focus trapping and preserve the requirement that a click (or tap or tab) outside of the disclosure will close/deactivate the content --- .../addon/components/hds/disclosure/index.hbs | 16 ++++---- .../addon/components/hds/disclosure/index.js | 39 +++++++++---------- .../app/templates/utilities/disclosure.hbs | 31 +++++---------- 3 files changed, 34 insertions(+), 52 deletions(-) diff --git a/packages/components/addon/components/hds/disclosure/index.hbs b/packages/components/addon/components/hds/disclosure/index.hbs index 304827bc67..7bb2e8d08f 100644 --- a/packages/components/addon/components/hds/disclosure/index.hbs +++ b/packages/components/addon/components/hds/disclosure/index.hbs @@ -1,16 +1,14 @@ -
+
{{yield (hash onClickToggle=this.onClickToggle isActive=this.isActive) to="toggle"}}
{{#if this.isActive}} -
+
{{yield to="content"}}
{{/if}} diff --git a/packages/components/addon/components/hds/disclosure/index.js b/packages/components/addon/components/hds/disclosure/index.js index a3f5af9089..3aa402f218 100644 --- a/packages/components/addon/components/hds/disclosure/index.js +++ b/packages/components/addon/components/hds/disclosure/index.js @@ -7,6 +7,11 @@ export default class HdsDisclosureComponent extends Component { @tracked toggleRef; @tracked isToggleClicked; + @action + didInsert(element) { + this.element = element; + } + @action onClickToggle(event) { // we store a reference to the DOM node that has the "onClickToggle" event associated with it @@ -17,31 +22,23 @@ export default class HdsDisclosureComponent extends Component { } @action - clickOutsideDeactivates(event) { - // we check if the toggle reference belongs to the tree of parent DOM nodes - // of the element that was clicked and triggered the "click outside" event handling - // notice: we use "event.composedPath" here because is now fully supported (see https://caniuse.com/?search=event%20path) - const path = event.composedPath(); - this.isToggleClicked = path.includes(this.toggleRef); - // here we need to return `true` to make sure that the focus trap will be deactivated (and allow the click event to do its thing (i.e. to pass-through to the element that was clicked). - // see: https://github.com/focus-trap/focus-trap#createoptions - return true; + onFocusOut(event) { + if ( + !event.relatedTarget || // click or tap a non-related target (e.g. outside the element) + !this.element.contains(event.relatedTarget) // move focus to a target outside the element + ) { + this.deactivate(); + } } @action - onDeactivate() { - // on deactivate we hide the content, except for the case when the button has been clicked - // the reason is that the "onClickToggle" is called in any case (there's no way to block the event) - // so when the user clicks the toggle to close the panel, we let the "onClickToggle" handle the closure - // otherwise we would have two changes of status, this and the toggle, and the panel would remain open - if (!this.isToggleClicked) { + deactivate() { + if (this.isActive) { this.isActive = false; - // we need to reset this check - this.isToggleClicked = false; - // we call the "onClose" callback if it exists (and is a function) - if (this.args.onClose && typeof this.args.onClose === 'function') { - this.args.onClose(); - } + } + // we call the "onClose" callback if it exists (and is a function) + if (this.args.onClose && typeof this.args.onClose === 'function') { + this.args.onClose(); } } } diff --git a/packages/components/tests/dummy/app/templates/utilities/disclosure.hbs b/packages/components/tests/dummy/app/templates/utilities/disclosure.hbs index 8d17811ceb..04e774e30c 100644 --- a/packages/components/tests/dummy/app/templates/utilities/disclosure.hbs +++ b/packages/components/tests/dummy/app/templates/utilities/disclosure.hbs @@ -85,25 +85,14 @@ enter/return), clicking outside of the content, or via the esc key.

-

Important:

-
    -
  • - the - HDS::Disclosure - component will trap the focus inside the "content" while it's open (we're using the - - - focus-trap - - library); at the moment we don't allow non-focusable content (it will trigger an error). -
    • -
  • the "content" is not positioned in any way in relation to the toggle: this - responsibility is left to the consumers (eg by applying a - position: absolute - to a wrapper around the content that is passed to the - :content - block).
    • -
+

+ Important: + The "content" is not positioned in any way in relation to the toggle: this responsibility is left to the consumers + (eg by applying a + position: absolute + to a wrapper around the content that is passed to the + :content + block).

@@ -123,9 +112,7 @@ <:content> - - - +
From cf95c67209275f766598a0cce3d16fb1f78bac39 Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 12 May 2022 12:35:43 +0100 Subject: [PATCH 2/7] Allow deactivation using Escape key Preserve the requirements that Escape key should close/deactivate the content and when used, focus should return to button/toggle. --- .../components/addon/components/hds/disclosure/index.hbs | 1 + .../components/addon/components/hds/disclosure/index.js | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/packages/components/addon/components/hds/disclosure/index.hbs b/packages/components/addon/components/hds/disclosure/index.hbs index 7bb2e8d08f..9335c6c772 100644 --- a/packages/components/addon/components/hds/disclosure/index.hbs +++ b/packages/components/addon/components/hds/disclosure/index.hbs @@ -3,6 +3,7 @@ ...attributes {{did-insert this.didInsert}} {{on "focusout" this.onFocusOut}} + {{on "keyup" this.onKeyUp}} >
{{yield (hash onClickToggle=this.onClickToggle isActive=this.isActive) to="toggle"}} diff --git a/packages/components/addon/components/hds/disclosure/index.js b/packages/components/addon/components/hds/disclosure/index.js index 3aa402f218..6ca7be2e07 100644 --- a/packages/components/addon/components/hds/disclosure/index.js +++ b/packages/components/addon/components/hds/disclosure/index.js @@ -31,6 +31,14 @@ export default class HdsDisclosureComponent extends Component { } } + @action + onKeyUp(event) { + if (event.key === 'Escape') { + this.deactivate(); + this.toggleRef.focus(); + } + } + @action deactivate() { if (this.isActive) { From 5bddea752ebc0523c40d4b93a7cedc0538496c3e Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 12 May 2022 13:05:28 +0100 Subject: [PATCH 3/7] Remove ember-focus-trap --- packages/components/package.json | 1 - .../components/hds/disclosure/index-test.js | 9 -------- yarn.lock | 22 +------------------ 3 files changed, 1 insertion(+), 31 deletions(-) diff --git a/packages/components/package.json b/packages/components/package.json index 78b3baf2fc..b7a690d987 100644 --- a/packages/components/package.json +++ b/packages/components/package.json @@ -40,7 +40,6 @@ "ember-cli-babel": "^7.26.11", "ember-cli-htmlbars": "^6.0.1", "ember-cli-sass": "^10.0.1", - "ember-focus-trap": "^1.0.1", "ember-keyboard": "^8.1.0", "ember-named-blocks-polyfill": "^0.2.5", "sass": "^1.43.4" diff --git a/packages/components/tests/integration/components/hds/disclosure/index-test.js b/packages/components/tests/integration/components/hds/disclosure/index-test.js index 0fedca8a29..ef7e7a250b 100644 --- a/packages/components/tests/integration/components/hds/disclosure/index-test.js +++ b/packages/components/tests/integration/components/hds/disclosure/index-test.js @@ -9,13 +9,6 @@ import { } from '@ember/test-helpers'; import { hbs } from 'ember-cli-htmlbars'; -// we need to wait for the next event loop cycle after clicking the "toggle" -// otherwise the test don't work (surely related to the focus-trap 'delayInitialFocus' parameter) -// see https://github.com/focus-trap/focus-trap#createoptions -// notice: you can use console.log(document.activeElement) to check which element has focus -const waitNextExecutionFrame = () => - new Promise((resolve) => setTimeout(resolve, 100)); - module('Integration | Component | hds/disclosure/index', function (hooks) { setupRenderingTest(hooks); @@ -73,7 +66,6 @@ module('Integration | Component | hds/disclosure/index', function (hooks) { `); await triggerKeyEvent('button#test-disclosure-button', 'keydown', 'Enter'); - // await waitNextExecutionFrame(); // await waitFor('.hds-disclosure__content', { timeout: 2000 }); assert.dom('.hds-disclosure__content').exists(); assert.dom('a#test-disclosure-link').exists(); @@ -100,7 +92,6 @@ module('Integration | Component | hds/disclosure/index', function (hooks) { `); await click('button#test-disclosure-button'); - await waitNextExecutionFrame(); // console.log('BBB', document.activeElement); assert.dom('a#test-disclosure-link-1').isFocused(); await triggerKeyEvent('a#test-disclosure-link-1', 'keydown', 'Tab'); diff --git a/yarn.lock b/yarn.lock index 4535d26378..b588c7e9c2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1383,7 +1383,7 @@ ember-cli-version-checker "^5.1.2" semver "^7.3.5" -"@embroider/addon-shim@^1.0.0", "@embroider/addon-shim@^1.5.0": +"@embroider/addon-shim@^1.5.0": version "1.5.0" resolved "https://registry.yarnpkg.com/@embroider/addon-shim/-/addon-shim-1.5.0.tgz#639b8b394336a5ae26dd3e24ffc3d34d864ac5ce" integrity sha512-5zgwA/wTYjgn2Oo6hKRQhF/5Gnwb+hGhj/WXhZQa5yA7fRRdBV1tVMS7b7SLawZcmOhuWkyPwFdgsYtGBvDB0w== @@ -6568,14 +6568,6 @@ ember-fetch@^8.1.1: node-fetch "^2.6.1" whatwg-fetch "^3.6.2" -ember-focus-trap@^1.0.1: - version "1.0.1" - resolved "https://registry.yarnpkg.com/ember-focus-trap/-/ember-focus-trap-1.0.1.tgz#a99565f6ce55d500b92a0965e79e3ad04219f157" - integrity sha512-ZUyq5ZkIuXp+ng9rCMkqBh36/V95PltL7iljStkma4+651xlAy3Z84L9WOu/uOJyVpNUxii8RJBbAySHV6c+RQ== - dependencies: - "@embroider/addon-shim" "^1.0.0" - focus-trap "^6.7.1" - ember-keyboard@^8.1.0: version "8.1.1" resolved "https://registry.yarnpkg.com/ember-keyboard/-/ember-keyboard-8.1.1.tgz#b12844a0f87cbb091b7c21293afccbf5aedc2ea3" @@ -7879,13 +7871,6 @@ flush-write-stream@^1.0.0: inherits "^2.0.3" readable-stream "^2.3.6" -focus-trap@^6.7.1: - version "6.7.3" - resolved "https://registry.yarnpkg.com/focus-trap/-/focus-trap-6.7.3.tgz#b5dc195b49c90001f08a63134471d1e6dd381ddd" - integrity sha512-8xCEKndV4KrseGhFKKKmczVA14yx1/hnmFICPOjcFjToxCJYj/NHH43tPc3YE/PLnLRNZoFug0EcWkGQde/miQ== - dependencies: - tabbable "^5.2.1" - follow-redirects@^1.0.0, follow-redirects@^1.14.0: version "1.14.9" resolved "https://registry.yarnpkg.com/follow-redirects/-/follow-redirects-1.14.9.tgz#dd4ea157de7bfaf9ea9b3fbd85aa16951f78d8d7" @@ -13260,11 +13245,6 @@ sync-disk-cache@^2.0.0: rimraf "^3.0.0" username-sync "^1.0.2" -tabbable@^5.2.1: - version "5.2.1" - resolved "https://registry.yarnpkg.com/tabbable/-/tabbable-5.2.1.tgz#e3fda7367ddbb172dcda9f871c0fdb36d1c4cd9c" - integrity sha512-40pEZ2mhjaZzK0BnI+QGNjJO8UYx9pP5v7BGe17SORTO0OEuuaAwQTkAp8whcZvqon44wKFOikD+Al11K3JICQ== - table@^6.0.9: version "6.8.0" resolved "https://registry.yarnpkg.com/table/-/table-6.8.0.tgz#87e28f14fa4321c3377ba286f07b79b281a3b3ca" From 3db16fcb4834cbe441397ec21199377e894fa75a Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 12 May 2022 13:15:18 +0100 Subject: [PATCH 4/7] Allow interactive events on our disclosure template Our linter doesn't allow interactive events on standard elements complaining about 'keyup' being used on the main component element. As our disclosure component is interactive (jn a sense that expands and collapses a container) we need to add this exception to the template. --- packages/components/addon/components/hds/disclosure/index.hbs | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/addon/components/hds/disclosure/index.hbs b/packages/components/addon/components/hds/disclosure/index.hbs index 9335c6c772..3b4bfc63e9 100644 --- a/packages/components/addon/components/hds/disclosure/index.hbs +++ b/packages/components/addon/components/hds/disclosure/index.hbs @@ -1,3 +1,4 @@ +{{! template-lint-disable no-invalid-interactive }}
Date: Fri, 13 May 2022 11:36:19 +0100 Subject: [PATCH 5/7] Replace skipped disclosure tests We explicitly test the functionality we introduced with this component, which is handling the escape key and the focus out. As a note, these two tests could be grouped under a 'when disclosure content is visible', but I would avoid introducing nested tests as we don't seem to have them anywhere else and we don't have many tests for this component anyway. --- .../components/hds/disclosure/index-test.js | 49 +++++++++---------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/packages/components/tests/integration/components/hds/disclosure/index-test.js b/packages/components/tests/integration/components/hds/disclosure/index-test.js index ef7e7a250b..cf043d84a5 100644 --- a/packages/components/tests/integration/components/hds/disclosure/index-test.js +++ b/packages/components/tests/integration/components/hds/disclosure/index-test.js @@ -1,9 +1,9 @@ -import { module, test, skip } from 'qunit'; +import { module, test } from 'qunit'; import { setupRenderingTest } from 'ember-qunit'; import { click, + triggerEvent, triggerKeyEvent, - // waitFor, render, resetOnerror, } from '@ember/test-helpers'; @@ -52,11 +52,13 @@ module('Integration | Component | hds/disclosure/index', function (hooks) { assert.dom('.hds-disclosure__content').exists(); assert.dom('a#test-disclosure-link').exists(); }); - // TODO this doesn't work - skip('it should render the "content" when the "toggle" is activated via "enter"', async function (assert) { - assert.expect(2); + + // ESCAPE KEY + + test('it should hide the "content" when the "toggle" is deactivated via "Escape"', async function (assert) { + assert.expect(4); await render(hbs` - + <:toggle as |t|>