Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Revert "DEV: Normalize event handling to improve Glimmer + Classic component compat (#18490)" (#18725) 

This reverts commit 28be5d3037 and fcb4675415

This caused qunit timeouts in our internal CI environments. Not sure of the exact cause yet, but we're reverting for now while we investigate.
This commit is contained in:
David Taylor 2022-10-24 18:30:08 +01:00 committed by GitHub
parent fcb4675415
commit 4c4ac9cc47
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 82 additions and 715 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
app/assets/javascripts/discourse/app/app.js
View File

@ -1,7 +1,6 @@
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 = [];
@ -55,10 +54,6 @@ 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;

63
app/assets/javascripts/discourse/app/components/d-button.js
View File

@ -118,7 +118,8 @@ export default Component.extend({
},
click(event) {
return this._triggerAction(event);
this._triggerAction(event);
return false;
},
mouseDown(event) {
@ -128,41 +129,37 @@ export default Component.extend({
},
_triggerAction(event) {
let { action, route, href } = this;
let { action } = this;
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 (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 (route) {
this.router.transitionTo(route);
}
if (href?.length) {
DiscourseURL.routeTo(href);
}
event.preventDefault();
event.stopPropagation();
return false;
}
if (this.route) {
this.router.transitionTo(this.route);
}
if (this.href && this.href.length) {
DiscourseURL.routeTo(this.href);
}
event.preventDefault();
event.stopPropagation();
},
});

4
app/assets/javascripts/discourse/app/components/d-editor.js
View File

@ -289,6 +289,10 @@ export default Component.extend(TextareaTextManipulation, {
"indentSelection"
);
}
if (isTesting()) {
this.element.addEventListener("paste", this.paste);
}
},
@bind

46
app/assets/javascripts/discourse/app/initializers/ember-events.js Normal file
View File

@ -0,0 +1,46 @@
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;
},
};

92
app/assets/javascripts/discourse/app/lib/ember-action-modifier.js
View File

@ -1,92 +0,0 @@
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;
}

253
app/assets/javascripts/discourse/app/lib/ember-events.js
View File

