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.

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

l10n: adds Localizable dict + TextDirection enum #455

Closed
wants to merge 1 commit into from

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Mar 14, 2017


Preview | Diff

@marcoscaceres
Copy link
Member Author

WIP, seeking feedback. It's just missing a statement about how to use the information when presenting the strings in a UI.

index.html Outdated
@@ -1108,7 +1108,7 @@
<dfn>PaymentDetailsBase</dfn> dictionary
</h2>
<pre class="idl">
dictionary PaymentDetailsBase {
dictionary PaymentDetailsBase : Localizable {
Copy link
Member Author

Choose a reason for hiding this comment

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

I want this to apply to PaymentDetailsUpdate.error, which is why put it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it should go here if it only applies to PaymentDetailsUpdate. Instead you're going to have to just duplicate the two things (or use a partial dictionary PaymentDetailsUpdate).

If you put it here then PaymentDetailsInit gets it, and in particular that means passing { get dir() { throw new Error("boo"); } to places that expect a PaymentDetailsInit must throw.

index.html Outdated
member that will be displayed to the user. For example, this might
commonly be used to explain why goods cannot be shipped to the chosen
shipping address.
<a>PaymentDetailsUpdate</a> can contain a human-readable message in
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using "human-readable" as the key that binds together all these "localizable" members. Need to formally define "human-readable" as being potentially affected by dir/lang when present.

Copy link
Member Author

Choose a reason for hiding this comment

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

("human-readable" was already in use in the spec, which is why I reused it - maybe we can come up with something nicer)

@domenic
Copy link
Collaborator

domenic commented Mar 14, 2017

Would be good to land after #451 to avoid conflicts.

Notably this does not allow serving up multiple options with different locales and allowing the browser to choose between them. I guess that is a non-goal.

It's also noteworthy that this matches NotificationOptions already, so that's good. Maybe we can consolidate this Localizable type into Web IDL or similar. /cc @annevk @beverloo @tobie. Notifications defaults lang to "" BTW to represent, I guess, no language selected. That makes sense.

Also, this standard says it represents a BCP 47 language tag, but that's not quite accurate; a developer could pass in "asdf" and there's no normative text rejecting that. You'll instead want something similar to the verbiage at https://notifications.spec.whatwg.org/#language.

@marcoscaceres
Copy link
Member Author

Notably this does not allow serving up multiple options with different locales and allowing the browser to choose between them. I guess that is a non-goal.

Yeah. Non-goal I think.

It's also noteworthy that this matches NotificationOptions already, so that's good. Maybe we can consolidate this Localizable type into Web IDL or similar. /cc @annevk @beverloo @tobie.

I was thinking the same.... would be great to have this in Web IDL. Maybe we can craft Localizable here first, and then push it to WebIDL?

Notifications defaults lang to "" BTW to represent, I guess, no language selected. That makes sense.

Will fix.

Also, this standard says it represents a BCP 47 language tag, but that's not quite accurate; a developer could pass in "asdf" and there's no normative text rejecting that. You'll instead want something similar to the verbiage at https://notifications.spec.whatwg.org/#language.

Yeah, that's some good wording.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

Uplifting to IDL seems reasonable. We already put some shared concepts and processing models there.

@tobie
Copy link
Member

tobie commented Mar 14, 2017

Yeah, seems this could go in §4 Common definitions.

@ianbjacobs
Copy link
Collaborator

@domenic wrote:

"Notably this does not allow serving up multiple options with different locales and allowing the browser to choose between them."

If I understand your comment, it is about support for multiple strings in multiple languages.
A long time back we discussed whether the API needed to support this. Our conclusion at the time was that it did not, because people could adjust content based on global directives like HTTP language negotiation.

(If you were talking about something else; sorry for the noise.)

Ian

@domenic
Copy link
Collaborator

domenic commented Mar 14, 2017

Yeah, it makes sense. So this is mostly about allowing people to markup certain messages as having a different lang/dir than the rest of the content on the page.

index.html Outdated
<dfn>auto</dfn>
</dt>
<dd>
Directionality is determined by the [[!BIDI]] algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

The bidi algorithm should be used for ltr and rtl as well... right?

I think we basically want to hook into http://www.unicode.org/reports/tr9/#Explicit_Directional_Isolates similarly to what CSS does in https://drafts.csswg.org/css-writing-modes-3/#unicode-bidi (for isolate or plaintext -- not sure which is more appropriate here).

Copy link
Member

Choose a reason for hiding this comment

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

https://notifications.spec.whatwg.org/#direction has the text you need for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will try to draft it soon.

@zcorpan
Copy link
Member

zcorpan commented Mar 15, 2017

cc @w3c/i18n-reviewers

index.html Outdated
@@ -1108,7 +1108,7 @@
<dfn>PaymentDetailsBase</dfn> dictionary
</h2>
<pre class="idl">
dictionary PaymentDetailsBase {
dictionary PaymentDetailsBase : Localizable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it should go here if it only applies to PaymentDetailsUpdate. Instead you're going to have to just duplicate the two things (or use a partial dictionary PaymentDetailsUpdate).

If you put it here then PaymentDetailsInit gets it, and in particular that means passing { get dir() { throw new Error("boo"); } to places that expect a PaymentDetailsInit must throw.

index.html Outdated
</pre>
<p>
The text-direction values are the following, implying that the value of
the human-readable members is by default:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence doesn't make sense to me.

@marcoscaceres
Copy link
Member Author

blocked on whatwg/webidl#358

@marcoscaceres
Copy link
Member Author

argh, that's not right. Seems I screwed up a rebase.

@marcoscaceres
Copy link
Member Author

better...

@marcoscaceres
Copy link
Member Author

Still blocked on discussions over in the WebIDL repo. Probably take another few days to resolve those.

@@ -2810,6 +2810,10 @@
</dt>
<dd>
<p>
The <code><dfn>LocalizableString</dfn></code> <a data-cite=
"!WEBIDL-LS#dfn-typedef">typedef</a> is defined by [[!WEBIDL-LS]].
Copy link
Member

Choose a reason for hiding this comment

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

You can go ahead and just use [[WEBIDL]], btw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix globally! Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

[[WEBIDL]] is informative reference, [[!WEBIDL]] is normative reference.

Copy link
Member

Choose a reason for hiding this comment

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

I meant WEBIDL vs WEBIDL-LS, sorry if that caused confusion. :(((

Copy link
Member Author

Choose a reason for hiding this comment

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

It's all good. I sent #524

@marcoscaceres marcoscaceres changed the title l10n: adds Localizable dict + TextDirection enum (closes #327) l10n: adds Localizable dict + TextDirection enum May 15, 2017
@marcoscaceres
Copy link
Member Author

Closing for now... will reopen if we want to add this in v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add dir/lang member on dictionaries whose content is displayed in browser UI
6 participants