-
-
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
Conversation
This partially addresses #661 by allowing the LRM character in message whitespace. This is whitespace **_outside_** pattern `text`. Tools can use this to help ensure that messages are formatted visually in a way consistent with LTR presentation of a message.
@eggrobin For review |
spec/syntax.md
Outdated
This definition of _whitespace_ implements | ||
[UTR#31 Rule 3a-2](https://www.unicode.org/reports/tr31/#R3a-2). | ||
It is a profile of R3a-1 in that specification because only the | ||
whitespace characters listed are permitted as whitespace. |
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.
UAX31-R2a-2 says you need to
define that profile with a precise specification of the characters that are added to or removed from the set of code points defined by the Pattern_White_Space property, and of any changes to the criteria under which a character or sequence of characters is interpreted as an end of line, as ignorable format controls, or as horizontal space.
The point is that the reader of such a conformance statement sees the difference from the default, which are the things that may need special attention when interoperating with an implementation based on the defaults.
So, if I am reading this right:
- Form feed, next line, line separator, paragraph separator, and right-to-left mark are removed from the set of white space characters;
- ideographic space is added to the set of white space characters;
- the sole ignorable format control (LRM) is allowed only in contexts UAX31-I1 and UAX31-I3 (not UAX31-I2), where UAX31-I1 is further restricted to the beginning of a sequence of horizontal spaces, and UAX31-I3 is further restricted to only the end and not the beginning of a line.
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.
further restricted to only the end and not the beginning of a line
Also it is disallowed on a blank line, I think?
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.
Thanks @eggrobin. Technically, we don't have lines. Message whitespace can be normalized to a single space (in cases where whitespace is required) or to nothing (in cases where the whitespace is optional).
Your reading is correct. Note that UAX31-I2 is not permitted because that would break our sigil-identifier syntax. In all of the other cases in our syntax, there is required whitespace.
Note that quoted literals or pattern text can contain bidi controls that might cause Trojan source effects unless/until we address placeholder isolation.
I can correct the description to list the differences.
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.
Note that quoted literals or pattern text can contain bidi controls that might cause Trojan source effects unless/until we address placeholder isolation.
Mostly that is something to be dealt with at a higher level (in editors and tooling); the main thing languages need to do is to treat the right things in the right way so that standardized tooling can deal with the issue, see UTS55.
Note that UAX31-I2 is not permitted because that would break our sigil-identifier syntax.
I don’t really understand what you mean here; UAX31-I2 does not say you should allow $⟨LRM⟩identifier
, it says that you should allow it if you allow $ identifier
.
The reason why you do not have UAX31-I2 is that you do not allow an LRM wherever you have [s]
, so for instance in
message-format-wg/spec/message.abnf
Lines 35 to 36 in ae712d7
markup = "{" [s] "#" identifier *(s option) *(s attribute) [s] ["/"] "}" ; open and standalone | |
/ "{" [s] "/" identifier *(s option) *(s attribute) [s] "}" ; close |
you do not allow {⟨LRM⟩#whatever⟨LRM⟩}
, only {⟨LRM⟩ #whatever⟨LRM⟩ }
, right?
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 do not allow {⟨LRM⟩#whatever⟨LRM⟩}, only {⟨LRM⟩ #whatever⟨LRM⟩ }, right?
Note that this probably throws a wrench into the UTS55 conversion to plain text algorithm.
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.
Yes. See my response to @gibson042's comment.
spec/syntax.md
Outdated
and the following character is not included in ignorable format controls: | ||
`U+200F RIGHT-TO-LEFT MARK`. |
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 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. ```Technically, we don't have lines
You don’t, but the Unicode Standard does :-)
spec/syntax.md
Outdated
This definition of _whitespace_ implements | ||
[UTR#31 Rule 3a-2](https://www.unicode.org/reports/tr31/#R3a-2). | ||
It is a profile of R3a-1 in that specification because only the | ||
whitespace characters listed are permitted as whitespace. |
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.
Note that quoted literals or pattern text can contain bidi controls that might cause Trojan source effects unless/until we address placeholder isolation.
Mostly that is something to be dealt with at a higher level (in editors and tooling); the main thing languages need to do is to treat the right things in the right way so that standardized tooling can deal with the issue, see UTS55.
Note that UAX31-I2 is not permitted because that would break our sigil-identifier syntax.
I don’t really understand what you mean here; UAX31-I2 does not say you should allow $⟨LRM⟩identifier
, it says that you should allow it if you allow $ identifier
.
The reason why you do not have UAX31-I2 is that you do not allow an LRM wherever you have [s]
, so for instance in
message-format-wg/spec/message.abnf
Lines 35 to 36 in ae712d7
markup = "{" [s] "#" identifier *(s option) *(s attribute) [s] ["/"] "}" ; open and standalone | |
/ "{" [s] "/" identifier *(s option) *(s attribute) [s] "}" ; close |
you do not allow {⟨LRM⟩#whatever⟨LRM⟩}
, only {⟨LRM⟩ #whatever⟨LRM⟩ }
, right?
spec/message.abnf
Outdated
; Whitespace | ||
s = 1*( SP / HTAB / CR / LF / %x3000 ) | ||
; Whitespace, optionally prepended with LRM | ||
s = [%x200E] 1*( SP / HTAB / CR / LF / %x3000 ) |
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.
This means that LRM is not valid after a line terminator or when not followed by a whitespace character, e.g. before the quoted patterns in a message like
.match {$count :number}
one
{{You have {$count} notification.}}
*
{{You have {$count} notifications.}}
or
.match {$count :number}
one
{{You have {$count} notification.}}
*
{{You have {$count} notifications.}}
Is that intentional?
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.
No, not really.
Probably what needs to happen here is a distinguishing of optional and non-optional whitespace. Everywhere we have [s]
should use a production that can be just LRM (or RLM, fwiw) or nothing, e.g.:
; optional whitespace
owsp = *( SP/ HTAB / CR / LF / %x3000 / %x200E / %x200F )
And everywhere that requires positive whitespace (i.e. just s
) permit controls to either side:
; required whitespace
wsp = [ (%x200E / %x200F) ] 1*( SP / HTAB / CR / LF / %x3000) [ (%x200E / %x200F) ]
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 think that would be an improvement (and then you can drop the convoluted explanation from the conformance statement, and the profile limits itself to changing the sets of characters).
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 did the hard change. We now need to get WG approval.
Co-authored-by: Robin Leroy <[email protected]>
Co-authored-by: Robin Leroy <[email protected]>
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 conformance statement seems good now.
There may be a discussion to be had as to whether the profile should be made smaller (in particular in the line break area discrepancies could lead to spoofing concerns), but this is probably not urgent.
In any case this clear conformance statement should facilitate CLDR-TC’s job when it comes to review—at least I should hope it will help the chair, who is my co-editor on UAX #31 :-)
Co-authored-by: Robin Leroy <[email protected]>
spec/syntax.md
Outdated
|
||
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 |
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:
X y ⎨⎨$count⎬⎬
Where X is a RTL character, or on
⎸X⎸ y ⎨⎨$count⎬⎬
In both cases the result is jumbled (disregard the direction of the fake braces, the tool just reorders.
⎬ ⎬ y ⎨ ⎨ $ c o u n t X
⎬ ⎬ y ⎨ ⎨ $ c o u n t ⎸ X ⎸
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.
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...
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
{{STUFF {$value option=|JUNK| ...} TO READ}}
You want to insert like:
{{<LRM>STUFF {<LRM>$value option=<LRM>|JUNK|<LRM> ...<LRM>} TO READ<LRM>}}
Of course:
- With LRM/RLM you can't get the the RTL message parts to reorder around the {$value}, but the order is predictable and far better than the raw message.
- in practice you want tooling to do this, not humans.
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.
{{<LRM>STUFF {<LRM>$value option=<LRM>|JUNK|<LRM> ...<LRM>} TO READ<LRM>}}
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:
<LRM>KEY1<LRM> KEY2<LRM> {{STUFF {<LRM>$value option=<LRM>|JUNK|<LRM>...<LRM>} TO READ}}
<LRM>key1<LRM> key2<LRM> {{ ... next variant...}}
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.
While a departure, I think it is cleaner than before....
…On Tue, Feb 20, 2024 at 10:36 AM Addison Phillips ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/syntax.md
<#673 (comment)>
:
>
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
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...
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.
—
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMCY2JTR2URWSKJV3DDYUTUMRAVCNFSM6AAAAABDP5KVM6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJRGIYTONJZGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I'm a little puzzled about the explicit choice made here of only allowing RLM and LRM, as opposed to other directional formatting characters. Why is that preferable here to LRI/RLI/FSI/PDI, which we're exclusively using in our default bidi isolation strategy?? I've not spent very long (yet...) with bidi concerns, but one aspect that I'm concerned with is the implementation and understanding of the recommendations added by this change. To me, isolates seems more in line with the shape of MF2 syntax and its nestings of code and message text. As a programmer, they're also somewhat easier to reason about as their effects are more direct and, well, isolated. I'm also a bit concerned about the effects that our allowance for RTL names and identifiers may have, esp. when they are mixed in with LTR names and identifiers, and our general allowance for newlines in whitespace. Rather than allowing directional formatting characters in any/all whitespace, could we be much more restrictive about which characters we allow, and where? And also perhaps include some explicit guidance about the preferred rendering order for syntax, such as the LTR order within expressions of operand > function name > options > attributes? For instance, would it be appropriate to only allow the following?
The intent with the above would be to ensure that it's possible to have a valid message for which the "code" portions always have an LTR paragraph direction, while allowing for all user-customizable strings to define their own direction. |
Co-authored-by: Robin Leroy <[email protected]>
Co-authored-by: Mark Davis <[email protected]>
Looks great. Will approve once I'm back at my computer
…On Wed, Feb 21, 2024, 12:08 Addison Phillips ***@***.***> wrote:
@aphillips <https://github.com/aphillips> requested your review on: #673
<#673> Fix
whitespace conformance to match UAX31 (including permitting LRM/RLM).
—
Reply to this email directly, view it on GitHub
<#673 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJLEMCFWF4UXF4P4SCKXSTYUZH43AVCNFSM6AAAAABDP5KVM6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRHA4DCNJZGA3DSMQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***
com>
|
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.
Thank you!
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.
Quoting @macchiati from #673 (comment):
It is quite tricky, and we should not derail the tech preview release for
this. That's why I (more strongly) urge that we capture this issue in a
note in the spec for tech preview, and make the fix afterwards.
I am not comfortable landing this in the timeframe required for LDML 45. I think we need to take the time to consider and address this properly, rather than rushing through a solution right now.
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 am split about this.
Looks OK, but incomplete.
We should also look at isolates, and it is a bit too close to deadline.
@mihnita noted:
Can you clarify? What is incomplete? Also, this uses isolates, so your last comment is mysterious to me. I agree that the deadlines are an issue. My concern here is that (a) we are a Unicode WG and this is a Unicode set of requirements. Even if we don't include it in 45, we need to deal with it and (b) syntax stability is important to me. Permitting bidi controls now in a somewhat (but not entirely!!) loose manner will prevent unnecessary churn later. Anyway, to discuss in a few minutes in our call 😉 |
WG will consider in post-45. @aphillips to create a 45-timed PR with a note. |
The working group discussed accepting #673 for the tech preview. Because this was a late-breaking change, the group decided to work on incorporating work on bidi and UAX31 conformance in the early post-45 period. I was tasked with creating a PR with a note about bidi for the Tech Preview specifically. This note is adapted from text proposed in #673.
The working group discussed accepting #673 for the tech preview. Because this was a late-breaking change, the group decided to work on incorporating work on bidi and UAX31 conformance in the early post-45 period. I was tasked with creating a PR with a note about bidi for the Tech Preview specifically. This note is adapted from text proposed in #673.
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.
Looks good. Added a minor suggestion.
> - put them outside of _literal_ quotes, such as `<LRM>|...|<LRM>` | ||
> - put them outside of quoted _patterns_, such as `<LRI>{{...}}<PDI>` |
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 suggest adding words Just to make it clear what the list is telling people to not do (vs. whether they are supposed to do it). Also, I think you want a newline to ensure that the following sentences of the note don't get collapsed into the second bullet point.
> - put them outside of _literal_ quotes, such as `<LRM>|...|<LRM>` | |
> - put them outside of quoted _patterns_, such as `<LRI>{{...}}<PDI>` | |
> - do not put them outside of _literal_ quotes, such as `<LRM>|...|<LRM>` | |
> - do not put them outside of quoted _patterns_, such as `<LRI>{{...}}<PDI>` | |
> |
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.
Good point. I am making changes to this text as I incorporate it into a new PR that replaces this one and implements the bidi design.
Closing as obsolete |
This partially addresses #661 by allowing the LRM character in message whitespace. This is whitespace outside pattern
text
.Tools can use this to help ensure that messages are formatted visually in a way consistent with LTR presentation of a message.