Closed Bug 1453480 Opened 6 years ago Closed 6 years ago

Update fluent to 0.6.4 and fluent-dom to 0.2.0

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1453480 - Update fluent to 0.6.4 and fluent-dom to 0.2.0.
59 bytes, text/x-review-board-request
stas
: review+
Details
Bug 1453480 - Migrate Fluent resources to use DOM Overlays v2.
59 bytes, text/x-review-board-request
flod
: review+
Details
Bug 1453480 - Update Preferences to use fluent-dom 0.2.0 API.
59 bytes, text/x-review-board-request
stas
: review+
Details
Bug 1453480 - Update Node.localize API to renamed attrs->attributes in fluent-dom 0.2.0.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
Changelog:

fluent 0.6.4 (April 11, 2018)

    Minor optimization to bidirectionality isolation
    Added error logging when attempting an already registered message id

fluent-dom 0.2.0

    DOM Overlays v2 (#168) Major refactor of DOM Overlays allowing developers to provide node elements as attributes.
    Refactored error reporting (#160)
    Fixed a minor bug which caused an extranous retranslation when data-l10n-id was being removed from an element.


We care especially for DOM Overlays v2, but will be nice to get the other fixes as well.
Assignee: nobody → gandalf
Blocks: 1415730
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8967155 [details]
Bug 1453480 - Update fluent to 0.6.4 and fluent-dom to 0.2.0.

https://reviewboard.mozilla.org/r/235808/#review241610


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: intl/l10n/DOMLocalization.jsm:97
(Diff revision 1)
> -      templateElement.innerHTML = value;
> -      targetElement.appendChild(
> -        // The targetElement will be cleared at the end of sanitization.
> -        sanitizeUsing(templateElement.content, targetElement)
>        );
> +      templateElement.innerHTML = value;

Error: Unsafe assignment to innerhtml [eslint: no-unsanitized/property]

::: intl/l10n/DOMLocalization.jsm:655
(Diff revision 1)
>        return frag.localize(getTranslationsForItems.bind(this))
>          .then(untranslatedElements => {
>            for (let i = 0; i < overlayTranslations.length; i++) {
>              if (overlayTranslations[i] !== undefined &&
>                  untranslatedElements[i] !== undefined) {
>                overlayElement(untranslatedElements[i], overlayTranslations[i]);

Error: 'overlayelement' is not defined. [eslint: no-undef]
The second patch is not yet finished, as it'll require string ID changes. It would be useful to have ftl->ftl migration here to be able to avoid having to make localizers pay the price, but maybe we can save ourselves and migrate from dtd/properties since all those strings should be still in gecko-strings.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8967155 [details]
Bug 1453480 - Update fluent to 0.6.4 and fluent-dom to 0.2.0.

https://reviewboard.mozilla.org/r/235808/#review241724
Attachment #8967155 - Flags: review?(stas) → review+
Since we don't have support for FTLtoFTL migrations, we should migrate those strings again from DTD and .properties.

The interesting part is that we now have diffs between what's in FTL, and what would be migrated, because localizers updated only the new FTL files.

I can try to track down the differences, and fix them in Pontoon. The list also highlight one issue: one of the en-US has the wrong apostrophe. One more reason to prioritize bug 1452962 and bug 1416149.

----


Checking browser/browser/preferences/preferences.ftl:search-results-sorry-message

Values are different for my
Found in FTL:
{ PLATFORM() ->
        [windows] ဝမ်းနည်းပါတယ်။ အပြင်အဆင်များထဲတွင် “<span></span>” အတွက် ရလဒ်များ မရှိပါ။
       *[other] ဝမ်းနည်းပါတယ်။ နှစ်သက်ရာအပြင်အဆင်များထဲတွင် “<span></span>” အတွက် ရလဒ်များ မရှိပါ။
    }
Expected from migration:
{ PLATFORM() ->
        [windows] ဝမ်းနည်းပါတယ်။ “<span></span>” အတွက် ရွေးစရာများထဲတွင် မရှိပါ။
       *[other] ဝမ်းနည်းပါတယ်။ “<span></span>” အတွက် နှစ်သက်ရာအပြင်အဆင်များထဲတွင် မတွေ့ပါ။
    }


Checking browser/browser/preferences/preferences.ftl:search-results-need-help

Values are different for ro
Found in FTL:
Ai nevoie de ajutor? Vizitează <a>pagina de suport { -brand-short-name }</a>
Expected from migration:
Ai nevoie de ajutor? Vizitează <a>Suport { -brand-short-name }</a>

Values are different for ka
Found in FTL:
გესაჭიროებათ დახმარება? ეწვიეთ <a>{ -brand-short-name } მხარდაჭერის გვერდს</a>
Expected from migration:
გესაჭიროებათ დახმარება? იხილეთ <a>{ -brand-short-name } მხარდაჭერის გვერდი</a>


Checking browser/browser/preferences/preferences.ftl:update-application-info

Values are different for hi-IN
Found in FTL:
संस्करण{ $version } <a>क्या नया है?</a>
Expected from migration:
संस्करण{ $version } <a>क्या नया है?</a>

Values are different for en-US
Found in FTL:
Version { $version } <a>What's new</a>
Expected from migration:
Version { $version } <a>What’s new</a>


Checking browser/browser/preferences/preferences.ftl:performance-limit-content-process-disabled-desc

Values are different for lij
Found in FTL:
Cangiâ o numero de cntegnui de processo o l'é poscibile solo in { -brand-short-name } moltiprocesso. <a>Amia comme controlâ se o moltiprocesso o l'é ativo</a>
Expected from migration:
Se peu cangiâ o limite de numero de contegnui de processo solo se ti gh'æ o { -brand-short-name } moltiprocesso. <a>Amia comme se fâ a vedde se o moltiprocesso o l'é abilitou</a>


Checking browser/browser/preferences/preferences.ftl:tracking-description

Values are different for pl
Found in FTL:
Ochrona przed śledzeniem blokuje elementy, które zbierają informacje o przeglądaniu na wielu różnych stronach. <a>Więcej informacji o ochronie przed śledzeniem i prywatności</a>.
Expected from migration:
Ochrona przed śledzeniem blokuje elementy, które zbierają informacje o przeglądaniu na wielu różnych stronach. <a>Więcej informacji o ochronie przed śledzeniem i prywatności</a>
Flags: needinfo?(francesco.lodolo)
I initially tried to fix Polish, but realized the result would be different (period outside of link) and backed it out
https://hg.mozilla.org/l10n-central/pl/rev/9a1cd7fa8d7e

All other differences should be fixed, keeping the most recent one (in FTL).
Comment on attachment 8967207 [details]
Bug 1453480 - Migrate Fluent resources to use DOM Overlays v2.

https://reviewboard.mozilla.org/r/235874/#review241766

::: browser/locales/en-US/browser/preferences/preferences.ftl:83
(Diff revision 1)
>  
>  ## Preferences UI Search Results
>  
>  search-results-header = Search Results
>  
>  # `<span></span>` will be replaced by the search term.

Need to update the comment

::: browser/locales/en-US/browser/preferences/preferences.ftl:270
(Diff revision 1)
>  
>  update-application-title = { -brand-short-name } Updates
>  
>  update-application-description = Keep { -brand-short-name } up to date for the best performance, stability, and security.
>  
> -update-application-info = Version { $version } <a>What's new</a>
> +update-application-info = Version { $version } <a data-l10n-name="learn-more">What's new</a>

What’s new (apostrophe)
Assuming we rename string ID adding a 2, otherwise needs updating.
Attachment #8967330 - Attachment is patch: true
Attachment #8967330 - Attachment mime type: text/x-python-script → text/plain
Comment on attachment 8967330 [details] [diff] [review]
bug_1453480_preferences_dom2_resources.py

Thank you Flod for the migration! I adjusted it and changed the IDs to be more meaningful. It's ready for review :)
Attachment #8967330 - Attachment is obsolete: true
Comment on attachment 8967207 [details]
Bug 1453480 - Migrate Fluent resources to use DOM Overlays v2.

https://reviewboard.mozilla.org/r/235874/#review241936

Looks good, thanks
Attachment #8967207 - Flags: review?(francesco.lodolo) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/385de3e4dca0
Update fluent to 0.6.4 and fluent-dom to 0.2.0. r=stas
https://hg.mozilla.org/integration/autoland/rev/6b5e7c13eb8c
Migrate Fluent resources to use DOM Overlays v2. r=flod
Backed out 3 changesets (bug 1453480, bug 1453878) for c2 failures at intl/l10n/test/dom/test_domloc_overlay.htm and browser-chrome failures at browser/components/preferences/in-content/tests/browser_fluent.js on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6b5e7c13eb8c7c77dec98f0eec17b4a26fdbf060

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173419656&repo=autoland&lineNumber=6078

Backout: https://hg.mozilla.org/integration/autoland/rev/de2f038f711e379b9ac4b7eb85fb5387b532ffc7
Flags: needinfo?(gandalf)
Comment on attachment 8967155 [details]
Bug 1453480 - Update fluent to 0.6.4 and fluent-dom to 0.2.0.

So, there's more. In the new overlays, stas changed `attrs` to `attributes` which requires updates to our Node.localize and Preferences calls.

I'd also like to re-request r? on the first chunk because I fixed some tests and ported the updated `messagesFromContext` (not sure how I missed it during the first migration, but now I tested and I'm confident it was the only thing I didn't include).
Flags: needinfo?(gandalf)
Attachment #8967155 - Flags: review+ → review?(stas)
Comment on attachment 8967155 [details]
Bug 1453480 - Update fluent to 0.6.4 and fluent-dom to 0.2.0.

https://reviewboard.mozilla.org/r/235808/#review242144

Should this also update intl/l10n/fluent.js.patch?
Attachment #8967155 - Flags: review?(stas) → review+
Comment on attachment 8967648 [details]
Bug 1453480 - Update Preferences to use fluent-dom 0.2.0 API.

https://reviewboard.mozilla.org/r/236384/#review242146

::: browser/components/preferences/in-content/findInPage.js:486
(Diff revision 1)
>          if (!msg) {
>            console.error(`Missing search l10n id "${refId}"`);
>            return null;
>          }
>          if (refAttr) {
> -          let attr = msg.attrs.find(a => a.name === refAttr);
> +          let attr = msg.attributes.find(a => a.name === refAttr);

`attributes` may be null if the message doesn't have any.
Attachment #8967648 - Flags: review?(stas) → review-
Comment on attachment 8967649 [details]
Bug 1453480 - Update Node.localize API to renamed attrs->attributes in fluent-dom 0.2.0.

https://reviewboard.mozilla.org/r/236386/#review242184
Attachment #8967649 - Flags: review?(bugs) → review+
> Should this also update intl/l10n/fluent.js.patch?

Good call, I'll add it before landing.
Comment on attachment 8967648 [details]
Bug 1453480 - Update Preferences to use fluent-dom 0.2.0 API.

https://reviewboard.mozilla.org/r/236384/#review242222
Attachment #8967648 - Flags: review?(stas) → review+
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #28)
> > Should this also update intl/l10n/fluent.js.patch?
> 
> Good call, I'll add it before landing.

Thanks. In general, I think it might be useful to review fluent.js.patch rather than the other files. If you're taking them from fluent.js's master, they've already been reviewed :)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/658fedb903d0
Update fluent to 0.6.4 and fluent-dom to 0.2.0. r=stas
https://hg.mozilla.org/integration/autoland/rev/46a634d6853c
Migrate Fluent resources to use DOM Overlays v2. r=flod
https://hg.mozilla.org/integration/autoland/rev/a3c36fa7ac0c
Update Preferences to use fluent-dom 0.2.0 API. r=stas
https://hg.mozilla.org/integration/autoland/rev/623b37fe0fe8
Update Node.localize API to renamed attrs->attributes in fluent-dom 0.2.0. r=smaug
Backed out 4 changesets (bug 1453480) for failing browser-chrome at browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js on a CLOSED TREE 

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=623b37fe0fe82d6eff6634cda655252f95b4f8b1

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=173543451&repo=autoland&lineNumber=6941

Backout: https://hg.mozilla.org/integration/autoland/rev/bab10eeb6799a277b7237851293041cbec55a370

[task 2018-04-13T16:24:08.387Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.388Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.390Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.391Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.394Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.395Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.396Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.399Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.401Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.402Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.405Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.407Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.408Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.409Z] 16:24:08     INFO - Buffered messages finished
[task 2018-04-13T16:24:08.410Z] 16:24:08     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should be in search results - 
[task 2018-04-13T16:24:08.411Z] 16:24:08     INFO - Stack trace:
[task 2018-04-13T16:24:08.413Z] 16:24:08     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/head.js:is_element_visible:24
[task 2018-04-13T16:24:08.415Z] 16:24:08     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/head.js:evaluateSearchResults:160
[task 2018-04-13T16:24:08.416Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.417Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should be in search results - 
[task 2018-04-13T16:24:08.418Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.419Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.420Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.421Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.422Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.423Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.424Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.425Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results - 
[task 2018-04-13T16:24:08.426Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Element should not be null, when checking visibility - 
[task 2018-04-13T16:24:08.427Z] 16:24:08     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_subdialogs_within_preferences_site_data.js | Should not be in search results -
Flags: needinfo?(gandalf)
This bug did not cause the above mentioned failure, relading it. We apologize for the confusion.
Flags: needinfo?(gandalf)
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e11e552e28c5
Update fluent to 0.6.4 and fluent-dom to 0.2.0. r=stas
https://hg.mozilla.org/integration/autoland/rev/54886ee3bc50
Migrate Fluent resources to use DOM Overlays v2. r=flod
https://hg.mozilla.org/integration/autoland/rev/4de9bbcfd440
Update Preferences to use fluent-dom 0.2.0 API. r=stas
https://hg.mozilla.org/integration/autoland/rev/b0f73dc15179
Update Node.localize API to renamed attrs->attributes in fluent-dom 0.2.0. r=smaug on a CLOSED TREE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: