From 043b4a418724ab701fd1a0dc133713417eef5b97 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Mon, 23 Oct 2023 21:09:02 +0200 Subject: [PATCH] FIX: replaces data-tooltip usage by (#24062) As much as possible I would like us to avoid having to go the with a global event listener on click/mouseover. For now I have removed all cases of `data-tooltip`, if we clearly identify a use case of a global event listener we might reconsider this. The following changes are also included: - by default tooltips won't attempt to focus first focusable element anymore - tooltip will now use `cursor: pointer` by default - a new service has been introduced: `InternalTooltip` which is responsible to track the current instance displayed by a ``. Portal elements when replaced are not properly cleaned and I couldn't figure out a way to have a proper hook to ensure the previous `DTooltipInstance` is properly set as not expanded; this problem was very visible when using a tooltip as interactive and hovering another tooltip, which would replace the interactive tooltip as not closed. --- .../admin/addon/components/admin-report.hbs | 37 ++++++++++--------- .../admin/addon/templates/api-keys-new.hbs | 11 +++--- .../admin/addon/templates/api-keys-show.hbs | 11 +++--- .../discourse/app/components/d-modal.hbs | 2 +- .../discourse/app/modifiers/trap-tab.js | 10 +++-- .../components/admin-report-test.js | 10 ++--- .../addon/components/d-float-body.gjs | 2 +- .../float-kit/addon/components/d-tooltip.gjs | 6 ++- .../addon/services/internal-tooltip.js | 12 ++++++ .../app/services/internal-tooltip.js | 1 + .../common/float-kit/d-tooltip.scss | 1 + 11 files changed, 62 insertions(+), 41 deletions(-) create mode 100644 app/assets/javascripts/float-kit/addon/services/internal-tooltip.js create mode 100644 app/assets/javascripts/float-kit/app/services/internal-tooltip.js diff --git a/app/assets/javascripts/admin/addon/components/admin-report.hbs b/app/assets/javascripts/admin/addon/components/admin-report.hbs index 0854431e1fa..7c0171dc81c 100644 --- a/app/assets/javascripts/admin/addon/components/admin-report.hbs +++ b/app/assets/javascripts/admin/addon/components/admin-report.hbs @@ -24,24 +24,27 @@ {{#if this.model.description}} - {{#if this.model.description_link}} - + + <:trigger> {{d-icon "question-circle"}} - - {{else}} - - {{d-icon "question-circle"}} - - {{/if}} + + <:content> + {{#if this.model.description_link}} + + {{this.model.description}} + + {{else}} + {{this.model.description}} + {{/if}} + + {{/if}} {{/unless}} diff --git a/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs b/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs index d616fd390b4..4e0fc952c6c 100644 --- a/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs +++ b/app/assets/javascripts/admin/addon/templates/api-keys-new.hbs @@ -88,16 +88,15 @@
{{act.name}}
- - {{d-icon "question-circle"}} - + /> {{scope.resource}} {{scope.action}} - - {{d-icon "question-circle"}} - + class="scope-tooltip" + />
diff --git a/app/assets/javascripts/discourse/app/modifiers/trap-tab.js b/app/assets/javascripts/discourse/app/modifiers/trap-tab.js index 946584c2572..9f471161d25 100644 --- a/app/assets/javascripts/discourse/app/modifiers/trap-tab.js +++ b/app/assets/javascripts/discourse/app/modifiers/trap-tab.js @@ -13,8 +13,9 @@ export default class TrapTabModifier extends Modifier { registerDestructor(this, (instance) => instance.cleanup()); } - modify(element, [options]) { - this.preventScroll = options?.preventScroll ?? true; + modify(element, _, { preventScroll, autofocus }) { + autofocus ??= true; + this.preventScroll = preventScroll ?? true; this.orignalElement = element; this.element = element.querySelector(".modal-inner-container") || element; this.orignalElement.addEventListener("keydown", this.trapTab); @@ -23,7 +24,10 @@ export default class TrapTabModifier extends Modifier { // and apply manual focus only if we don't have any autofocus element const autofocusedElement = this.element.querySelector("[autofocus]"); - if (!autofocusedElement || document.activeElement !== autofocusedElement) { + if ( + autofocus && + (!autofocusedElement || document.activeElement !== autofocusedElement) + ) { // if there's not autofocus, or the activeElement, is not the autofocusable element // attempt to focus the first of the focusable elements or just the modal-body // to make it possible to scroll with arrow down/up diff --git a/app/assets/javascripts/discourse/tests/integration/components/admin-report-test.js b/app/assets/javascripts/discourse/tests/integration/components/admin-report-test.js index 0b69b15f369..a6bbc66d642 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/admin-report-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/admin-report-test.js @@ -21,11 +21,11 @@ module("Integration | Component | admin-report", function (hooks) { "it has a title" ); - assert.strictEqual( - query(".header .info").getAttribute("data-tooltip"), - "New account registrations for this period", - "it has a description" - ); + await click("[data-trigger]"); + + assert + .dom("[data-content]") + .hasText("New account registrations for this period"); assert.strictEqual( query( diff --git a/app/assets/javascripts/float-kit/addon/components/d-float-body.gjs b/app/assets/javascripts/float-kit/addon/components/d-float-body.gjs index ab6a3a4ac8c..2e22d6f8324 100644 --- a/app/assets/javascripts/float-kit/addon/components/d-float-body.gjs +++ b/app/assets/javascripts/float-kit/addon/components/d-float-body.gjs @@ -62,7 +62,7 @@ export default class DFloatBody extends Component { aria-expanded={{if @instance.expanded "true" "false"}} role={{@role}} {{FloatKitApplyFloatingUi this.trigger this.options @instance}} - {{(if @trapTab (modifier TrapTab))}} + {{(if @trapTab (modifier TrapTab autofocus=false))}} {{(if this.supportsCloseOnClickOutside (modifier FloatKitCloseOnClickOutside this.trigger @instance.close) diff --git a/app/assets/javascripts/float-kit/addon/components/d-tooltip.gjs b/app/assets/javascripts/float-kit/addon/components/d-tooltip.gjs index 116a7113782..7b870e7ed61 100644 --- a/app/assets/javascripts/float-kit/addon/components/d-tooltip.gjs +++ b/app/assets/javascripts/float-kit/addon/components/d-tooltip.gjs @@ -11,6 +11,7 @@ import and from "truth-helpers/helpers/and"; export default class DTooltip extends Component { @service tooltip; + @service internalTooltip; @tracked tooltipInstance = null; @@ -19,8 +20,9 @@ export default class DTooltip extends Component { ...this.args, ...{ listeners: true, - beforeTrigger: () => { - this.tooltip.close(); + beforeTrigger: (instance) => { + this.internalTooltip.activeTooltip?.close?.(); + this.internalTooltip.activeTooltip = instance; }, }, }; diff --git a/app/assets/javascripts/float-kit/addon/services/internal-tooltip.js b/app/assets/javascripts/float-kit/addon/services/internal-tooltip.js new file mode 100644 index 00000000000..d34070b899a --- /dev/null +++ b/app/assets/javascripts/float-kit/addon/services/internal-tooltip.js @@ -0,0 +1,12 @@ +import { tracked } from "@glimmer/tracking"; +import Service from "@ember/service"; + +/* + This service holds the current tooltip displayed when using component. + All of these tooltips share a commong portal outlet element, which means + we have to ensure we close them before their html is replaced, otherwise + we end up with a detached element in the DOM and unexpected behavior. +*/ +export default class InternalTooltip extends Service { + @tracked activeTooltip; +} diff --git a/app/assets/javascripts/float-kit/app/services/internal-tooltip.js b/app/assets/javascripts/float-kit/app/services/internal-tooltip.js new file mode 100644 index 00000000000..f5e9efb2260 --- /dev/null +++ b/app/assets/javascripts/float-kit/app/services/internal-tooltip.js @@ -0,0 +1 @@ +export { default } from "float-kit/services/internal-tooltip"; diff --git a/app/assets/stylesheets/common/float-kit/d-tooltip.scss b/app/assets/stylesheets/common/float-kit/d-tooltip.scss index 355e9afdd61..ea35fb4825a 100644 --- a/app/assets/stylesheets/common/float-kit/d-tooltip.scss +++ b/app/assets/stylesheets/common/float-kit/d-tooltip.scss @@ -23,6 +23,7 @@ &__trigger { display: inline-flex; + cursor: pointer; .touch & { @include unselectable;