Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@
},
"//dependencies": "entities pinned to 6.x — latest dual CJS/ESM release; 7+ are ESM-only and break the CommonJS jest tests and the .cjs build",
"dependencies": {
"bidi-js": "^1.0.3",
"entities": "^6.0.1"
}
}
32 changes: 32 additions & 0 deletions src/bidi-js.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
declare module "bidi-js" {
export type BidiCharTypeName =
| "L"
| "R"
| "AL"
| "EN"
| "ES"
| "ET"
| "AN"
| "CS"
| "NSM"
| "BN"
| "B"
| "S"
| "WS"
| "ON"
| "LRE"
| "LRO"
| "RLE"
| "RLO"
| "PDF"
| "LRI"
| "RLI"
| "FSI"
| "PDI";

export interface Bidi {
getBidiCharTypeName(char: string): BidiCharTypeName;
}

export default function bidiFactory(): Bidi;
}
42 changes: 34 additions & 8 deletions src/parser/htmlIsolatesPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ import {
} from "@codemirror/view";
import { Prec } from "@codemirror/state";
import { syntaxTree } from "@codemirror/language";
import { Tree } from "@lezer/common";
import { SyntaxNodeRef, Tree } from "@lezer/common";
import { RangeSetBuilder } from "@codemirror/state";
import bidiFactory from "bidi-js";

export const htmlIsolatesPlugin = ViewPlugin.fromClass(
class {
Expand Down Expand Up @@ -62,18 +63,43 @@ function computeIsolates(view: EditorView) {
from,
to,
enter(node) {
if (
node.name === "HtmlTagOpen" ||
node.name === "HtmlTagClose" ||
node.name === "Expression"
) {
if (node.name === "HtmlTagOpen" || node.name === "HtmlTagClose") {
set.add(node.from, node.to, isolateLTR);
}
if (node.name === "TextRoot" || node.name === "Text") {
} else if (node.name === "Expression") {
// A valid placeholder is markup and stays LTR. An invalid one is
// arbitrary user text, so it follows its own content's direction
// instead of being forced LTR (which would scramble RTL content).
const followsRtlContent =
isInvalidExpression(node) &&
contentDirection(
view.state.doc.sliceString(node.from, node.to)
) === Direction.RTL;
set.add(node.from, node.to, followsRtlContent ? isolateRTL : isolateLTR);
} else if (node.name === "TextRoot" || node.name === "Text") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Design note: enter now runs three direction policies — tags & valid Expression forced LTR, Text/TextRoot forced RTL, and invalid Expression content-derived (first-strong). By this PR's own reasoning an invalid expression is 'arbitrary user text' — yet the visually-identical Text node is forced RTL while the invalid Expression uses plaintext direction. Two rules for the same kind of content, and a direction boundary between an invalid expression and the text around it. Worth considering one content-direction policy for all user text (bigger blast radius, needs RTL regression tests) instead of a special case — and deriving direction from a maintained bidi utility rather than a bespoke 5-script regex in the view layer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Kept the special case deliberately. This plugin only mounts when the language itself is RTL, so forcing Text to RTL is the point: a Latin-only fragment of an Arabic translation must still sit in the RTL paragraph flow. Switching Text to first-strong would relayout every Latin-heavy RTL translation. The invalid expression genuinely cannot reuse that policy: forcing it RTL renders Latin content with the braces flipped outward (}placeholder:space{ ), which is the neutral-brackets problem this PR fixes. The bespoke-regex half of the concern is gone now that bidi-js owns the data. If we later want one plaintext policy for all user text, I would do that as its own change with RTL regression tests.

set.add(node.from, node.to, isolateRTL);
}
},
});
}
return set.finish();
}

function isInvalidExpression(node: SyntaxNodeRef) {
return node.node.firstChild?.nextSibling?.name === "InvalidExpressionBody";
}

const bidi = bidiFactory();

// First-strong direction (UAX#9 rules P2-P3), character classes from bidi-js.
function contentDirection(text: string): Direction {
for (const char of text) {
const type = bidi.getBidiCharTypeName(char);
if (type === "R" || type === "AL") {
return Direction.RTL;
}
if (type === "L") {
return Direction.LTR;
}
}
return Direction.LTR;
}
30 changes: 29 additions & 1 deletion src/parser/lezer/tokenizer/tokenizer.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { Text, TextNested } from "../tolgeeParser.terms";
import {
InvalidExpressionContent,
Text,
TextNested,
} from "../tolgeeParser.terms";
import { ExternalTokenizer, InputStream } from "@lezer/lr";
import { matchText } from "./matchText";
import { fromCodePoint } from "@codemirror/state";

function getEscapable(isNested: boolean) {
const result = new Set(["{", "}"]);
Expand Down Expand Up @@ -48,3 +53,26 @@ export const textNested = new ExternalTokenizer((input) => {
textToken: TextNested,
});
});

