projectfluent / fluent.js

JavaScript implementation of Project Fluent
https://projectfluent.org/
Apache License 2.0
928 stars 77 forks source link

Allow for elements to be preserved when localizing a fragment #169

Open zbraniecki opened 6 years ago

zbraniecki commented 6 years ago

In https://bugzilla.mozilla.org/show_bug.cgi?id=1445084 we have a case of a DOM Fragment where we'd like to leave an element untouched when localizing the fragment its a part of:

<p data-l10n-id="search-results-sorry-message">
  <span id="query" data-l10n-static></span>
</p>
search-results-sorry-message = Sorry! There are no results in Options for “</span>”.

This would allow localization to work independently from data updates to the span#query element and avoid retranslation on every character changed in the query.

stasm commented 6 years ago

Yes, I agree that we should allow this somehow. I also see how this would be useful in fluent-react. Even if the exact API will end up being different I think it's useful to consider it as well for the purpose of the design.

As far as I understand it, the goal here is to allow some of the named elements to be blackboxes: the translation can move them around to make it look right in the sentence but it should not be allowed to modify their subtrees.

We should make it possible for developer to protect the named element in such scenarios, so that it doesn't have its children removed by the translation's textContent. At the same time, I think it should still be possible to translate their attributes as per the default allowed list or the data-l10n-attrs attribute defined by the developer on the named element. For this reason, I'd suggest data-l10n-attrsonly as the name of the protecting attribute.

The full example would look as follows:

<p data-l10n-id="search-results-sorry-message">
  <span id="query" data-l10n-name="query" data-l10n-attrsonly></span>
</p>
search-results-sorry-message = Sorry! There are no results in Options for “<span data-l10n-name="query"/>”.
stasm commented 6 years ago

data-l10n-attrsonly would also be useful when defined on elements with data-l10n-id:

<window data-l10n-id="window" data-l10n-attrsonly>
    <!-- The whole app, really -->
</window>
window =
    .title = Window Title
zbraniecki commented 6 years ago

I have to say I really dislike attrsonly name and scope. The name feels very quirky and hacky, and the functionality seems very one-off and limited. Why value only? Why preserving attrs is not an option?

I'd suggest we go for something like "this element is not to be localized" sth like data-l10n-static or data-l10n-skip or data-l10n-preserve.

stasm commented 6 years ago

We should extend data-l10n-attrs to allow protecting attributes.

zbraniecki commented 6 years ago

so, in order to say, "don't touch " you'd have to do <menulist data-l10n-attrsonly data-l10n-attrs="-title,-placeholder,-src,-tooltip,-class,-style,...">? I don't think that's the right UX

stasm commented 6 years ago

The default list of localizable attributes would apply here:

https://github.com/projectfluent/fluent.js/blob/de35e58b9e8499119decbce6415c46599ed81ac0/fluent-dom/src/overlay.js#L24-L48

zbraniecki commented 6 years ago

The default list of localizable attributes would apply here:

Which doesn't resolve anything because a user has to plan for us changing that whitelist, or the whitelist is unchangable without breaking in a very hard to debug ways.

stasm commented 6 years ago

I don't understand this concern. Can you elaborate, please? Surely it solves something: in an unlikely event that you'd like to protect the title attribute, this solution allows you to do so:

<window
    data-l10n-id="window"
    data-l10n-attrsonly
    data-l10n-attrs="-title"
>
    <!-- The whole app. -->
</window>
zbraniecki commented 6 years ago

Instead of <window> let's take <moz-smartlist> Custom Element.

Over the life cycle you have to maintain the list of attributes that can affect the moz-smartlist element experience and the current whitelist of HTML attributes in Fluent, combine them and keep that list up to date listed in data-l10n-attrs as disabled to achieve the simple state - "Make Fluent not touch this element".

I find this UX unnecessary complicated and can't see any value in this approach over data-l10n-skip.

stasm commented 6 years ago

