github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-01-18 11:02:46 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: Smart list jankiness in Chrome and fix for Firefox (#27762)

Browse Source 
Last week I disabled smart lists in Firefox in 2ab4913d13.

This week the same issue presented itself in Chrome. Turns out,
the list modification was still not firing at the right time
in the event chain. I investigated and it looks as though
`beforeinput` is a better fit, since:

> This allows web apps to override text edit behavior before the browser
modifies the DOM tree, and provides more control over input events to
improve performance.

c.f. https://developer.mozilla.org/en-US/docs/Web/API/Element/beforeinput_event
and https://webkit.org/blog/7358/enhanced-editing-with-input-events/
and https://www.w3.org/TR/uievents/#events-keyboard-event-order

The order of keyboard events is `keydown` -> `beforeinput` -> `input` -> `keyup`

I changed to detect the event type of `insertLineBreak` which is
not always consistently true in `input` events. If it's true when
`beforeinput` is fired then we go ahead with the smart list when
`input` fires.
This commit is contained in:
Martin Brennan 2024-07-10 09:31:16 +10:00 committed by GitHub
parent 0165460626
commit 8c038d9498
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 73 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -53,6 +53,7 @@ class Toolbar {
const { siteSettings, capabilities } = opts;
this.shortcuts = {};
this.context = null;
this.handleSmartListAutocomplete = false;
this.groups = [
{ group: "fontStyles", buttons: [] },
@ -326,12 +327,23 @@ export default Component.extend(TextareaTextManipulation, {
this._itsatrap.bind(`${PLATFORM_KEY_MODIFIER}+shift+.`, () =>
this.send("insertCurrentTime")
);
this._itsatrap.bind("enter", () => {
// Check the textarea value after a brief delay to ensure the input event has updated the value
setTimeout(() => {
this.maybeContinueList();
}, 0);
});
// These must be bound manually because itsatrap does not support
// beforeinput or input events.
//
// beforeinput is better used to detect line breaks because it is
// fired before the actual value of the textarea is changed,
// and sometimes in the input event no `insertLineBreak` event type
// is fired.
//
// c.f. https://developer.mozilla.org/en-US/docs/Web/API/Element/beforeinput_event
if (this._textarea) {
this._textarea.addEventListener(
"beforeinput",
this.onBeforeInputSmartList
);
this._textarea.addEventListener("input", this.onInputSmartList);
}
// disable clicking on links in the preview
this.element
@ -351,6 +363,21 @@ export default Component.extend(TextareaTextManipulation, {
}
},
@bind
onBeforeInputSmartList(event) {
// This inputType is much more consistently fired in `beforeinput`
// rather than `input`.
this.handleSmartListAutocomplete = event.inputType === "insertLineBreak";
},
@bind
onInputSmartList() {
if (this.handleSmartListAutocomplete) {
this.maybeContinueList();
}
this.handleSmartListAutocomplete = false;
},
@bind
_handlePreviewLinkClick(event) {
if (wantsNewWindow(event)) {
@ -393,6 +420,14 @@ export default Component.extend(TextareaTextManipulation, {
);
}
if (this._textarea) {
this._textarea.removeEventListener(
"beforeinput",
this.onBeforeInputSmartList
);
this._textarea.removeEventListener("input", this.onInputSmartList);
}
this._itsatrap?.destroy();
this._itsatrap = null;

9
app/assets/javascripts/discourse/app/mixins/textarea-text-manipulation.js
View File

@ -515,15 +515,6 @@ export default Mixin.create({
@bind
maybeContinueList() {
// TODO (martin) Very inconsistent on Firefox at the moment, we end up with
// things like this. Something about newlines maybe?
//
// 1. a
// 2. test? 2. 2. 2. 2. 2.
if (this.capabilities.isFirefox) {
return false;
}
const offset = caretPosition(this._textarea);
const text = this._textarea.value;
const lines = text.substring(0, offset).split("\n");

59
app/assets/javascripts/discourse/tests/integration/components/d-editor-test.js
View File

@ -5,7 +5,7 @@ import {
focus,
render,
settled,
triggerKeyEvent,
triggerEvent,
} from "@ember/test-helpers";
import { hbs } from "ember-cli-htmlbars";
import { module, test } from "qunit";
@ -14,7 +14,6 @@ import { setCaretPosition } from "discourse/lib/utilities";
import { setupRenderingTest } from "discourse/tests/helpers/component-test";
import formatTextWithSelection from "discourse/tests/helpers/d-editor-helper";
import {
chromeTest,
exists,
paste,
query,
@ -78,13 +77,8 @@ module("Integration | Component | d-editor", function (hooks) {
return textarea;
}
function testCase(title, testFunc, opts = {}) {
let testFn = test;
if (opts.chrome) {
testFn = chromeTest;
}
testFn(title, async function (assert) {
function testCase(title, testFunc) {
test(title, async function (assert) {
this.set("value", "hello world.");
await render(hbs`<DEditor @value={{this.value}} />`);
@ -989,16 +983,32 @@ third line`
}
);
// Smart list functionality relies on beforeinput, which QUnit does not send with
// `typeIn` synthetic events. We need to send it ourselves manually along with `input`.
// Not ideal, but gets the job done.
//
// c.f. https://github.com/emberjs/ember-test-helpers/blob/master/API.md#typein and
// https://github.com/emberjs/ember-test-helpers/issues/1336
async function triggerEnter(textarea) {
await triggerEvent(textarea, "beforeinput", {
inputType: "insertLineBreak",
});
await triggerEvent(textarea, "input", {
inputType: "insertText",
data: "\n",
});
}
testCase(
"smart lists - pressing enter on a line with a list item starting with * creates a list item on the next line",
async function (assert, textarea) {
const initialValue = "* first item in list\n";
this.set("value", initialValue);
setCaretPosition(textarea, initialValue.length);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(this.value, initialValue + "* ");
},
{ chrome: true }
}
);
testCase(
@ -1007,10 +1017,9 @@ third line`
const initialValue = "- first item in list\n";
this.set("value", initialValue);
setCaretPosition(textarea, initialValue.length);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(this.value, initialValue + "- ");
},
{ chrome: true }
}
);
testCase(
@ -1019,10 +1028,9 @@ third line`
const initialValue = "1. first item in list\n";
this.set("value", initialValue);
setCaretPosition(textarea, initialValue.length);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(this.value, initialValue + "2. ");
},
{ chrome: true }
}
);
testCase(
@ -1031,13 +1039,12 @@ third line`
const initialValue = "* first item in list\n\n* second item in list";
this.set("value", initialValue);
setCaretPosition(textarea, 21);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(
this.value,
"* first item in list\n* \n* second item in list"
);
},
{ chrome: true }
}
);
testCase(
@ -1046,13 +1053,12 @@ third line`
const initialValue = "1. first item in list\n\n2. second item in list";
this.set("value", initialValue);
setCaretPosition(textarea, 22);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(
this.value,
"1. first item in list\n2. \n3. second item in list"
);
},
{ chrome: true }
}
);
testCase(
@ -1061,10 +1067,9 @@ third line`
const initialValue = "* first item in list with empty line\n* \n";
this.set("value", initialValue);
setCaretPosition(textarea, initialValue.length);
await triggerKeyEvent(textarea, "keydown", "Enter");
await triggerEnter(textarea);
assert.strictEqual(this.value, "* first item in list with empty line\n");
},
{ chrome: true }
}
);
(() => {