// Everything inside a `{...}` up to the closing brace, used when the contents
// aren't a valid ICU expression (e.g. `placeholder:space`). Kept as one token so
// the whole `{...}` stays a single Expression node — else RTL bidi isolation
// splits it. `extend: true` lets it coexist with the real `Param`/`space` tokens
// so both parses are explored; InvalidExpressionBody's negative dynamicPrecedence
// then makes a valid format/select parse win whenever one exists.
export const invalidExpression = new ExternalTokenizer(
(input) => {
const startPosition = input.pos;
while (input.next !== -1) {
const char = fromCodePoint(input.next);
if (char === "{" || char === "}") {
break;
}
input.advance();
}
if (startPosition < input.pos) {
input.acceptToken(InvalidExpressionContent);
}
},
{ extend: true }
);
12 changes: 10 additions & 2 deletions src/parser/lezer/tolgee.grammar
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@
content { (Text | Expression | PluralPlaceholder ) content? }
contentNested { (TextNested | Expression | PluralPlaceholder ) contentNested? }

Expression { ExpressionOpen (FormatExpression | SelectExpression) ExpressionClose }
Expression { ExpressionOpen (FormatExpression | SelectExpression | InvalidExpressionBody) ExpressionClose }

// dynamicPrecedence=-1 makes a real format/select parse win whenever one exists;
// this is only reached for `{...}` that can't parse otherwise. See the
// `invalidExpression` tokenizer for the full rationale.
InvalidExpressionBody[@dynamicPrecedence=-1] { InvalidExpressionContent }

FormatExpression { space? Param space? (sep space? FormatFunction space? (sep (space? FormatStyle)+ space?)?)? }
SelectExpression { space? Param space? sep space? SelectFunction space? sep (space? Offset)? (space? SelectVariant)+ space? }
Expand All @@ -26,7 +30,11 @@ FormatStyle { format_style }
Text
}

@external tokens textNested from "./tokenizer/tokenizer" {
@external tokens invalidExpression from "./tokenizer/tokenizer" {
InvalidExpressionContent
}

@external tokens textNested from "./tokenizer/tokenizer" {
TextNested
}

Expand Down
36 changes: 19 additions & 17 deletions src/parser/lezer/tolgeeParser.terms.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
// This file was generated by lezer-generator. You probably shouldn't edit it.
export const
Text = 1,
TextNested = 2,
Root = 3,
Expression = 4,
ExpressionOpen = 5,
FormatExpression = 7,
Param = 8,
FormatFunction = 9,
FormatStyle = 10,
SelectExpression = 11,
SelectFunction = 12,
Offset = 13,
SelectVariant = 14,
VariantDescriptor = 15,
VariantContent = 16,
PluralPlaceholder = 17,
ExpressionClose = 19,
Nested = 20
InvalidExpressionContent = 2,
TextNested = 3,
Root = 4,
Expression = 5,
ExpressionOpen = 6,
FormatExpression = 8,
Param = 9,
FormatFunction = 10,
FormatStyle = 11,
SelectExpression = 12,
SelectFunction = 13,
Offset = 14,
SelectVariant = 15,
VariantDescriptor = 16,
VariantContent = 17,
PluralPlaceholder = 18,
InvalidExpressionBody = 20,
ExpressionClose = 21,
Nested = 22
38 changes: 33 additions & 5 deletions src/parser/lezer/tolgeeParser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ function expectToThrow(text = getText()) {
expect(() => tolgee(text)).toThrow();
}

function expectIcuOnlyThrows(text = getText()) {
expect(() => icu(text)).toThrow();
expect(() => tolgee(text)).not.toThrow();
}

describe("simple formatter", () => {
test("simple test", () => {
matchIcu();
Expand Down Expand Up @@ -84,27 +89,27 @@ describe("simple formatter", () => {
});

test("this also { } wrong", () => {
expectToThrowWithIcu();
expectIcuOnlyThrows();
});

test("this plain { , } wrong", () => {
expectToThrowWithIcu();
expectIcuOnlyThrows();
});

test("this is { unexpected", () => {
expectToThrowWithIcu();
});

test("this is obviously bad { yo yo }", () => {
expectToThrowWithIcu();
expectIcuOnlyThrows();
});

test("this is obviously bad { yo, }", () => {
expectToThrowWithIcu();
expectIcuOnlyThrows();
});

test(`test {', number, ::percent} in param name`, () => {
expectToThrowWithIcu();
expectIcuOnlyThrows();
});

// plurals
Expand Down Expand Up @@ -149,4 +154,27 @@ describe("simple formatter", () => {
"Auto translated {test, plural, =1 {# translation} other {# translations}}"
);
});

test("Leniently accepts invalid param contents {placeholder:space}", () => {
expectIcuOnlyThrows("{placeholder:space}");
});

test("Lenient invalid expression is a single Expression node", () => {
const tree = parser.parse(
"some rtl text {placeholder:space} more rtl text"
);
const expressions: [number, number][] = [];
tree.iterate({
enter(node) {
if (node.name === "Expression") {
expressions.push([node.from, node.to]);
}
},
});
expect(expressions).toEqual([[14, 33]]);
});

test("Lenient acceptance doesn't swallow following select variants", () => {
matchIcu("{name, plural, one {# one} other {# other}}");
});
});
21 changes: 11 additions & 10 deletions src/parser/lezer/tolgeeParser.ts

Large diffs are not rendered by default.

Loading
Loading