zbraniecki / fluent-domoverlays-js

Fluent DOM Overlays (JS)
1 stars 0 forks source link

Verify that stakeholders agree on the current master #13

Open zbraniecki opened 5 years ago

zbraniecki commented 5 years ago

This issue is devoted to ensure that we agree on the subset of possible revision 3 features and the general direction of the code base.

I separated all disputable items to separate issues and PRs.

What's in master now implements everything Revision 2 did (it passes fluent-dom overlay tests) plus:

It has good performance as per #12, and should be easy to read (I'll document the code).

My biggest open question is about whether we can agree on Item 3 and Item 7.

Item 7 seems to be less controversial from what I understand, in the sense that if the element was in the DOM and the translation keeps it there, we should retain the identity of the element rather than recreating it and removing the old & inserting the new.

But without Item 3 that still means that if the localization doesn't provide the element, we'll remove it from the DOM during translation and the developer cannot and should not assume that an element within a DOM Fragment is going to be there. That's especially tangible in context of Item 9 (and #2) because that feature will enable localizers to add/remove elements from the DOM and in result we will not be able to use the fallback mechanism proposed in 3 (to retain all unaccounted for elements at the end of the fragment).

@gijsk, @msujaws, @stasm, @flodolo, @delphine - can we work toward some shared baseline to work with?

Pike commented 5 years ago

I have doubts about Item 3:

As a developer, I want to be sure that a translation will not remove a child element which is important for the functioning of the UI.

I guess my concern is that I doubt that every node in the original DOM is important for the UI to function. Some I totally agree with, and IMHO they should have a data-l10n-name on 'em.

I'm also concerned about the way the current code recovers from that error. Just adding stuff to the end, I can contrive so many examples where that goes wrong. Including but not limited to a 5 child elements example with 4 language switches and 5 different languages in the UI. I did say contrive.

Pike commented 5 years ago

Some thoughts on Item 7, retain element identity: I'd be OK with that not being guaranteed. I'm running on the assumption that DOM attributes and event handlers on the l10n container have us covered, or even custom elements. I don't think that dropping element identity raises hard problems.

Historical note, in v2 of dom fragments, stas chose to recreate elements always, to avoid surprises around event listeners working or not in some cases. I personally don't feel strongly on whether this is a good choice or not.

gijsk commented 5 years ago

FWIW, I'm not a fan of data-l10n-name being required generally. Feels like a dedicated attribute is good for cases where there isn't one, but really id without an additional data-l10n-name should also be sufficient to guarantee nodes stay around, and is conveniently a lot shorter and less cumbersome to type and more 'natural' than data-l10n-name.

I think if possible we should maintain DOM node identities and not disappear (non-inline-text) child elements unnecessarily. I can't really think of non-contrived examples where that'd present problems. If/when it does we should consider them as/when they come up, I think.

Pike commented 5 years ago

but really id without an additional data-l10n-name should also be sufficient to guarantee nodes stay around

Filed #14 on that.

zbraniecki commented 5 years ago

I'm also concerned about the way the current code recovers from that error. Just adding stuff to the end, I can contrive so many examples where that goes wrong. Including but not limited to a 5 child elements example with 4 language switches and 5 different languages in the UI. I did say contrive.

Can you elaborate on that example? I'm not sure if I understand and I think it's important to have an example of why this may not be what we want.

p.s. In the current code, I leave elements at the end, not add, but that's a subtle difference, just wanted to make it clear in case it may matter in our conversation :)

Pike commented 5 years ago

I've whacked the thought experiment up on https://gist.github.com/Pike/7a917cb897455dbf6dc139275d4f4fa8

zbraniecki commented 5 years ago

I see what's going on in your experiment, but I don't think I understand the use case. When would it happen that a localization should affect the DOM structure - in your case the number of <div/> elements?

My position at the moment is that there should be no case like that, and developer is the only person deciding on the number of <div> elements while the localizer populates them with translation (optionally, reordering them within the same parentNode).

This model allows us to provide the Item 3 and Item 7.

mozfreddyb commented 5 years ago

Regarding item 3, I think it makes sense to agree on a contract to guarantee that specific nodes must still exist after localization (apparently that's id or data-l10n-name).

I've come here to suggest we error out and ensure there's early validation for translations that are violationg this rather than trying to fix it in fluent. @zbraniecki mentioned there are stronger errors in nightly/beta, but we try to repair things in product to not punish our users, which makes some sense.

But I do wonder if there's something that could be done in our community translation process, even before stuff lands in a langpack. There is some sort of validation and automated review, no?

Pike commented 5 years ago

I've come here to suggest we error out and ensure there's early validation for translations that are violationg this rather than trying to fix it in fluent.

I'm all for finding and reporting problems early. Which is also why I lobby to have something with limited bells and whistles, so that it's implementable not just on the domoverlays side, but also on build and editing toolchains.

That said, we can't rely on those early reports at runtime. Firefox language packs contributed by the community don't go through our build checks at all. Also, non-gecko projects don't go through any build checks.

zbraniecki commented 5 years ago

Regarding item 3, I think it makes sense to agree on a contract to guarantee that specific nodes must still exist after localization (apparently that's id or data-l10n-name).

The proposal in Item 3 and at least some stakeholders claim that all functional elements (as in, non text level elements) should be retained, without requiring an additional attribute to be put on them (Item 4 #5 ).