Skip to content

Ensure that keypad backspaces don't delete entire parentheses nodes at once. #2292

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 11, 2025
Merged

Ensure that keypad backspaces don't delete entire parentheses nodes at once. #2292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa3b6df
[dial-back] Ensure that keypad backspaces don't delete entire parenth…
SonicScrewdriver Mar 10, 2025
178db0a
[dial-back] docs(changeset): Ensure that keypad backspaces don't dele…
SonicScrewdriver Mar 10, 2025
fe0b74b
[dial-back] Remove tests for behaviour we no longer want, or need to …
SonicScrewdriver Mar 10, 2025
419bb9d
[dial-back] Added new test!
SonicScrewdriver Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/spotty-pans-yell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/math-input": patch
---

Ensure that keypad backspaces don't delete entire parentheses nodes at once.
107 changes: 5 additions & 102 deletions packages/math-input/src/components/input/__tests__/mathquill.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,62 +302,16 @@ describe("MathQuill", () => {
expect(mathField.getContent()).toEqual("\\left(\\right)");
});

it("should select an expression when deleting from outside (1)", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply deleted these tests as we no longer want this unique behaviour. I didn't think there'd be much value in adding new tests to test the default behaviour, but I'm happy to take feedback on this!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be worth adding one test to make sure we don't use the old behavior? Something like it("it doesn't delete whole expressions when using backspace (MOB-5576)", () => {

Were these tests failing after you made your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were (failing)!

I like including the ticket number in the test - great idea! I'll get that added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@handeyeco , I've added the test! I added some comments as it's a little confusing with how it works. :)

While we may show the user that we've "deleted" the parens, they both still exist in the code until the expression is fully empty and the backspace is hit one more time to remove the opening parens.

it("should not select or delete the entire expression on backspace from outside (MOB-5576)", () => {
const expr = "\\left(35x+5\\right)";
mathField.setContent(expr);
// "Delete" the right parens (it still exists until the parens are fully empty)
mathField.pressKey("BACKSPACE");
expect(mathField.isSelected()).toBeTruthy();
expect(mathField.getContent()).toEqual(expr);
});
expect(mathField.isSelected()).toBeFalsy();

it("should select an expression when deleting from outside (2)", () => {
const expr = "1+\\left(35x+5\\right)";
mathField.setContent(expr);
// Delete the first element within the parens
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left.ctrlSeq).toEqual("+");
expect(right).toEqual(END_OF_EXPR);
expect(mathField.getContent()).toEqual(expr);
});

it("should select an expression when deleting from outside (3)", () => {
const expr = "1+\\left(35x+5\\right)-1";
mathField.setContent(expr);
mathField.pressKey("LEFT");
mathField.pressKey("LEFT");
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left.ctrlSeq).toEqual("+");
expect(right.ctrlSeq).toEqual("-");
expect(mathField.getContent()).toEqual(expr);
});

it("should select an expression when deleting from outside (4)", () => {
const expr = "\\left(35x+5\\right)-1";
mathField.setContent(expr);
mathField.pressKey("LEFT");
mathField.pressKey("LEFT");
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left).toEqual(END_OF_EXPR);
expect(right.ctrlSeq).toEqual("-");
expect(mathField.getContent()).toEqual(expr);
});

it("should select an expression when deleting from outside", () => {
mathField.setContent("\\left(35x+5\\right)");
mathField.pressKey("BACKSPACE");
expect(mathField.isSelected()).toBeTruthy();
expect(mathField.getContent()).toEqual("\\left(35x+5\\right)");
expect(mathField.getContent()).toEqual("\\left(35x+\\right)");
});

// TODO(kevinb) fix this behavior so that we delete the exponent too
Expand Down Expand Up @@ -509,57 +463,6 @@ describe("MathQuill", () => {
expect(mathField.getContent()).toEqual(expr);
});

it("should select log when outside full log at tail (1)", () => {
const expr = "\\log\\left(35x\\right)";
mathField.setContent(expr);
mathField.pressKey("BACKSPACE");
expect(mathField.isSelected()).toBeTruthy();
expect(mathField.getContent()).toEqual(expr);
});

it("should select log when outside full log at tail (2)", () => {
const expr = "1+\\log\\left(35x\\right)";
mathField.setContent(expr);
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left.ctrlSeq).toEqual("+");
expect(right).toEqual(END_OF_EXPR);
expect(mathField.getContent()).toEqual(expr);
});

it("should select log when outside full log at tail (3)", () => {
const expr = "1+\\log\\left(35x\\right)-1";
mathField.setContent(expr);
mathField.pressKey("LEFT");
mathField.pressKey("LEFT");
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left.ctrlSeq).toEqual("+");
expect(right.ctrlSeq).toEqual("-");
expect(mathField.getContent()).toEqual(expr);
});

it("should select log when outside full log at tail (4)", () => {
const expr = "\\log\\left(35x\\right)-1";
mathField.setContent(expr);
mathField.pressKey("LEFT");
mathField.pressKey("LEFT");
mathField.pressKey("BACKSPACE");
const selection = mathField.getSelection();
const left = selection.ends[MQ.L][MQ.L];
const right = selection.ends[MQ.R][MQ.R];

expect(left).toEqual(END_OF_EXPR);
expect(right.ctrlSeq).toEqual("-");
expect(mathField.getContent()).toEqual(expr);
});

it("should delete empty log when at index", () => {
mathField.setContent("\\log_{ }\\left(\\right)");
mathField.moveToStart();
Expand Down
35 changes: 0 additions & 35 deletions packages/math-input/src/components/key-handlers/handle-backspace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,39 +118,6 @@ function handleBackspaceInLogIndex(
}
}

function handleBackspaceOutsideParens(cursor: MathQuill.Cursor) {
// In this case the node with '\\left(' for its ctrlSeq
// is the parent of the expression contained within the
// parentheses.
//
// Handle selecting an expression before deleting:
// (x+1)| => |(x+1)|
// \log(x+1)| => |\log(x+1)|

const leftNode = cursor[mathQuillInstance.L];
const rightNode = cursor[mathQuillInstance.R];
const command = maybeFindCommandBeforeParens(leftNode);

if (command && command.startNode) {
// There's a command before the parens so we select it as well as
// the parens.
cursor.insLeftOf(command.startNode);
cursor.startSelection();
if (rightNode === MathFieldActionType.MQ_END) {
cursor.insAtRightEnd(cursor.parent);
} else {
cursor.insLeftOf(rightNode);
}
cursor.select();
cursor.endSelection();
} else {
cursor.startSelection();
cursor.insLeftOf(leftNode); // left of \\left(
cursor.select();
cursor.endSelection();
}
}

function handleBackspaceInsideParens(
mathField: MathFieldInterface,
cursor: MathQuill.Cursor,
Expand Down Expand Up @@ -248,8 +215,6 @@ function handleBackspace(mathField: MathFieldInterface) {
selectNode(leftNode, cursor);
} else if (isNthRootIndex(parent)) {
handleBackspaceInRootIndex(mathField, cursor);
} else if (leftNode.ctrlSeq === "\\left(") {
handleBackspaceOutsideParens(cursor);
} else if (grandparent.ctrlSeq === "\\left(") {
handleBackspaceInsideParens(mathField, cursor);
} else if (isInsideLogIndex(cursor)) {
Expand Down
Loading