-
-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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 whitespace conformance to match UAX31 (including permitting LRM/RLM) #673
Closed
Closed
Changes from 9 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
b387301
Allow LRM in whitespace
aphillips ae712d7
Update syntax.md
aphillips c556ecf
Make 3a-2 definition consistent with requirements
aphillips 8558eef
Update spec/syntax.md
aphillips d830550
Replace `[s]` with owsp production
aphillips 42b7744
`s` != `wsp`
aphillips af75420
Update syntax.md
aphillips 6a6dc70
Update message.abnf
aphillips db5e97c
Update spec/syntax.md
aphillips 3fede7a
Update spec/syntax.md
aphillips 586a4d6
Update spec/syntax.md
aphillips 502e1fe
Update spec/syntax.md
aphillips d7af5eb
Address literals
aphillips 68f32f8
Fix converting some `s` productions to `wsp`
aphillips e36d01d
Add bidi isolate support
aphillips 5609dc0
Missing one "x"
aphillips e74b34c
Add a warning/tech preview feedback note
aphillips 9557849
Update spec/syntax.md
aphillips ec369ec
Update spec/syntax.md
aphillips 2532206
Address @macchiati's comment
aphillips f985860
Update spec/syntax.md
aphillips 52c5c86
Update spec/syntax.md
aphillips 4a6c96c
Fix up @machiatti's suggested text
aphillips 19864c3
Address @mihnita's suggestion, fix expression brackets
aphillips 27b9e42
Make syntax.md make abnf
aphillips 00e3796
Merge branch 'main' into aphillips-allow-lrm
aphillips File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -905,24 +905,55 @@ backslash = %x5C ; U+005C REVERSE SOLIDUS "\" | |
|
||
### Whitespace | ||
|
||
**_<dfn>Whitespace</dfn>_** is defined as one or more of | ||
U+0009 CHARACTER TABULATION (tab), | ||
U+000A LINE FEED (new line), | ||
U+000D CARRIAGE RETURN, | ||
U+3000 IDEOGRAPHIC SPACE, | ||
or U+0020 SPACE. | ||
The syntax limits whitespace characters outside of a _pattern_ to the following: | ||
`U+0009 CHARACTER TABULATION` (tab), | ||
`U+000A LINE FEED` (new line), | ||
`U+000D CARRIAGE RETURN`, | ||
`U+3000 IDEOGRAPHIC SPACE`, | ||
or `U+0020 SPACE`. | ||
|
||
Inside _patterns_ and _quoted literals_, | ||
whitespace is part of the content and is recorded and stored verbatim. | ||
Whitespace is not significant outside translatable text, except where required by the syntax. | ||
|
||
There are two whitespace productions in the syntax. | ||
**_<dfn>Optional whitespace</dfn>_** is whitespace that is not required by the syntax, | ||
but which users might want to include to increase the readability of a _message_. | ||
**_<dfn>Required whitespace</dfn>_** is whitespace that is required by the syntax. | ||
|
||
Tools SHOULD generate `U+200E LEFT-TO-RIGHT MARK` or `U+200F RIGHT-TO-LEFT MARK` | ||
characters where permitted by the syntax before or following _identifiers_, | ||
_unquoted literals_, or _option_ values that use right-to-left characters | ||
so that the _message_ displays intelligibly. | ||
|
||
These definitions of _whitespace_ implement | ||
[UTR#31 Rule 3a-2](https://www.unicode.org/reports/tr31/#R3a-2). | ||
aphillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
It is a profile of R3a-1 in that specification because: | ||
the following pattern whitespace characters are not allowed: | ||
`U+000B FORM FEED`, | ||
`U+000C VERTICAL TABULATION`, | ||
`U+0085 NEXT LINE`, | ||
`U+2028 LINE SEPARATOR` and | ||
`U+2029 PARAGRAPH SEPARATOR`; | ||
the character `U+3000 IDEOGRAPHIC SPACE` | ||
_is_ interpreted as whitespace; | ||
and the following character is not included in ignorable format controls: | ||
aphillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`U+200F RIGHT-TO-LEFT MARK`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had suggested more thing, they are now obsolete.``` the following character is not interpreted as whitespace (in particular, it is not treated as an ignorable format control): `U+200F RIGHT-TO-LEFT MARK`; the ignorable format control U+200E LEFT-TO-RIGHT mark is only allowed in contexts UAX31-I1 and UAX31-I3, further restricted to the beginning of a nonempty sequence of horizontal spaces and line terminators. ```
You don’t, but the Unicode Standard does :-) |
||
|
||
> [!NOTE] | ||
> The character U+3000 IDEOGRAPHIC SPACE is included in whitespace for | ||
> compatibility with certain East Asian keyboards and input methods, | ||
> in which users might accidentally create these characters in a _message_. | ||
|
||
```abnf | ||
s = 1*( SP / HTAB / CR / LF / %x3000 ) | ||
; optional whitespace | ||
owsp = *( s / %x200E / %x200F ) | ||
|
||
; required whitespace | ||
wsp = [ (%x200E / %x200F) ] 1*s [ (%x200E / %x200F) ] | ||
|
||
; whitespace characters | ||
s = ( SP / HTAB / CR / LF / %x3000 ) | ||
aphillips marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
## Complete ABNF | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why there is the restriction on unquoted literals. Shouldn't it be any literal? That is, anywhere a unquoted literal can appear, and unquoted one can. So it seems like either both (aka just 'literal') or neither can appear.
I could match on:
Where X is a RTL character, or on
In both cases the result is jumbled (disregard the direction of the fake braces, the tool just reorders.
Now, in this case I could put LRMs before the first ⎸ and after the second, because both positions allow whitespace. Are there circumstances around literals where it makes a difference in the insertability of LRM/RLM because of the quoting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than that, the changes look good to me. Note that if there are any issues in the WG about this, we refrain from these changes until after the v45 release, just leaving a note that we're looking at the bidi ordering issues...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. The key thing I think is to remind tool writers not to quote the mark onto the value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are to the syntax and I think important enough to merit doing the change now--the better to stabilize the syntax. It does represent a relaxation of what is allowed in free whitespace. I would like to avoid having a lot of Tech Preview implementations reject bidi-friendlier messages in the fall.
OTOH, it does represent a departure from how we set up the
s
production.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, for a LTR reading, you want to put LRM around any 'element' that could contain RTL characters. Each literal being matched, literals in option values, etc. So in
You want to insert like:
Of course:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You definitely do not want the LRMs in the text part of the message, which is where the ones after the
{{
and before the}}
are. Instead there should be an LRM following the pattern so that any keys in the next variant aren't reordered:Agreed that this is a job for tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yes, imediately outside {{ and }}, not inside.