Closed Bug 922576 Opened 9 years ago Closed 9 years ago

Allow reordering HTML elements in translations

Categories

(L20n :: HTML Bindings, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file)

The work in bug 921169 made it impossible (or risky) to reorder elements in the translations.  We should consider allowing (from easiest to most difficult):

 1. reordering elements of different type: input a → a input,
 2. reordering elements of same type with an id: a#one a#two → a#two a#one (this introduces a dependency on developers who need to add ids),
 3. reordering elements of same type without additional work from developers: a a → a#l10n-path=a[2] a#l10n-path=a[1]
Can't it be done using `insertBefore`?
We already use insertBefore.  The problem is that I want to identify elements that need not be replaced, but also insert new elements.  Right now, I do this sequentially:  start from the first child in the source, find its equivalent in the translation, take all nodes up that equivalent from the translation and move them into source.  If source has two elements (input a), I'll start with input, find it in the translation (where the order is reversed: a input), take all nodes preceding input (a) and move them into source.  Then wehn I move on to 'a' in source, I don't find it in the translation anymore.

So doing this sequentially is limiting.

An alternative would be to sanitize translation's DOM first and then use translation.replaceChild to move the source element from source DOM to translation overlay it there, and then move the whole translation back into source.
Attached patch Allow reorderingSplinter Review
This patch implements the approach from comment 2 in order to allow the first 
type of the reordering mentioned in comment 0.  I should have done this like 
this from the beginning, it's much simpler and just as fast.

Settings, l20n-master, languages_dev, 30 iterations:

master: 1139
patch: 1143
Attachment #813869 - Flags: review?(gandalf)
Comment on attachment 813869 [details] [diff] [review]
Allow reordering

Review of attachment 813869 [details] [diff] [review]:
-----------------------------------------------------------------

sweet work Stas!
Attachment #813869 - Flags: review?(gandalf) → review+
Assignee: nobody → stas
Comment on attachment 813869 [details] [diff] [review]
Allow reordering

Michał, would you mind taking a look and seeing if this still works with Angular?
Attachment #813869 - Flags: feedback?(m.goleb+mozilla)
Stas: your patch generates a massive amount of errors to the console in Polona. There are two kinds of errors, here are tracebacks: http://pastebin.com/z5WSMMQq
Michał:  are these really related to my patch?  I think they might be caused by the bug we discussed on IRC last week about _source being null instead of '' when the resource download fails (I filed bug 924071 for this).
Flags: needinfo?(m.goleb+mozilla)
(In reply to Staś Małolepszy :stas from comment #7)
> Michał:  are these really related to my patch?  I think they might be caused
> by the bug we discussed on IRC last week about _source being null instead of
> '' when the resource download fails (I filed bug 924071 for this).

That might be true, your patch might have just made these bugs more explicit. However, that's what I see after applying your patch, I'm not saying it's your fault, I only said what happened. ;)
Flags: needinfo?(m.goleb+mozilla)
Apart from these errors, it seems to work fine.
@stas Ah, sorry, those errors are already present on master, some other changes had to introduce them. :/
(In reply to Staś Małolepszy :stas from comment #0)

>  1. reordering elements of different type: input a → a input,

Fixed in https://github.com/l20n/l20n.js/commit/97ecf63377a789aeaff2f08b3e366fbbc47c4fd6

>  2. reordering elements of same type with an id: a#one a#two → a#two a#one
> (this introduces a dependency on developers who need to add ids),
>  3. reordering elements of same type without additional work from
> developers: a a → a#l10n-path=a[2] a#l10n-path=a[1]

If we get to implementing #2 and #3, we should file new bugs.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #813869 - Flags: feedback?(m.goleb+mozilla) → feedback+
You need to log in before you can comment on or make changes to this bug.