Skip to content
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

Fix tests for bidirectional isolation #917

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Fix tests for bidirectional isolation #917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions test/schemas/v0/tests.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@
"src": {
"$ref": "#/$defs/src"
},
"bidiIsolation": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/unicode-org/conformance/blob/main/schema/message_fmt2/testgen_schema.json will need to be updated to reflect this change.

"$ref": "#/$defs/bidiIsolation"
},
"params": {
"$ref": "#/$defs/params"
},
Expand Down Expand Up @@ -146,6 +149,9 @@
"src": {
"$ref": "#/$defs/src"
},
"bidiIsolation": {
"$ref": "#/$defs/bidiIsolation"
},
"params": {
"$ref": "#/$defs/params"
},
Expand All @@ -172,6 +178,10 @@
"description": "The MF2 syntax source.",
"type": "string"
},
"bidiIsolation": {
"description": "The bidi isolation strategy.",
"enum": ["default", "none"]
},
"params": {
"description": "Parameters to pass in to the formatter for resolving external variables.",
"type": "array",
Expand Down Expand Up @@ -243,6 +253,23 @@
}
}
},
{
"description": "Bidi isolation part.",
"type": "object",
"additionalProperties": false,
"required": [
"type",
"value"
],
"properties": {
"type": {
"const": "bidiIsolation"
},
"value": {
"enum": ["\u2066", "\u2067", "\u2068", "\u2069"]
}
}
},
{
"description": "Message markup part.",
"type": "object",
Expand Down
19 changes: 10 additions & 9 deletions test/tests/bidi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"scenario": "Bidi support",
"description": "Tests for correct parsing of messages with bidirectional marks and isolates",
"defaultTestProperties": {
"bidiIsolation": "default",
"locale": "en-US"
},
"tests": [
Expand All @@ -13,17 +14,17 @@
{
"description": "complex-message = o *(declaration o) complex-body o",
"src": "\u200E .local $x = {1} {{ {$x}}}",
"exp": " 1"
"exp": " \u20681\u2069"
},
{
"description": "complex-message = o *(declaration o) complex-body o",
"src": ".local $x = {1} \u200F {{ {$x}}}",
"exp": " 1"
"exp": " \u20681\u2069"
},
{
"description": "complex-message = o *(declaration o) complex-body o",
"src": ".local $x = {1} {{ {$x}}} \u2066",
"exp": " 1"
"exp": " \u20681\u2069"
},
{
"description": "input-declaration = input o variable-expression",
Expand Down Expand Up @@ -79,12 +80,12 @@
{
"description": "literal-expression = \"{\" o literal [s function] *(s attribute) o \"}\"",
"src": "{\u200E hello \u200F}",
"exp": "hello"
"exp": "\u2068hello\u2069"
},
{
"description": "variable-expression = \"{\" o variable [s function] *(s attribute) o \"}\"",
"src": ".local $x = {1} {{ {\u200E $x \u200F} }}",
"exp": " 1 "
"exp": " \u20681\u2069 "
},
{
"description": "function-expression = \"{\" o function *(s attribute) o \"}\"",
Expand Down Expand Up @@ -114,7 +115,7 @@
{
"description": " name... excludes U+FFFD and U+061C -- this pases as name -> [bidi] name-start *name-char",
"src": ".local $\u061Cfoo = {1} {{ {$\u061Cfoo} }}",
"exp": " 1 "
"exp": " \u20681\u2069 "
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess in this case, the direction of the message is determined to be unknown? Which is a little surprising to me, since the only bidi control character appears in one of the variable names, which I would expect not to have an effect on the formatted result of the message. Maybe what's throwing me off is that the explanation of the "Default Bidi Strategy" at the end of this section doesn't say how the message directionality is determined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The bidi controls in the syntax source don't matter here at all, and the message has LTR direction because of the en-US locale. It's the placeholder's direction that is unknown. For it to be known, we'd need to introspect the 1 string with something like UAX#9 P2, which will ultimately be done by the rendering engine, and even if we do, the direction would still be unknown because European numbers are only weakly directional. And so this is appropriately reflected by FSI/PDI isolating it.

This is much the same thing as I mention in #917 (comment), but for LTR instead of RTL content.

Maybe what's throwing me off is that the explanation of the "Default Bidi Strategy" at the end of this section doesn't say how the message directionality is determined?

Good point. We may want to add a mention under Formatting Context about deducing the message direction from the locale if it's not explicitly given.

Copy link
Member

Choose a reason for hiding this comment

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

If not explicitly provided, the direction of the message should be computed from the locale. As @eemeli notes, controls in the source don't matter. As noted elsewhere, you can't just look at the rendered placeholder to determine its desired direction--even strongly directional characters do not necessarily provide an accurate signal.

},
{
"description": " name matches https://www.w3.org/TR/REC-xml-names/#NT-NCName but excludes U+FFFD and U+061C",
Expand All @@ -124,17 +125,17 @@
{
"description": "name = [bidi] name-start *name-char [bidi]",
"src": ".local $\u200Efoo\u200F = {3} {{{$\u200Efoo\u200F}}}",
"exp": "3"
"exp": "\u20683\u2069"
},
{
"description": "name = [bidi] name-start *name-char [bidi]",
"src": ".local $foo = {4} {{{$\u200Efoo\u200F}}}",
"exp": "4"
"exp": "\u20684\u2069"
},
{
"description": "name = [bidi] name-start *name-char [bidi]",
"src": ".local $\u200Efoo\u200F = {5} {{{$foo}}}",
"exp": "5"
"exp": "\u20685\u2069"
},
{
"description": "name = [bidi] name-start *name-char [bidi]",
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/date.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Date function",
"description": "The built-in formatter for dates.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
},
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/datetime.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Datetime function",
"description": "The built-in formatter for datetimes.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
},
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/integer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Integer function",
"description": "The built-in formatter for integers.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US"
},
"tests": [
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/number.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Number function",
"description": "The built-in formatter for numbers.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US"
},
"tests": [
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/string.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "String function",
"description": "The built-in formatter for strings.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US"
},
"tests": [
Expand Down
1 change: 1 addition & 0 deletions test/tests/functions/time.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Time function",
"description": "The built-in formatter for times.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US",
"expErrors": false
},
Expand Down
1 change: 1 addition & 0 deletions test/tests/syntax.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "Syntax",
"description": "Test cases that do not depend on any registry definitions.",
"defaultTestProperties": {
"bidiIsolation": "none",
"locale": "en-US"
},
"tests": [
Expand Down
28 changes: 14 additions & 14 deletions test/tests/u-options.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"scenario": "u: Options",
"description": "Common options affecting the function context",
"defaultTestProperties": {
"bidiIsolation": "default",
"locale": "en-US"
},
"tests": [
Expand All @@ -24,7 +25,7 @@
"type": "markup",
"kind": "close",
"id": "x",
"name": "tag"
"name": "ns:tag"
}
]
},
Expand All @@ -45,7 +46,7 @@
{
"type": "markup",
"kind": "close",
"name": "tag"
"name": "ns:tag"
}
]
},
Expand Down Expand Up @@ -74,32 +75,31 @@
"src": "hello {world :string u:dir=rtl}",
"exp": "hello \u2067world\u2069",
"expParts": [
{
"type": "literal",
"value": "hello "
},
{ "type": "literal", "value": "hello " },
{ "type": "bidiIsolation", "value": "\u2067" },
{
"type": "string",
"source": "|world|",
"dir": "rtl",
"locale": "en-US",
"value": "world"
}
},
{ "type": "bidiIsolation", "value": "\u2069" }
]
},
{
"src": "hello {world :string u:dir=auto}",
"exp": "hello \u2068world\u2069",
"expParts": [
{
"type": "literal",
"value": "hello "
},
{ "type": "literal", "value": "hello " },
{ "type": "bidiIsolation", "value": "\u2068" },
{
"type": "string",
"source": "|world|",
"dir": "auto",
"locale": "en-US",
"value": "world"
}
},
{ "type": "bidiIsolation", "value": "\u2069" }
]
},
{
Expand All @@ -120,7 +120,7 @@
{
"locale": "ar",
"src": "أهلاً {بالعالم :string}",
"exp": "أهلاً \u2067بالعالم\u2069"
"exp": "أهلاً \u2068بالعالم\u2069"
}
Comment on lines 120 to 124
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may need to drop this test, or more explicitly define whether :string applies introspection to its value to determine its directionality. As we don't forbid introspection, an implementation could use either an RLI or FSI here.

Presuming that the introspection would follow UAX#9, the user's perception of the results would be the same, so my preference would be to make the character-level output predictable by requiring that no introspection is done in the absence of a u:dir override or an operand that includes an explicit direction.

]
}