From 28be5d303716134a76ffef1c846bcda465101208 Mon Sep 17 00:00:00 2001 From: Dan Gebhardt Date: Mon, 24 Oct 2022 11:06:11 -0400 Subject: [PATCH] DEV: Normalize event handling to improve Glimmer + Classic component compat (#18490) Classic Ember components (i.e. "@ember/component") rely upon "event delegation" to listen for events at the application root and then dispatch those events to any event handlers defined on individual Classic components. This coordination is handled by Ember's EventDispatcher. In contrast, Glimmer components (i.e. "@glimmer/component") expect event listeners to be added to elements using modifiers (such as `{{on "click"}}`). These event listeners are added directly to DOM elements using `addEventListener`. There is no need for an event dispatcher. Issues may arise when using Classic and Glimmer components together, since it requires reconciling the two event handling approaches. For instance, event propagation may not work as expected when a Classic component is nested inside a Glimmer component. `normalizeEmberEventHandling` helps an application standardize upon the Glimmer event handling approach by eliminating usage of event delegation and instead rewiring Classic components to directly use `addEventListener`. Specifically, it performs the following: - Invokes `eliminateClassicEventDelegation()` to remove all events associated with Ember's EventDispatcher to reduce its runtime overhead and ensure that it is effectively not in use. - Invokes `rewireClassicComponentEvents(app)` to rewire each Classic component to add its own event listeners for standard event handlers (e.g. `click`, `mouseDown`, `submit`, etc.). - Configures an instance initializer that invokes `rewireActionModifier(appInstance)` to redefine the `action` modifier with a substitute that uses `addEventListener`. Additional changes include: * d-button: only preventDefault / stopPropagation for handled actions This allows unhandled events to propagate as expected. * d-editor: avoid adding duplicate event listener for tests This extra event listener causes duplicate paste events in tests. * group-manage-email-settings: Monitor `input` instead of `change` event for checkboxes --- app/assets/javascripts/discourse/app/app.js | 5 + .../discourse/app/components/d-button.js | 59 ++-- .../discourse/app/components/d-editor.js | 4 - .../app/initializers/ember-events.js | 46 ---- .../app/lib/ember-action-modifier.js | 92 +++++++ .../discourse/app/lib/ember-events.js | 241 +++++++++++++++++ .../group-manage-email-settings.hbs | 4 +- .../unit/lib/ember-action-modifer-test.js | 76 ++++++ .../tests/unit/lib/ember-events-test.js | 254 ++++++++++++++++++ 9 files changed, 701 insertions(+), 80 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/initializers/ember-events.js create mode 100644 app/assets/javascripts/discourse/app/lib/ember-action-modifier.js create mode 100644 app/assets/javascripts/discourse/app/lib/ember-events.js create mode 100644 app/assets/javascripts/discourse/tests/unit/lib/ember-action-modifer-test.js create mode 100644 app/assets/javascripts/discourse/tests/unit/lib/ember-events-test.js diff --git a/app/assets/javascripts/discourse/app/app.js b/app/assets/javascripts/discourse/app/app.js index 8d5d9d8ea3a..6b2bd6af253 100644 --- a/app/assets/javascripts/discourse/app/app.js +++ b/app/assets/javascripts/discourse/app/app.js @@ -1,6 +1,7 @@ import Application from "@ember/application"; import { buildResolver } from "discourse-common/resolver"; import { isTesting } from "discourse-common/config/environment"; +import { normalizeEmberEventHandling } from "./lib/ember-events"; const _pluginCallbacks = []; let _unhandledThemeErrors = []; @@ -54,6 +55,10 @@ const Discourse = Application.extend({ start() { document.querySelector("noscript")?.remove(); + // Rewire event handling to eliminate event delegation for better compat + // between Glimmer and Classic components. + normalizeEmberEventHandling(this); + if (Error.stackTraceLimit) { // We need Errors to have full stack traces for `lib/source-identifier` Error.stackTraceLimit = Infinity; diff --git a/app/assets/javascripts/discourse/app/components/d-button.js b/app/assets/javascripts/discourse/app/components/d-button.js index 1726bb76d84..b2f8d2ccb10 100644 --- a/app/assets/javascripts/discourse/app/components/d-button.js +++ b/app/assets/javascripts/discourse/app/components/d-button.js @@ -118,8 +118,7 @@ export default Component.extend({ }, click(event) { - this._triggerAction(event); - return false; + return this._triggerAction(event); }, mouseDown(event) { @@ -129,37 +128,41 @@ export default Component.extend({ }, _triggerAction(event) { - let { action } = this; + let { action, route, href } = this; - if (action) { - if (typeof action === "string") { - // Note: This is deprecated in new Embers and needs to be removed in the future. - // There is already a warning in the console. - this.sendAction("action", this.actionParam); - } else if (typeof action === "object" && action.value) { - if (this.forwardEvent) { - action.value(this.actionParam, event); - } else { - action.value(this.actionParam); - } - } else if (typeof this.action === "function") { - if (this.forwardEvent) { - action(this.actionParam, event); - } else { - action(this.actionParam); + if (action || route || href?.length) { + if (action) { + if (typeof action === "string") { + // Note: This is deprecated in new Embers and needs to be removed in the future. + // There is already a warning in the console. + this.sendAction("action", this.actionParam); + } else if (typeof action === "object" && action.value) { + if (this.forwardEvent) { + action.value(this.actionParam, event); + } else { + action.value(this.actionParam); + } + } else if (typeof this.action === "function") { + if (this.forwardEvent) { + action(this.actionParam, event); + } else { + action(this.actionParam); + } } } - } - if (this.route) { - this.router.transitionTo(this.route); - } + if (route) { + this.router.transitionTo(route); + } - if (this.href && this.href.length) { - DiscourseURL.routeTo(this.href); - } + if (href?.length) { + DiscourseURL.routeTo(href); + } - event.preventDefault(); - event.stopPropagation(); + event.preventDefault(); + event.stopPropagation(); + + return false; + } }, }); diff --git a/app/assets/javascripts/discourse/app/components/d-editor.js b/app/assets/javascripts/discourse/app/components/d-editor.js index 3dcdfd0f7e2..298dc542ecb 100644 --- a/app/assets/javascripts/discourse/app/components/d-editor.js +++ b/app/assets/javascripts/discourse/app/components/d-editor.js @@ -289,10 +289,6 @@ export default Component.extend(TextareaTextManipulation, { "indentSelection" ); } - - if (isTesting()) { - this.element.addEventListener("paste", this.paste); - } }, @bind diff --git a/app/assets/javascripts/discourse/app/initializers/ember-events.js b/app/assets/javascripts/discourse/app/initializers/ember-events.js deleted file mode 100644 index 231b01712c4..00000000000 --- a/app/assets/javascripts/discourse/app/initializers/ember-events.js +++ /dev/null @@ -1,46 +0,0 @@ -import Ember from "ember"; - -let initializedOnce = false; - -export default { - name: "ember-events", - - initialize() { - // By default Ember listens to too many events. This tells it the only events - // we're interested in. (it removes mousemove, touchstart and touchmove) - if (initializedOnce) { - return; - } - - Ember.EventDispatcher.reopen({ - events: { - touchend: "touchEnd", - touchcancel: "touchCancel", - keydown: "keyDown", - keyup: "keyUp", - keypress: "keyPress", - mousedown: "mouseDown", - mouseup: "mouseUp", - contextmenu: "contextMenu", - click: "click", - dblclick: "doubleClick", - focusin: "focusIn", - focusout: "focusOut", - mouseenter: "mouseEnter", - mouseleave: "mouseLeave", - submit: "submit", - input: "input", - change: "change", - dragstart: "dragStart", - drag: "drag", - dragenter: "dragEnter", - dragleave: "dragLeave", - dragover: "dragOver", - drop: "drop", - dragend: "dragEnd", - }, - }); - - initializedOnce = true; - }, -}; diff --git a/app/assets/javascripts/discourse/app/lib/ember-action-modifier.js b/app/assets/javascripts/discourse/app/lib/ember-action-modifier.js new file mode 100644 index 00000000000..61177cce935 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/ember-action-modifier.js @@ -0,0 +1,92 @@ +import { modifier } from "ember-modifier"; + +/** + * Creates a replacement for Ember's built-in `action` modifier that uses + * `addEventListener` directly instead of relying upon classic event delegation. + * + * This relies upon a deep override of Ember's rendering internals. If possible, + * consider eliminating usage of `action` as a modifier instead. + * + * Reference: https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/glimmer/lib/helpers/action.ts + */ +export const actionModifier = modifier( + ( + element, + [context, callback, ...args], + { on, bubbles, preventDefault, allowedKeys } + ) => { + const handler = (event) => { + const fn = typeof callback === "string" ? context[callback] : callback; + if (fn === undefined) { + throw new Error( + "Unexpected callback for `action` modifier. Please provide either a function or the name of a method on the current context." + ); + } + + if (!isAllowedEvent(event, allowedKeys)) { + return true; + } + + if (preventDefault !== false) { + event.preventDefault(); + } + + let shouldBubble = bubbles !== false; + if (!shouldBubble) { + event.stopPropagation(); + } + + if (args.length > 0) { + return fn.call(context, ...args); + } else { + return fn.call(context, event); + } + }; + + const eventName = on ?? "click"; + element.addEventListener(eventName, handler); + + return () => { + element.removeEventListener(eventName, handler); + }; + }, + { eager: false } +); + +export function isSimpleClick(event) { + if (!(event instanceof MouseEvent)) { + return false; + } + let modKey = event.shiftKey || event.metaKey || event.altKey || event.ctrlKey; + let secondaryClick = event.which > 1; // IE9 may return undefined + + return !modKey && !secondaryClick; +} + +const MODIFIERS = ["alt", "shift", "meta", "ctrl"]; +const POINTER_EVENT_TYPE_REGEX = /^click|mouse|touch/; + +function isAllowedEvent(event, allowedKeys) { + if (allowedKeys === null || allowedKeys === undefined) { + if (POINTER_EVENT_TYPE_REGEX.test(event.type)) { + return isSimpleClick(event); + } else { + allowedKeys = ""; + } + } + + if (allowedKeys.indexOf("any") >= 0) { + return true; + } + + for (let i = 0; i < MODIFIERS.length; i++) { + if ( + event[MODIFIERS[i] + "Key"] && + allowedKeys.indexOf(MODIFIERS[i]) === -1 + ) { + return false; + } + } + + return true; +} diff --git a/app/assets/javascripts/discourse/app/lib/ember-events.js b/app/assets/javascripts/discourse/app/lib/ember-events.js new file mode 100644 index 00000000000..1c4e6012181 --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/ember-events.js @@ -0,0 +1,241 @@ +// eslint-disable-next-line ember/no-classic-components +import Component from "@ember/component"; +import EmberObject from "@ember/object"; +import { actionModifier } from "./ember-action-modifier"; + +/** + * Classic Ember components (i.e. "@ember/component") rely upon "event + * delegation" to listen for events at the application root and then dispatch + * those events to any event handlers defined on individual Classic components. + * This coordination is handled by Ember's EventDispatcher. + * + * In contrast, Glimmer components (i.e. "@glimmer/component") expect event + * listeners to be added to elements using modifiers (such as `{{on "click"}}`). + * These event listeners are added directly to DOM elements using + * `addEventListener`. There is no need for an event dispatcher. + * + * Issues may arise when using Classic and Glimmer components together, since it + * requires reconciling the two event handling approaches. For instance, event + * propagation may not work as expected when a Classic component is nested + * inside a Glimmer component. + * + * `normalizeEmberEventHandling` helps an application standardize upon the + * Glimmer event handling approach by eliminating usage of event delegation and + * instead rewiring Classic components to directly use `addEventListener`. + * + * Specifically, it performs the following: + * + * - Invokes `eliminateClassicEventDelegation()` to remove all events associated + * with Ember's EventDispatcher to reduce its runtime overhead and ensure that + * it is effectively not in use. + * + * - Invokes `rewireClassicComponentEvents(app)` to rewire each Classic + * component to add its own event listeners for standard event handlers (e.g. + * `click`, `mouseDown`, `submit`, etc.). + * + * - Configures an instance initializer that invokes + * `rewireActionModifier(appInstance)` to redefine the `action` modifier with + * a substitute that uses `addEventListener`. + * + * @param {Application} app + */ +export function normalizeEmberEventHandling(app) { + eliminateClassicEventDelegation(); + rewireClassicComponentEvents(app); + app.instanceInitializer({ + name: "rewire-action-modifier", + initialize: (appInstance) => rewireActionModifier(appInstance), + }); +} + +/** + * Remove all events registered with Ember's EventDispatcher to reduce its + * runtime overhead. + */ +function eliminateClassicEventDelegation() { + // eslint-disable-next-line no-undef + Ember.EventDispatcher.reopen({ + events: {}, + }); +} + +/** + * Standard Ember event handlers, keyed by matching DOM events. + * + * Source: https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/views/lib/system/event_dispatcher.ts#L64-L89 + * + * @type {Record} + */ +const EVENTS = { + touchstart: "touchStart", + touchmove: "touchMove", + touchend: "touchEnd", + touchcancel: "touchCancel", + keydown: "keyDown", + keyup: "keyUp", + keypress: "keyPress", + mousedown: "mouseDown", + mouseup: "mouseUp", + contextmenu: "contextMenu", + click: "click", + dblclick: "doubleClick", + focusin: "focusIn", + focusout: "focusOut", + submit: "submit", + input: "input", + change: "change", + dragstart: "dragStart", + drag: "drag", + dragenter: "dragEnter", + dragleave: "dragLeave", + dragover: "dragOver", + drop: "drop", + dragend: "dragEnd", +}; + +/** + * @type {WeakMap} + */ +const COMPONENT_SETUP = new WeakMap(); + +const INTERNAL = Symbol("INTERNAL"); + +/** + * Rewires classic component event handling to use `addEventListener` directly + * on inserted elements, instead of relying upon classic event delegation. + * + * This maximizes compatibility with glimmer components and event listeners + * added via the `on` modifier. In particular, using `addEventListener` + * consistently everywhere ensures that event propagation works as expected + * between parent and child elements. + * + * @param {Application} app + */ +function rewireClassicComponentEvents(app) { + const allEvents = { ...EVENTS }; + + if (app.customEvents) { + for (const [event, methodName] of Object.entries(app.customEvents)) { + allEvents[event] = methodName; + } + } + + const allEventMethods = {}; + for (const [event, methodName] of Object.entries(allEvents)) { + allEventMethods[methodName] = event; + } + + // Avoid Component.reopen to stop `ember.component.reopen` deprecation warning + EmberObject.reopen.call(Component, { + /** + * @param {string | typeof INTERNAL} name + * @param {unknown[]} args + */ + trigger(name, ...args) { + if (name === INTERNAL) { + if (this.element) { + return this._super.call(this, ...args); + } + } else if (name.toLowerCase() in allEvents) { + return; + } else { + return this._super.call(this, name, ...args); + } + }, + + initEventListeners() { + const proto = Object.getPrototypeOf(this); + let protoEvents = COMPONENT_SETUP.get(proto); + const ownProps = Reflect.ownKeys(this); + + // Memoize prototype event handlers at the prototype and add listeners + // to every instance. + if (!protoEvents) { + protoEvents = []; + COMPONENT_SETUP.set(proto, protoEvents); + + for (const method of Object.keys(allEventMethods)) { + if (this.has(method) && !ownProps.includes(method)) { + const event = allEventMethods[method]; + protoEvents.push({ event, method }); + } + } + } + addComponentEventListeners(this, protoEvents); + + // Check every component instance for event handlers added via arguments + // specific to the instance. + // + // TODO: optimize perf since this will be run for every component instance + let ownEvents; + for (const method of Object.keys(allEventMethods)) { + if (ownProps.includes(method)) { + const event = allEventMethods[method]; + ownEvents ??= []; + ownEvents.push({ event, method }); + } + } + if (ownEvents) { + addComponentEventListeners(this, ownEvents); + } + }, + + // eslint-disable-next-line ember/no-component-lifecycle-hooks + didInsertElement() { + this._super(...arguments); + this.initEventListeners(); + }, + }); +} + +/** + * Rewires the `action` modifier to use `addEventListener` directly instead of + * relying upon classic event delegation. + * + * This relies upon a deep override of Ember's rendering internals. If possible, + * consider eliminating usage of `action` as a modifier instead. + * + * @param {ApplicationInstance} appInstance + */ +function rewireActionModifier(appInstance) { + // This is a deep runtime override, since neither the runtime resolver nor the + // built-in `action` modifier seem to be available otherwise. + // + // TODO: Investigate if a cleaner override is possible. + const renderer = appInstance.lookup("renderer:-dom"); + const lookupModifier = renderer._runtimeResolver.lookupModifier; + renderer._runtimeResolver.lookupModifier = (name, owner) => { + if (name === "action") { + return actionModifier; + } else { + return lookupModifier(name, owner); + } + }; +} + +function addComponentEventListeners(component, events) { + if (events?.length > 0) { + const { element } = component; + if (element) { + for (const { event, method } of events) { + element.addEventListener(event, (e) => { + const ret = component.trigger.call(component, INTERNAL, method, e); + // If an event handler returns `false`, assume the intent is to stop + // propagation and default event handling, as per the behavior + // encoded in Ember's `EventDispatcher`. + // + // See: https://github.com/emberjs/ember.js/blob/7d9095f38911d30aebb0e67ceec13e4a9818088b/packages/%40ember/-internals/views/lib/system/event_dispatcher.ts#L331-L337 + if (ret === false) { + e.preventDefault(); + e.stopPropagation(); + } + return ret; + }); + } + } else { + throw new Error( + `Could not configure classic component event listeners on '${component.toString()}' without 'element'` + ); + } + } +} diff --git a/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs index 7c59effdfd8..bccf3ff4ac5 100644 --- a/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs @@ -3,7 +3,7 @@

{{i18n "groups.manage.email.smtp_instructions"}}

@@ -23,7 +23,7 @@
{{i18n "groups.manage.email.imap_alpha_warning"}}
diff --git a/app/assets/javascripts/discourse/tests/unit/lib/ember-action-modifer-test.js b/app/assets/javascripts/discourse/tests/unit/lib/ember-action-modifer-test.js new file mode 100644 index 00000000000..8ede3ff97b6 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/unit/lib/ember-action-modifer-test.js @@ -0,0 +1,76 @@ +import { module, test } from "qunit"; +import { setupRenderingTest } from "ember-qunit"; +import { click, doubleClick, render } from "@ember/test-helpers"; +import { hbs } from "ember-cli-htmlbars"; + +module("Unit | Lib | ember-action-modifer", function (hooks) { + setupRenderingTest(hooks); + + test("`{{action}}` can target a function", async function (assert) { + let i = 0; + + this.setProperties({ + onChildClick: () => this.set("childClicked", i++), + childClicked: undefined, + }); + + await render(hbs` + +`; + +module("Unit | Lib | ember-events", function (hooks) { + setupRenderingTest(hooks); + + hooks.beforeEach(function () { + this.owner.register( + "component:example-classic-button", + ExampleClassicButton + ); + this.owner.register( + "template:components/example-classic-button", + exampleClassicButtonTemplate + ); + + this.owner.register( + "component:example-glimmer-button", + ExampleGlimmerButton + ); + this.owner.register( + "template:components/example-glimmer-button", + exampleGlimmerButtonTemplate + ); + }); + + module("nested glimmer inside classic", function () { + test("it handles click events and allows propagation by default", async function (assert) { + let i = 0; + + this.setProperties({ + onParentClick: () => this.set("parentClicked", i++), + onChildClick: () => this.set("childClicked", i++), + parentClicked: undefined, + childClicked: undefined, + }); + + await render(hbs` + + + + `); + + await click("#childButton"); + + assert.strictEqual(this.childClicked, 0); + assert.strictEqual(this.parentClicked, 1); + }); + + test("it handles click events and can prevent event propagation", async function (assert) { + let i = 0; + + this.setProperties({ + onParentClick: () => this.set("parentClicked", i++), + onChildClick: () => this.set("childClicked", i++), + parentClicked: undefined, + childClicked: undefined, + }); + + await render(hbs` + + + + `); + + await click("#childButton"); + + assert.strictEqual(this.childClicked, 0); + assert.strictEqual(this.parentClicked, undefined); + }); + }); + + module("nested classic inside glimmer", function () { + test("it handles click events and allows propagation by default", async function (assert) { + let i = 0; + + this.setProperties({ + onParentClick: () => this.set("parentClicked", i++), + onChildClick: () => this.set("childClicked", i++), + parentClicked: undefined, + childClicked: undefined, + }); + + await render(hbs` + + + + `); + + await click("#childButton"); + + assert.strictEqual(this.childClicked, 0); + assert.strictEqual(this.parentClicked, 1); + }); + + test("it handles click events and can prevent event propagation", async function (assert) { + let i = 0; + + this.setProperties({ + onParentClick: () => this.set("parentClicked", i++), + onChildClick: () => this.set("childClicked", i++), + parentClicked: undefined, + childClicked: undefined, + }); + + await render(hbs` + + + + `); + + await click("#childButton"); + + assert.strictEqual(this.childClicked, 0); + assert.strictEqual(this.parentClicked, undefined); + }); + }); + + module("nested `{{action}}` usage inside classic", function () { + test("it handles click events and allows propagation by default", async function (assert) { + let i = 0; + + this.setProperties({ + onParentClick: () => this.set("parentClicked", i++), + onChildClick: () => this.set("childClicked", i++), + parentClicked: undefined, + childClicked: undefined, + }); + + await render(hbs` + +