Skip to content

Commit 4318601

Browse files
nadalabaMiodec
andauthored
fix(funbox): caret/tape/ui issues in backwards funbox (@nadalaba) (monkeytypegame#6956)
### Description 1. refactor: - store (language direction / state of direction reversing funbox) in test-state.ts, and set them on `TestLogic.init()` which is called on each restart which happens on each change of Config.language or funbox. - use these new direction variables in (caret.ts / test-ui.ts / pace-caret.ts /result-word-highlight.ts) instead of calling `await JSONData.getCurrentLanguage(Config.language)`. 2. css changes: - add `unicode-bidi: bidi-override;` to .words with ligatures in backwards to fix the direction of LTR words on LTR languages in custom tests (which now have `.withLigatures` class regardless of language). - remove `direction: rtl;` from right to left .word and keep it on right to left #words. This was done because after adding the above `bidi-override`, (.word)s directioin was being forced to rtl on tests with RTL language and RTL words (custom and none custom tests), which is wrong (should be ltr on those tests because of the backwards funbox). - P.S., removing this from .word does not affect normal tests, because .word direction is inherited from #words directtion on non .withLigatures tests (e.g, non custom tests in non withLigatures languages), and it is calculated using internal browser algorithm based on characters used in .withLigatures tests (tests in languages with ligatures and all custom tests). 3. add the property "reverseDirection" to backwards funbox, which signifies that the direction of the test should be the reverse of the direction of Config.language, and the direction of a word should be the reverse of `Strings.isWordRightToLeft()`. 4. allow backwards funbox to work on languages with ligatures. 5. move `void Caret.updatePosition()` call to after the call of `TestUI.lineJump()` in `input-controller.ts:handleSpace()`. 6. change name of `Strings.getWordDirection()` to `Strings.isWordRightToLeft()` which explains what does the returned boolean mean, and add a parameter `reverseDirection` that flips the final result if true. --------- Co-authored-by: Miodec <[email protected]>
1 parent 5026f41 commit 4318601

File tree

14 files changed

+114
-98
lines changed

14 files changed

+114
-98
lines changed

frontend/__tests__/utils/strings.spec.ts

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ describe("string utils", () => {
270270
);
271271
});
272272

