Skip to content

fix(keymap): prevent tilde key from highlighting entire keymap (@LodunCoombs) #7128

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
Miodec merged 4 commits into from
Nov 26, 2025
Merged

fix(keymap): prevent tilde key from highlighting entire keymap (@LodunCoombs) #7128

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a9a540
fix: prevent tilde key from highlighting entire keymap
LodunCoombs Nov 20, 2025
101f4b4
fix(keymap): handle key finding edge cases with performant CSS selectors
LodunCoombs Nov 21, 2025
1ba24a9
refactor(keymap): use native selectors for performance
LodunCoombs Nov 21, 2025
3257d84
fix(keymap): change delimiter to private unicode char and simplify se…
LodunCoombs Nov 22, 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
42 changes: 21 additions & 21 deletions frontend/src/ts/elements/keymap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { areSortedArraysEqual } from "../utils/arrays";
import { LayoutObject } from "@monkeytype/schemas/layouts";
import { animate } from "animejs";

export const keyDataDelimiter = "~~";
export const keyDataDelimiter = "\uE000";

const stenoKeys: LayoutObject = {
keymapShowTopRow: true,
Expand Down Expand Up @@ -58,27 +58,30 @@ const stenoKeys: LayoutObject = {
},
};

function findKeyElements(char: string): JQuery {
Copy link
Member

@fehmer fehmer Nov 20, 2025

Choose a reason for hiding this comment

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

This fix is not flashing/highlighting the ~ key anymore.

extracting the findKeyElements is a good idea. What about keeping the selector and just add a special handling the tilde key ?

function findKeyElements(currentKey: string): JQuery {
  if (currentKey === " ") {
    return $("#keymap .keySpace");
  }
  
  if (currentKey === '"') {
    return $(`#keymap .keymapKey[data-key*='${currentKey}']`);
  }

  if (currentKey === "~") {
    return $(`#keymap .keymapKey[data-key*="~~~"]`);
  }

  return $(`#keymap .keymapKey[data-key*="${currentKey}"]`);
}

Copy link
Contributor Author

@LodunCoombs LodunCoombs Nov 20, 2025

Choose a reason for hiding this comment

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

Hi, thank you for the review and the feedback!

I did some extra testing on my end, and the ~ key appears to be flashing correctly. I tested this by finding a visible key in DevTools (like the "K" key) and changing its data-key attribute to `~~~. When I pressed the tilde key, it lit up as expected.

The findKeyElements function handles this because "~~~".split('~~') correctly produces the array ['`', '~'], and .includes(char) returns true, which returns the correct element.

The main reason I chose this .filter() approach over adding a special case for the tilde was to create a more robust solution. It prevents the original bug (where the *= selector matched the ~~ delimiter itself) and also handles other special characters like " without needing more if statements in the future.

Could you let me know if you're seeing different behavior, or if there's a specific test case I might have missed? Happy to make changes if so!

Copy link
Member

@fehmer fehmer Nov 21, 2025

Choose a reason for hiding this comment

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

I modified the qwerty layout and replaced ; with ~

resulting in

<div class="keymapKey" data-key="~~~:"><span class="letter">~</span></div>

I am testing with chrome.

diff --git a/frontend/static/layouts/qwerty.json b/frontend/static/layouts/qwerty.json
index e17574f45..a389be9d8 100644
--- a/frontend/static/layouts/qwerty.json
+++ b/frontend/static/layouts/qwerty.json
@@ -42,7 +42,7 @@
       ["j", "J"],
       ["k", "K"],
       ["l", "L"],
-      [";", ":"],
+      ["~", ":"],
       ["'", "\""]
     ],
     "row4": [

Copy link
Member

@fehmer fehmer Nov 21, 2025

Choose a reason for hiding this comment

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

I agree that the filter is the more robust solution, but it seems to bit quite a bit slower.

Image

Copy link
Contributor Author

@LodunCoombs LodunCoombs Nov 21, 2025

Choose a reason for hiding this comment

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

Good catch, thank you for spotting that! I just pushed a new commit that should address the issues. It uses a combined CSS selector, which handles all edge cases, including when the data-key is ~~~:. And, the performance should be much faster using the native CSS selectors. Let me know what you think!

Copy link
Member

@fehmer fehmer Nov 21, 2025

Choose a reason for hiding this comment

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

New approach seems even slower

Image

Copy link
Contributor Author

@LodunCoombs LodunCoombs Nov 21, 2025

Choose a reason for hiding this comment

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

I've updated the code to follow your approach (special handling for " and ~).

I added one small tweak to the tilde selector: [data-key*="~~~"], [data-key="~"]. The second part handles the theoretical edge case where a key is mapped only to ~ (no delimiter). What do you think?

Copy link
Member

@fehmer fehmer Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @LodunCoombs ,

thanks for the change. I noticed another problem within the updateLegends function. I had a talk with @Miodec. We think the easiest fix for now would be to change the delimiter from ~~ to private use unicode character.

Please keep the extracted findKeyElements but remove the additional check for the tilde key.

Copy link
Contributor Author

@LodunCoombs LodunCoombs Nov 22, 2025

Choose a reason for hiding this comment

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

Great idea!
I've updated keyDataDelimiter to \uE000 and removed the special tilde handling from findKeyElements. What do you think?

if (char === " ") {
return $("#keymap .keySpace");
}

if (char === '"') {
return $(`#keymap .keymapKey[data-key*='${char}']`);
}

return $(`#keymap .keymapKey[data-key*="${char}"]`);
}

function highlightKey(currentKey: string): void {
if (Config.mode === "zen") return;
if (currentKey === "") currentKey = " ";
try {
$(".activeKey").removeClass("activeKey");

let highlightKey;
if (Config.language.startsWith("korean")) {
currentKey = Hangul.disassemble(currentKey)[0] ?? currentKey;
}
if (currentKey === " ") {
highlightKey = "#keymap .keySpace";
} else if (currentKey === '"') {
highlightKey = `#keymap .keymapKey[data-key*='${currentKey}']`;
} else {
highlightKey = `#keymap .keymapKey[data-key*="${currentKey}"]`;
}

// console.log("highlighting", highlightKey);

$(highlightKey).addClass("activeKey");
const $target = findKeyElements(currentKey);
$target.addClass("activeKey");
} catch (e) {
if (e instanceof Error) {
console.log("could not update highlighted keymap key: " + e.message);
Expand All @@ -88,14 +91,11 @@ function highlightKey(currentKey: string): void {

async function flashKey(key: string, correct?: boolean): Promise<void> {
if (key === undefined) return;
//console.log("key", key);
if (key === " ") {
key = "#keymap .keySpace";
} else if (key === '"') {
key = `#keymap .keymapKey[data-key*='${key}']`;
} else {
key = `#keymap .keymapKey[data-key*="${key}"]`;
}

const $target = findKeyElements(key);

const elements = $target.toArray();
if (elements.length === 0) return;

const themecolors = await ThemeColors.getAll();

Expand All @@ -120,7 +120,7 @@ async function flashKey(key: string, correct?: boolean): Promise<void> {
};
}

animate(key, {
animate(elements, {
color: [startingStyle.color, themecolors.sub],
backgroundColor: [startingStyle.backgroundColor, themecolors.subAlt],
borderColor: [startingStyle.borderColor, themecolors.sub],
Expand Down