@ -1,253 +0,0 @@
// eslint-disable-next-line ember/no-classic-components
import Component from "@ember/component";
import EmberObject from "@ember/object";
import { actionModifier } from "./ember-action-modifier";
import Ember from "ember";
/**
* 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),
});
}
let eliminatedClassicEventDelegation = false;
/**
* Remove all events registered with Ember's EventDispatcher to reduce its
* runtime overhead.
*/
function eliminateClassicEventDelegation() {
if (eliminatedClassicEventDelegation) {
return;
}
eliminatedClassicEventDelegation = true;
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<string, string>}
*/
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<object, { event: string; method: string }[]>}
*/
const COMPONENT_SETUP = new WeakMap();
const INTERNAL = Symbol("INTERNAL");
let rewireDone = false;
/**
* 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) {
if (rewireDone) {
return;
}
rewireDone = true;
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'`
);
}
}
}

4
app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs
View File

@ -3,7 +3,7 @@
<p>{{i18n "groups.manage.email.smtp_instructions"}}</p>
<label for="enable_smtp">
<Input @type="checkbox" @checked={{this.group.smtp_enabled}} id="enable_smtp" tabindex="1" {{on "input" this.smtpEnabledChange}} />
<Input @type="checkbox" @checked={{this.group.smtp_enabled}} id="enable_smtp" tabindex="1" {{on "change" this.smtpEnabledChange}} />
{{i18n "groups.manage.email.enable_smtp"}}
</label>
@ -23,7 +23,7 @@
<div class="alert alert-warning">{{i18n "groups.manage.email.imap_alpha_warning"}}</div>
<label for="enable_imap">
<Input @type="checkbox" disabled={{not this.enableImapSettings}} @checked={{this.group.imap_enabled}} id="enable_imap" tabindex="8" {{on "input" this.imapEnabledChange}} />
<Input @type="checkbox" disabled={{not this.enableImapSettings}} @checked={{this.group.imap_enabled}} id="enable_imap" tabindex="8" {{on "change" this.imapEnabledChange}} />
{{i18n "groups.manage.email.enable_imap"}}
</label>

76
app/assets/javascripts/discourse/tests/unit/lib/ember-action-modifer-test.js
View File

@ -1,76 +0,0 @@
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`
<button id="childButton" {{action this.onChildClick}} />
`);
await click("#childButton");
assert.strictEqual(this.childClicked, 0);
});
test("`{{action}}` can target a method on the current context by name", async function (assert) {
let i = 0;
this.setProperties({
onChildClick: () => this.set("childClicked", i++),
childClicked: undefined,
});
await render(hbs`
<button id="childButton" {{action 'onChildClick'}} />
`);
await click("#childButton");
assert.strictEqual(this.childClicked, 0);
});
test("`{{action}}` will ignore clicks combined with modifier keys", async function (assert) {
let i = 0;
this.setProperties({
onChildClick: () => this.set("childClicked", i++),
childClicked: undefined,
});
await render(hbs`
<button id="childButton" {{action 'onChildClick'}} />
`);
await click("#childButton", { ctrlKey: true });
assert.strictEqual(this.childClicked, undefined);
});
test("`{{action}}` can specify an event other than `click` via `on`", async function (assert) {
let i = 0;
this.setProperties({
onDblClick: () => this.set("dblClicked", i++),
dblClicked: undefined,
});
await render(hbs`
<button id="childButton" {{action this.onDblClick on='dblclick'}} />
`);
await doubleClick("#childButton");
assert.strictEqual(this.dblClicked, 0);
});
});

254
app/assets/javascripts/discourse/tests/unit/lib/ember-events-test.js
View File

@ -1,254 +0,0 @@
import { module, test } from "qunit";
import { setupRenderingTest } from "ember-qunit";
import { click, render } from "@ember/test-helpers";
import { hbs } from "ember-cli-htmlbars";
/* eslint-disable ember/require-tagless-components */
/* eslint-disable ember/no-classic-classes */
/* eslint-disable ember/no-classic-components */
import { default as ClassicComponent } from "@ember/component";
import { default as GlimmerComponent } from "@glimmer/component";
import { action } from "@ember/object";
// Configure test-local Classic and Glimmer components that
// will be immune from upgrades to actual Discourse components.
const ExampleClassicButton = ClassicComponent.extend({
tagName: "button",
type: "button",
preventEventPropagation: false,
onClick: null,
onMouseDown: null,
click(event) {
event.preventDefault();
if (this.preventEventPropagation) {
event.stopPropagation();
}
this.onClick?.(event);
},
});
const exampleClassicButtonTemplate = hbs`{{yield}}`;
class ExampleGlimmerButton extends GlimmerComponent {
@action
click(event) {
event.preventDefault();
if (this.args.preventEventPropagation) {
event.stopPropagation();
}
this.args.onClick?.(event);
}
}
const exampleGlimmerButtonTemplate = hbs`
<button {{on 'click' this.click}} type='button' ...attributes>
{{yield}}
</button>
`;
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`
<ExampleClassicButton id="parentButton" @onClick={{this.onParentClick}}>
<ExampleGlimmerButton id="childButton" @onClick={{this.onChildClick}} />
</ExampleClassicButton>
`);
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`
<ExampleClassicButton id="parentButton" @onClick={{this.onParentClick}}>
<ExampleGlimmerButton id="childButton" @preventEventPropagation={{true}} @onClick={{this.onChildClick}} />
</ExampleClassicButton>
`);
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`
<ExampleGlimmerButton id="parentButton" @onClick={{this.onParentClick}}>
<ExampleClassicButton id="childButton" @onClick={{this.onChildClick}} />
</ExampleGlimmerButton>
`);
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`
<ExampleGlimmerButton id="parentButton" @onClick={{this.onParentClick}}>
<ExampleClassicButton id="childButton" @preventEventPropagation={{true}} @onClick={{this.onChildClick}} />
</ExampleGlimmerButton>
`);
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`
<ExampleClassicButton id="parentButton" @onClick={{this.onParentClick}}>
<button id="childButton" {{action this.onChildClick}} />
</ExampleClassicButton>
`);
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`
<ExampleClassicButton id="parentButton" @onClick={{this.onParentClick}}>
<button id="childButton" {{action this.onChildClick bubbles=false}} />
</ExampleClassicButton>
`);
await click("#childButton");
assert.strictEqual(this.childClicked, 0);
assert.strictEqual(this.parentClicked, undefined);
});
});
module("nested `{{action}}` usage 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`
<ExampleGlimmerButton id="parentButton" @onClick={{this.onParentClick}}>
<button id="childButton" {{action this.onChildClick}} />
</ExampleGlimmerButton>
`);
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`
<ExampleGlimmerButton id="parentButton" @onClick={{this.onParentClick}}>
<button id="childButton" {{action this.onChildClick bubbles=false}} />
</ExampleGlimmerButton>
`);
await click("#childButton");
assert.strictEqual(this.childClicked, 0);
assert.strictEqual(this.parentClicked, undefined);
});
});
});