273-
describe("getWordDirection", () => {
273+
describe("isWordRightToLeft", () => {
274274
beforeEach(() => {
275275
Strings.clearWordDirectionCache();
276276
});
@@ -321,13 +321,27 @@ describe("string utils", () => {
321321
languageRTL: boolean,
322322
_description: string
323323
) => {
324-
expect(Strings.getWordDirection(word, languageRTL)).toBe(expected);
324+
expect(Strings.isWordRightToLeft(word, languageRTL)).toBe(expected);
325325
}
326326
);
327327

328328
it("should return languageRTL for undefined word", () => {
329-
expect(Strings.getWordDirection(undefined, false)).toBe(false);
330-
expect(Strings.getWordDirection(undefined, true)).toBe(true);
329+
expect(Strings.isWordRightToLeft(undefined, false)).toBe(false);
330+
expect(Strings.isWordRightToLeft(undefined, true)).toBe(true);
331+
});
332+
333+
// testing reverseDirection
334+
it("should return true for LTR word with reversed direction", () => {
335+
expect(Strings.isWordRightToLeft("hello", false, true)).toBe(true);
336+
expect(Strings.isWordRightToLeft("hello", true, true)).toBe(true);
337+
});
338+
it("should return false for RTL word with reversed direction", () => {
339+
expect(Strings.isWordRightToLeft("مرحبا", true, true)).toBe(false);
340+
expect(Strings.isWordRightToLeft("مرحبا", false, true)).toBe(false);
341+
});
342+
it("should return reverse of languageRTL for undefined word with reversed direction", () => {
343+
expect(Strings.isWordRightToLeft(undefined, false, true)).toBe(true);
344+
expect(Strings.isWordRightToLeft(undefined, true, true)).toBe(false);
331345
});
332346

333347
describe("caching", () => {
@@ -349,7 +363,7 @@ describe("string utils", () => {
349363

350364
it("should use cache for repeated calls", () => {
351365
// First call should cache the result (cache miss)
352-
const result1 = Strings.getWordDirection("hello", false);
366+
const result1 = Strings.isWordRightToLeft("hello", false);
353367
expect(result1).toBe(false);
354368
expect(mapSetSpy).toHaveBeenCalledWith("hello", false);
355369

@@ -358,7 +372,7 @@ describe("string utils", () => {
358372
mapSetSpy.mockClear();
359373

360374
// Second call should use cache (cache hit)
361-
const result2 = Strings.getWordDirection("hello", false);
375+
const result2 = Strings.isWordRightToLeft("hello", false);
362376
expect(result2).toBe(false);
363377
expect(mapGetSpy).toHaveBeenCalledWith("hello");
364378
expect(mapSetSpy).not.toHaveBeenCalled(); // Should not set again
@@ -367,47 +381,47 @@ describe("string utils", () => {
367381
mapGetSpy.mockClear();
368382
mapSetSpy.mockClear();
369383

370-
const result3 = Strings.getWordDirection("hello", true);
384+
const result3 = Strings.isWordRightToLeft("hello", true);
371385
expect(result3).toBe(false); // Still false because "hello" is LTR regardless of language
372386
expect(mapGetSpy).toHaveBeenCalledWith("hello");
373387
expect(mapSetSpy).not.toHaveBeenCalled(); // Should not set again
374388
});
375389

376390
it("should cache based on core word without punctuation", () => {
377391
// First call should cache the result for core "hello"
378-
const result1 = Strings.getWordDirection("hello", false);
392+
const result1 = Strings.isWordRightToLeft("hello", false);
379393
expect(result1).toBe(false);
380394
expect(mapSetSpy).toHaveBeenCalledWith("hello", false);
381395

382396
mapGetSpy.mockClear();
383397
mapSetSpy.mockClear();
384398

385399
// These should all use the same cache entry since they have the same core
386-
const result2 = Strings.getWordDirection("hello!", false);
400+
const result2 = Strings.isWordRightToLeft("hello!", false);
387401
expect(result2).toBe(false);
388402
expect(mapGetSpy).toHaveBeenCalledWith("hello");
389403
expect(mapSetSpy).not.toHaveBeenCalled();
390404

391405
mapGetSpy.mockClear();
392406
mapSetSpy.mockClear();
393407

394-
const result3 = Strings.getWordDirection("!hello", false);
408+
const result3 = Strings.isWordRightToLeft("!hello", false);
395409
expect(result3).toBe(false);
396410
expect(mapGetSpy).toHaveBeenCalledWith("hello");
397411
expect(mapSetSpy).not.toHaveBeenCalled();
398412

399413
mapGetSpy.mockClear();
400414
mapSetSpy.mockClear();
401415

402-
const result4 = Strings.getWordDirection("!hello!", false);
416+
const result4 = Strings.isWordRightToLeft("!hello!", false);
403417
expect(result4).toBe(false);
404418
expect(mapGetSpy).toHaveBeenCalledWith("hello");
405419
expect(mapSetSpy).not.toHaveBeenCalled();
406420
});
407421

408422
it("should handle cache clearing", () => {
409423
// Cache a result
410-
Strings.getWordDirection("test", false);
424+
Strings.isWordRightToLeft("test", false);
411425
expect(mapSetSpy).toHaveBeenCalledWith("test", false);
412426

413427
// Clear cache
@@ -419,14 +433,14 @@ describe("string utils", () => {
419433
mapClearSpy.mockClear();
420434

421435
// Should work normally after cache clear (cache miss again)
422-
const result = Strings.getWordDirection("test", false);
436+
const result = Strings.isWordRightToLeft("test", false);
423437
expect(result).toBe(false);
424438
expect(mapSetSpy).toHaveBeenCalledWith("test", false);
425439
});
426440

427441
it("should demonstrate cache miss vs cache hit behavior", () => {
428442
// Test cache miss - first time seeing this word
429-
const result1 = Strings.getWordDirection("unique", false);
443+
const result1 = Strings.isWordRightToLeft("unique", false);
430444
expect(result1).toBe(false);
431445
expect(mapGetSpy).toHaveBeenCalledWith("unique");
432446
expect(mapSetSpy).toHaveBeenCalledWith("unique", false);
@@ -435,7 +449,7 @@ describe("string utils", () => {
435449
mapSetSpy.mockClear();
436450

437451
// Test cache hit - same word again
438-
const result2 = Strings.getWordDirection("unique", false);
452+
const result2 = Strings.isWordRightToLeft("unique", false);
439453
expect(result2).toBe(false);
440454
expect(mapGetSpy).toHaveBeenCalledWith("unique");
441455
expect(mapSetSpy).not.toHaveBeenCalled(); // No cache set on hit
@@ -444,7 +458,7 @@ describe("string utils", () => {
444458
mapSetSpy.mockClear();
445459

446460
// Test cache miss - different word
447-
const result3 = Strings.getWordDirection("different", false);
461+
const result3 = Strings.isWordRightToLeft("different", false);
448462
expect(result3).toBe(false);
449463
expect(mapGetSpy).toHaveBeenCalledWith("different");
450464
expect(mapSetSpy).toHaveBeenCalledWith("different", false);

frontend/src/styles/test.scss

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -307,10 +307,6 @@
307307
&.rightToLeftTest {
308308
//flex-direction: row-reverse; // no need for hacking 😉, CSS fully support right-to-left languages
309309
direction: rtl;
310-
.word {
311-
//flex-direction: row-reverse;
312-
direction: rtl;
313-
}
314310
}
315311
&.withLigatures {
316312
.word {
@@ -749,10 +745,6 @@
749745
&.rightToLeftTest {
750746
//flex-direction: row-reverse; // no need for hacking 😉, CSS fully support right-to-left languages
751747
direction: rtl;
752-
.word {
753-
//flex-direction: row-reverse;
754-
direction: rtl;
755-
}
756748
}
757749
&.withLigatures {
758750
.word {

frontend/src/ts/controllers/input-controller.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,6 @@ async function handleSpace(): Promise<void> {
326326
void TestLogic.addWord();
327327
}
328328
TestUI.updateActiveElement();
329-
void Caret.updatePosition();
330329

331330
const shouldLimitToThreeLines =
332331
Config.mode === "time" ||
@@ -344,8 +343,10 @@ async function handleSpace(): Promise<void> {
344343

345344
if ((nextTop ?? 0) > currentTop) {
346345
void TestUI.lineJump(currentTop);
347-
} //end of line wrap
348-
}
346+
}
347+
} //end of line wrap
348+
349+
void Caret.updatePosition();
349350

350351
// enable if i decide that auto tab should also work after a space
351352
// if (

frontend/src/ts/elements/result-word-highlight.ts

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,7 @@
44
// Constants for padding around the highlights
55

66
import * as Misc from "../utils/misc";
7-
import * as JSONData from "../utils/json-data";
8-
import Config from "../config";
7+
import * as TestState from "../test/test-state";
98

109
const PADDING_X = 16;
1110
const PADDING_Y = 12;
@@ -56,7 +55,6 @@ let isInitialized = false;
5655
let isHoveringChart = false;
5756
let isFirstHighlightSinceInit = true;
5857
let isFirstHighlightSinceClear = true;
59-
let isLanguageRightToLeft = false;
6058
let isInitInProgress = false;
6159

6260
// Highlights .word elements in range [firstWordIndex, lastWordIndex]
@@ -104,7 +102,7 @@ export async function highlightWordsInRange(
104102
const newHighlightElementPositions = getHighlightElementPositions(
105103
firstWordIndex,
106104
lastWordIndex,
107-
isLanguageRightToLeft
105+
TestState.isLanguageRightToLeft
108106
);
109107

110108
// For each line...
@@ -198,10 +196,6 @@ async function init(): Promise<boolean> {
198196
);
199197
}
200198

201-
// Set isLanguageRTL
202-
const currentLanguage = await JSONData.getCurrentLanguage(Config.language);
203-
isLanguageRightToLeft = currentLanguage.rightToLeft ?? false;
204-
205199
RWH_el = $("#resultWordsHistory")[0] as HTMLElement;
206200
RWH_rect = RWH_el.getBoundingClientRect();
207201
wordEls = $(RWH_el).find(".words .word[input]");
@@ -309,7 +303,7 @@ async function init(): Promise<boolean> {
309303

310304
// For RTL languages, account for difference between highlightContainer left and RWH_el left
311305
let RTL_offset;
312-
if (isLanguageRightToLeft) {
306+
if (TestState.isLanguageRightToLeft) {
313307
RTL_offset = line.rect.left - RWH_rect.left + PADDING_X;
314308
} else {
315309
RTL_offset = 0;

frontend/src/ts/test/caret.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
1-
import * as JSONData from "../utils/json-data";
21
import Config from "../config";
32
import * as TestInput from "./test-input";
43
import * as SlowTimer from "../states/slow-timer";
54
import * as TestState from "../test/test-state";
65
import * as TestWords from "./test-words";
76
import { convertRemToPixels } from "../utils/numbers";
8-
import { splitIntoCharacters, getWordDirection } from "../utils/strings";
7+
import { splitIntoCharacters, isWordRightToLeft } from "../utils/strings";
98
import { safeNumber } from "@monkeytype/util/numbers";
109
import { subscribe } from "../observables/config-event";
1110

@@ -53,7 +52,6 @@ function getSpaceWidth(wordElement?: HTMLElement): number {
5352

5453
function getTargetPositionLeft(
5554
fullWidthCaret: boolean,
56-
isLanguageRightToLeft: boolean,
5755
activeWordElement: HTMLElement,
5856
currentWordNodeList: NodeListOf<HTMLElement>,
5957
fullWidthCaretWidth: number,
@@ -65,9 +63,10 @@ function getTargetPositionLeft(
6563
let result = 0;
6664

6765
// use word-specific direction if available and different from language direction
68-
const isWordRightToLeft = getWordDirection(
66+
const isWordRTL = isWordRightToLeft(
6967
currentWord,
70-
isLanguageRightToLeft
68+
TestState.isLanguageRightToLeft,
69+
TestState.isDirectionReversed
7170
);
7271

7372
if (Config.tapeMode === "off") {
@@ -77,7 +76,7 @@ function getTargetPositionLeft(
7776
const lastWordLetter = currentWordNodeList[wordLen - 1];
7877
const lastInputLetter = currentWordNodeList[inputLen - 1];
7978

80-
if (isWordRightToLeft) {
79+
if (isWordRTL) {
8180
if (inputLen <= wordLen && currentLetter) {
8281
// at word beginning in zen mode both lengths are 0, but currentLetter is defined "_"
8382
positionOffsetToWord =
@@ -110,13 +109,10 @@ function getTargetPositionLeft(
110109
$(document.querySelector("#wordsWrapper") as HTMLElement).width() ?? 0;
111110
const tapeMargin =
112111
wordsWrapperWidth *
113-
(isWordRightToLeft
114-
? 1 - Config.tapeMargin / 100
115-
: Config.tapeMargin / 100);
112+
(isWordRTL ? 1 - Config.tapeMargin / 100 : Config.tapeMargin / 100);
116113

117114
result =
118-
tapeMargin -
119-
(fullWidthCaret && isWordRightToLeft ? fullWidthCaretWidth : 0);
115+
tapeMargin - (fullWidthCaret && isWordRTL ? fullWidthCaretWidth : 0);
120116

121117
if (Config.tapeMode === "word" && inputLen > 0) {
122118
let currentWordWidth = 0;
@@ -131,7 +127,7 @@ function getTargetPositionLeft(
131127
// if current letter has zero width move the caret to previous positive width letter
132128
if ($(currentWordNodeList[inputLen] as Element).outerWidth(true) === 0)
133129
currentWordWidth -= lastPositiveLetterWidth;
134-
if (isWordRightToLeft) currentWordWidth *= -1;
130+
if (isWordRTL) currentWordWidth *= -1;
135131
result += currentWordWidth;
136132
}
137133
}
@@ -185,9 +181,6 @@ export async function updatePosition(noAnim = false): Promise<void> {
185181
const lastInputLetter = currentWordNodeList[inputLen - 1];
186182
const lastWordLetter = currentWordNodeList[wordLen - 1];
187183

188-
const currentLanguage = await JSONData.getCurrentLanguage(Config.language);
189-
const isLanguageRightToLeft = currentLanguage.rightToLeft ?? false;
190-
191184
// in blind mode, and hide extra letters, extra letters have zero offsets
192185
// offsetHeight is the same for all visible letters
193186
// so is offsetTop (for same line letters)
@@ -224,7 +217,6 @@ export async function updatePosition(noAnim = false): Promise<void> {
224217

225218
const letterPosLeft = getTargetPositionLeft(
226219
fullWidthCaret,
227-
isLanguageRightToLeft,
228220
activeWordEl,
229221
currentWordNodeList,
230222
letterWidth,

0 commit comments

Comments
 (0)