If it's a custom element, then it's not present in fluent-dom's default list, is it? In which case only the global attributes are allowed, i.e. title and aria-*.

When you say "over the life cycle" -- whose life cycle do you mean? When you say "you have to maintain" -- who is "you"?

zbraniecki commented 6 years ago

In which case only the default attributes are allowed, i.e. title and aria-*.

At the moment, yes. But I'm don't think we can guarantee this list will not change in a year or two.

When you say "over the life cycle" -- whose life cycle do you mean?

The app life cycle. They deploy Fluent 1.0 and moz-smartlist, then they keep updating moz-smartlist and they keep updating Fluent.

When you say "you have to maintain" -- who is "you"?

The maintainer of the app that uses Fluent and moz-smartlist. If moz-smartlist 3.0 adds a new attribute, the user has to add it to data-l10n-attrs on every element they don't want to be localized, if Fluent 2.0 adds placeholder to global attrs, the maintainer had to add it to data-l10n-attrs.

They don't need per-attribute granularity. They just want the element to not be touched by Fluent.

Stas - can you please provide reasoning aginst a single attribute solve a single use case, rather than a mesh of attributes to solve the same use case?

stasm commented 6 years ago

If someone updates to Fluent 2.0, they'd better read the changelog. I 100% expect them to have to update their code.

My reasoning is based on my attempt to have the same API for elements localized via data-l10n-id (let's call them parents) and for elements localized by DOM overlays via data-l10n-name (let's call them children). Both parents and children need some kind of protection from translation nuking their contents. Attributes are already protected because we only allow a small set of them. Values are not. I suggested attrsonly similar to the readonly attribute.

I'm trying to solve this for both parents and children at the same time to keep the API consistent.

At the same time I propose that a child's attributes are not part of the blackbox. Just as the localizer has the freedom to move the child around inside of the translation to satisfy the grammar of their language, they should also be able to add localizable attributes allowed by default by fluent-dom. More concretely, a localizer should be able to move an <img> element around and always add alt to it. Or to move a <span> around and always add title to it, because it may only make the translation better.

zbraniecki commented 6 years ago

I agree with the first part of your reasoning. I don't agree with the proposal to treat child's attributes as not part of the blackbox.

stasm commented 6 years ago

So, if I understand correctly, you agree that we need data-l10n-attrsonly on parent elements, but you don't agree to use the same attribute to protect child elements, because you'd like to protection to extend to both their value and their attributes. And you don't agree that the current protection of child attributes is good enough?

Pike commented 5 years ago

This has come up again in the light of bug 1498444, where we need a pre- and post-label around a drop-down with localized entries.

There are two issues here, AFAICT:

  1. We drop data-l10n-name elements if they're not in the localization. And thus clone from l10n to target.
  2. We want to protect the elements in some cases, so that their content dom can be complex, and independently localized.
  3. We want to localize the text content of some named elements, for things like links etc.

Early in this issue, stas asserted that we want to modify attributes on named elements. I wonder if we really want that? Also, is dropping the DOM node when there's no named element in the translation really the right choice? Would just appending to the end be just as good/better?

zbraniecki commented 5 years ago

Another example where the developer has to jump through hoops to workaround this bug - https://phabricator.services.mozilla.com/D17731

zbraniecki commented 5 years ago

Another example where the developer has spent multiple days debugging a very tricky to recognize consequence of the DOMOverlays behavior - https://bugzilla.mozilla.org/show_bug.cgi?id=1576609

In this case, the reason it was so tricky is because he cached the reference to the element, so the element still existed even after we removed it from the tree and replaced with a new one. During debugging, the developer was able to console.log the cached element and it looked identical to the element attached to the tree except that any manipulation on it wasn't reflected to the tree anymore. That mislead them to think that localization is somehow removing the attribute, rather than that we're removing the functional element from the tree.

Rob--W commented 1 year ago

Another set of examples: https://bugzilla.mozilla.org/show_bug.cgi?id=1835559#c2