Migrate search results pane of Preferences to Fluent

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

Part of bug 1415730 - there's a separate screen for search results and it brings several strings that we should migrate.
Assignee: nobody → gandalf
Blocks: 1415730
Status: NEW → ASSIGNED
Component: Internationalization → Preferences
Priority: -- → P3
Product: Core → Firefox
The patch is functionally ready, and once I add migration it can be reviewed, but there's one issue:

We are adding an attribute to a Message. I changed the message id to reflect that, but I'm not sure how to capture that in our migration script.

One escape strategy is to give up and let people translate it as a new string.

Not sure if there's a better way. Flod? Stas?
Flags: needinfo?(stas)
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233076

::: browser/locales/en-US/browser/preferences/preferences.ftl:82
(Diff revision 1)
> +
> +search-results-header = Search Results
> +
> +search-results-sorry-message =
> +    { PLATFORM() ->
> +        [windows] Sorry! There are no results in Options for "{ $query }".

Use curly quotes “” in strings (also bug 1416149).
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> We are adding an attribute to a Message. I changed the message id to reflect
> that, but I'm not sure how to capture that in our migration script.

You migrate the string as brand new, taking the value from 3 different strings (width, 2 placeholders).

In that case, this would be actively blocked by bug 1445001.
 
> One escape strategy is to give up and let people translate it as a new
> string.

We could do that, but it feels like a shortcut, especially considering that Pontoon will fail to give you suggestions from translation memory.
Flags: needinfo?(francesco.lodolo)
Attachment #8958275 - Flags: feedback?(francesco.lodolo)
I second what flod said: the easiest way is to re-migrate the whole string, including the style attribute.
Flags: needinfo?(stas)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233104

::: browser/locales/en-US/browser/preferences/preferences.ftl:22
(Diff revision 1)
>          }
>  
>  # This is used to determine the width of the search field in about:preferences,
>  # in order to make the entire placeholder string visible
>  #
> +# Please keep the placeholder string shorter than around 30 characters

I wonder if this actually makes sense, since we set the width, and it's clear it is the same element.

Maybe just this, or drop it completely:

# Please keep the placeholder string short to avoid truncation.
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

In general it looks good, pending the migration issue.

If we don't want to get blocked, we can decide to keep the old string around for a while, while the migration is fixed.
Attachment #8958275 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

The patch is ready, but I encountered a piece I'm not sure how to handle in migration.

I need to remove "%S" from a string - I tried replacing it with None, but that crashes.

Stas - any recommendations?
Attachment #8958275 - Flags: feedback?(stas)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> Stas - any recommendations?

I'd also like to hear your opinion on the migration for search-input, in relation with bug 1445001. Possible solutions that don't require to touch the core migration code:
* We write the migration module with paths referencing gecko-strings-quarantine. That needs testing, to verify if it actually works, and likely a look at the code to confirm.
* We keep around search-input, and file a bug to schedule its removal (but it will take a long time to be able to do it).
I was able to use nested concat in the migration instead.

Jared, Francesco - This is not ready for review.

The question for :stas from comment 10 stands, but shouldn't affect the review process.
Flags: needinfo?(stas)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> I was able to use nested concat in the migration instead.
> 
> Jared, Francesco - This is not ready for review.

I assume this should be "is ready for review", given the next paragraph.

> The question for :stas from comment 10 stands, but shouldn't affect the
> review process.
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233386

This looks good. Will give an r+ as soon as the migration question is answered.

::: browser/components/preferences/in-content/findInPage.js:296
(Diff revision 3)
> -          strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]);
>          let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences";
> -        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> -        let helpString = strings.getString("searchResults.needHelp3");
>          let helpContainer = document.getElementById("need-help");
> -        let link = document.createElement("label");
> +        let link = helpContainer.querySelector("a").setAttribute("href", helpUrl);

This is an error if it doesn't land with bug 1415733.

::: browser/locales/en-US/browser/preferences/preferences.ftl:30
(Diff revision 3)
>  # is the name of the CSS property. It is intended only to adjust the element's width.
>  # Do not translate.
> -search-input =
> +search-input-box =
>      .style = width: 15.4em
> +    .placeholder =
> +        { $PLATFORM ->

$PLATFORM should be PLATFORM()

::: python/l10n/fluent_migrations/bug_144508_preferences_search_results.py:114
(Diff revision 3)
> +                    ]
> +                ),
> +            ),
> +            FTL.Message(
> +                id=FTL.Identifier('search-results-need-help'),
> +                value=CONCAT(

value=REPLACE() directly should work here
(In reply to Francesco Lodolo [:flod] from comment #10)
> * We write the migration module with paths referencing
> gecko-strings-quarantine. That needs testing, to verify if it actually
> works, and likely a look at the code to confirm.

From a quick test.

1) Changed the migration module to

    ctx.add_transforms(
        'browser/browser/preferences/preferences.ftl',
        'browser/browser/preferences/preferences.ftl',

2) Manually copied preferences.ftl over to gecko-strings-quarantine and restored search-input (that would be done by x-channel).

3) Run migration using the quarantine repo as --reference-dir

Migration works fine, and keep both strings.

This might be a solution: delegate conflicts management to x-channel, run migrations against gecko-strings-quarantine. That wouldn't change our current steps for migration, since we wait for the strings to be in quarantine (and verified) before starting.
(In reply to Francesco Lodolo [:flod] from comment #13)

> I assume this should be "is ready for review", given the next paragraph.

"This is now ready for review"! :D
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> I was able to use nested concat in the migration instead.

Looking the interdiff, this is the correct approach, nice work. I'd be surprised if a migration just removed a placeable from translated sentences :)
Flags: needinfo?(stas)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233396

::: python/l10n/fluent_migrations/bug_144508_preferences_search_results.py:10
(Diff revision 3)
> +
> +from __future__ import absolute_import
> +import fluent.syntax.ast as FTL
> +from fluent.migrate.helpers import MESSAGE_REFERENCE, EXTERNAL_ARGUMENT
> +from fluent.migrate.transforms import REPLACE
> +from fluent.migrate import COPY, CONCAT

You can import REPLACE from fluent.migrate as well.

::: python/l10n/fluent_migrations/bug_144508_preferences_search_results.py:36
(Diff revision 3)
> +                        ),
> +                    ),
> +                    FTL.Attribute(
> +                        FTL.Identifier('placeholder'),
> +                        FTL.Pattern(
> +                            elements=[

Hint: you can reduce the indentation here and in other values by making `elements` a positional arg: `FTL.Pattern([…])`. In fact, I would recommend that for the AST nodes that take a single argument (Pattern, Placeable) we use posargs.
(In reply to Francesco Lodolo [:flod] from comment #10)
> I'd also like to hear your opinion on the migration for search-input, in
> relation with bug 1445001.

Thanks for asking.

> Possible solutions that don't require to touch
> the core migration code:
> * We write the migration module with paths referencing
> gecko-strings-quarantine. That needs testing, to verify if it actually
> works, and likely a look at the code to confirm.

I guess this would make the most sense with bug 1413514. Otherwise, if we hard-code the path from gecko-strings, what's the purpose of keeping the migration .py files in mozilla-central?

As a general rule, I'd like us to run migrations against gecko-strings-quarantine. Let's try to compile a list of things blocking us from doing so.

Perhaps we should move this to bug 1445001.

> * We keep around search-input, and file a bug to schedule its removal (but
> it will take a long time to be able to do it).

This might be a good work-around for now.
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233514
Attachment #8958275 - Flags: review?(francesco.lodolo) → review+
To document the decision.

We're going to keep the old string around for now, so it won't be removed when running the migration code. This unblocks the current queue of patches.

Later, we need to decide how to fix it, hopefully before 61 moves to Beta at the beginning of May.

Two options:
1) We update the migration code in bug 1445001
2) We start running the migration code against gecko-strings-quarantine instead of mozilla-central

#2 is also tied to a larger discussion on where these migration modules should live, given that migration can run independently from the code in mozilla-central.
Attachment #8958275 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233626

I'm really happy with the removal of the complicated innerHTML hijinks. But I still noticed a few issues...

::: browser/components/preferences/in-content/findInPage.js:291
(Diff revision 4)
> -
> -        document.getElementById("sorry-message").textContent = AppConstants.platform == "win" ?
> -          strings.getFormattedString("searchResults.sorryMessageWin", [this.query]) :
> +        document.l10n.setAttributes(msgElem, "search-results-sorry-message", {
> +          query: this.query
> +        });

This element is empty before this happens (assuming there are hits" prior to us taking this codepath), because we set .textContent to emptystring. In practice, what seems to happen is that we then unhide it above (hidden = false), localize this asynchronously, and set the href for the help URL (synchronously) which doesn't need localizing here. The result is that I see at least 1 frame with only the 'help' text, and then the 'sorry, ...' text appears. This causes flicker.

The documentation (at https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html ) doesn't seem to say if `setAttributes` returns a promise or something that lets us wait until the localization has happened before unhiding the block (if indeed it is hidden - otherwise the effect is fine, it just means there's 1 frame of lag before the "sorry" text updates based on your search string).

It would be nice to do better here...

::: browser/components/preferences/in-content/findInPage.js:296
(Diff revision 4)
> -          strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]);
>          let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences";
> -        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> -        let helpString = strings.getString("searchResults.needHelp3");
>          let helpContainer = document.getElementById("need-help");
> -        let link = document.createElement("label");
> +        helpContainer.querySelector("a").setAttribute("href", helpUrl);

I don't really understand flod's comment that this doesn't work without the other change.

However, you're switching from a `<label class="text-link">` to an "<a>". This has the side-effect that the link opens in the same tab, replacing the prefs (and causing a weird interim load that just shows a bunch of grey and `about:preferences#searchResults` in the tab as the title...). We should avoid doing this. It's also not clear why you're switching away from the existing `<label>` stuff. I'm all for removing XUL in favour of HTML stuff, but doing it as part of this change feels a bit odd (and, as said, breaks stuff).

If we do end up doing this, then also: nit: `.href =` instead of `setAttribute`, please. :-)

::: browser/locales/en-US/browser/preferences/preferences.ftl:86
(Diff revision 4)
> +        [windows] Sorry! There are no results in Options for “{ $query }”.
> +       *[other] Sorry! There are no results in Preferences for “{ $query }”.

So my understanding is that one of the things fluent is good at is solving cases where either plural forms or case/gender of a substituted thing (`$query` in this case) is important. However, `$query` is just an arbitrary search string (not even guaranteed to be a reasonable word, or in the right language, because it's just user input). So I assume none of that magic applies here, even for other languages. Is that fair?

If so, is there a way we can do this without doing `setAttributes` "just in time" localization? In particular, can the string basically just be:

`Sorry! There are no results in Options for <span></span>`

, inserted via `data-l10n-id`, and then we can just update the contents of the span synchronously when the search term changes? Or does that not work for some reason?
Attachment #8958275 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233640

::: browser/components/preferences/in-content/findInPage.js:296
(Diff revision 4)
> -          strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]);
>          let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences";
> -        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> -        let helpString = strings.getString("searchResults.needHelp3");
>          let helpContainer = document.getElementById("need-help");
> -        let link = document.createElement("label");
> +        helpContainer.querySelector("a").setAttribute("href", helpUrl);

Oh, and separately... this is sort of because of the poor ergonomics of l10n pre-fluent, but we're setting this HTML fragment up every. single. time. You removed most of that, but kept setting the href. But the href never changes. Can we just do this (ie setting the href) in init() instead of every time we display the warning? :-)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233626

> This element is empty before this happens (assuming there are hits" prior to us taking this codepath), because we set .textContent to emptystring. In practice, what seems to happen is that we then unhide it above (hidden = false), localize this asynchronously, and set the href for the help URL (synchronously) which doesn't need localizing here. The result is that I see at least 1 frame with only the 'help' text, and then the 'sorry, ...' text appears. This causes flicker.
> 
> The documentation (at https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html ) doesn't seem to say if `setAttributes` returns a promise or something that lets us wait until the localization has happened before unhiding the block (if indeed it is hidden - otherwise the effect is fine, it just means there's 1 frame of lag before the "sorry" text updates based on your search string).
> 
> It would be nice to do better here...

The promise would be basically a callback to the next animation frame, so instead I just set a `rAF` manually. It fixes the flicker frame, so I hope it'll be ok with you :)

> I don't really understand flod's comment that this doesn't work without the other change.
> 
> However, you're switching from a `<label class="text-link">` to an "<a>". This has the side-effect that the link opens in the same tab, replacing the prefs (and causing a weird interim load that just shows a bunch of grey and `about:preferences#searchResults` in the tab as the title...). We should avoid doing this. It's also not clear why you're switching away from the existing `<label>` stuff. I'm all for removing XUL in favour of HTML stuff, but doing it as part of this change feels a bit odd (and, as said, breaks stuff).
> 
> If we do end up doing this, then also: nit: `.href =` instead of `setAttribute`, please. :-)

Thanks for the explanation! Didn't know about this difference!

We use `<a>` here because the first iteration of DOM Overlays has been intentionally limited to a small number of known (HTML) elements for security reasons.
The good news is that Stas is about to release second revision of DOM Overlays which start treating elements in content as arguments, which is exactly what we'd need here to use a label (it's not really a localizable text level semantic!).

With it, you'll be able to place `<label>` in the XUL source, and it'll be populated with the DOM Fragment localization.

This will hopefully still land in 61, but I expect closer to mid-april. I'd like to not wait with this patch for that and that's why I switched to `<a>` (and now added `target="_blank"`. Let me know if that's ok!

> So my understanding is that one of the things fluent is good at is solving cases where either plural forms or case/gender of a substituted thing (`$query` in this case) is important. However, `$query` is just an arbitrary search string (not even guaranteed to be a reasonable word, or in the right language, because it's just user input). So I assume none of that magic applies here, even for other languages. Is that fair?
> 
> If so, is there a way we can do this without doing `setAttributes` "just in time" localization? In particular, can the string basically just be:
> 
> `Sorry! There are no results in Options for <span></span>`
> 
> , inserted via `data-l10n-id`, and then we can just update the contents of the span synchronously when the search term changes? Or does that not work for some reason?

While looking at how to fix your previous comment I thought exactly the same. It'd be sweet just populate the query.

This will be possible with the 2nd revision of DOM Overlays!
Attachment #8958275 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #24)
> ::: browser/components/preferences/in-content/findInPage.js:296
> (Diff revision 4)
> > -          strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]);
> >          let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences";
> > -        let brandName = document.getElementById("bundleBrand").getString("brandShortName");
> > -        let helpString = strings.getString("searchResults.needHelp3");
> >          let helpContainer = document.getElementById("need-help");
> > -        let link = document.createElement("label");
> > +        helpContainer.querySelector("a").setAttribute("href", helpUrl);
> 
> I don't really understand flod's comment that this doesn't work without the
> other change.

Sorry, I forget there are people following the discussion.

"let link" was an ESlint error, because the variable link is unused. It was fixed in a different bug that built on top of this, so I pointed out that this needed fixing here too.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #27)
> > If so, is there a way we can do this without doing `setAttributes` "just in time" localization? In particular, can the string basically just be:
> > 
> > `Sorry! There are no results in Options for <span></span>`
> > 
> > , inserted via `data-l10n-id`, and then we can just update the contents of the span synchronously when the search term changes? Or does that not work for some reason?
> 
> While looking at how to fix your previous comment I thought exactly the
> same. It'd be sweet just populate the query.
> 
> This will be possible with the 2nd revision of DOM Overlays!

Correction: Overlays2 can only populate _attributes_ from child elements defined in the source HTML. The source text content is always removed and replaced by the translation if it's available.

Populating the <span> outside of the setAttributes call could result in a race condition: if the query is inserted into the <span> before the translation triggered by setAttributes runs, the overlays2 logic will remove the contents of the <span>.

Doing it via setAttributes (the way the current patch does) ensure the unilateral flow of data (query -> translation -> dom). I think we should document it as a good practice.
Just realized that the migration file's name is missing a 4, it should be bug_1444508_preferences_search_results
(In reply to Staś Małolepszy :stas (back on Mar 26) from comment #30)
> Doing it via setAttributes (the way the current patch does) ensure the
> unilateral flow of data (query -> translation -> dom). I think we should
> document it as a good practice.

Early morning typo. I meant to say: …will ensure the unidirectional flow of data.
(In reply to Francesco Lodolo [:flod] from comment #23)
> 2) We start running the migration code against gecko-strings-quarantine
> instead of mozilla-central
> 
> #2 is also tied to a larger discussion on where these migration modules
> should live, given that migration can run independently from the code in
> mozilla-central.

Thanks for the summary, flod. I filed bug 1445881 to discuss this in detail.
(In reply to Staś Małolepszy :stas (back on Mar 26) from comment #30)
> Populating the <span> outside of the setAttributes call could result in a
> race condition: if the query is inserted into the <span> before the
> translation triggered by setAttributes runs, the overlays2 logic will remove
> the contents of the <span>.
> 
> Doing it via setAttributes (the way the current patch does) ensure the
> unilateral flow of data (query -> translation -> dom). I think we should
> document it as a good practice.

Whereas I think that we should add logic to fluent to avoid this, so that we can populate content and tell fluent "The contents of this node are provided by something else, leave any contents alone", perhaps by adding an attribute on the markup side (data-l10n-static or data-l10n-dontchange or something). "Static" translation like that is always going to be better and more predictable than the runtime variations. The "unilateral flow of data" thing is causing us to duplicate l10n work every time the user types any letter in this case, and is also causing us to have to write workarounds for the fact that translation is async. From an engineering point, that makes l10n more painful than it needs to be.

Would this be possible?
Flags: needinfo?(stas)
Flags: needinfo?(gandalf)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review233860

r=me with the race condition below fixed. I'd still be interested if we can make fluent easier to work with for cases like this where we're inserting content in a localized container.

::: browser/components/preferences/in-content/findInPage.js:297
(Diff revisions 4 - 6)
> -        let helpContainer = document.getElementById("need-help");
> -        helpContainer.querySelector("a").setAttribute("href", helpUrl);
> +        // We want to unhide this section only when the translation for the
> +        // `sorry-message` is displayed. Since that's going to happen
> +        // in the next animation frame, we will unhide it then.
> +        window.requestAnimationFrame(() => {
> +          noResultsEl.hidden = false;
> +        });

In theory there's a race condition here with this happening after we re-enter this method once newer search results come in, where we then re-hide the content of the 'sorry' message (this happens non-obviously by virtue of the `gotoPref("paneGeneral")`, in the else branch). This would cause the header to display in addition to the search results if it happens, which would obviously be bad.

You can avoid this by checking `query === this.query` (the other code in this function does this in some places, so even though I don't think the triple equals is necessary, let's keep using that to avoid missing edgecases that I don't really want to think about right now) before setting `.hidden = false` .
Attachment #8958275 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #34)
> Whereas I think that we should add logic to fluent to avoid this, so that we
> can populate content and tell fluent "The contents of this node are provided
> by something else, leave any contents alone", perhaps by adding an attribute
> on the markup side (data-l10n-static or data-l10n-dontchange or something).
> "Static" translation like that is always going to be better and more
> predictable than the runtime variations. The "unilateral flow of data" thing
> is causing us to duplicate l10n work every time the user types any letter in
> this case, and is also causing us to have to write workarounds for the fact
> that translation is async. From an engineering point, that makes l10n more
> painful than it needs to be.
> 
> Would this be possible?

Thanks for sharing your thoughts on this! It's definitely possible and I'm open to discussing this approach as a future good practice. My previous comment was mostly intended as a clarification to what fluent-dom is capable of right now. Your feedback will help shape the future iterations of flunet-dom and the overlays logic.
Flags: needinfo?(stas)
Thank you Gijs! I filed bug https://github.com/projectfluent/fluent.js/issues/169 and I'll update this fragment once we have a fix in FluentDOM.
Flags: needinfo?(gandalf)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32f45689f033
Migrate search results pane of Preferences to Fluent. r=flod,Gijs
https://hg.mozilla.org/mozilla-central/rev/32f45689f033
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Backout by ncsoregi@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/24912c654197
Backout commit 32f45689f033 for frequently failing browser-chrome's browser_search_within_preferences_1.js (bug 1446186) a=backout
Backed out for frequently failing browser-chrome's browser_search_within_preferences_1.js (bug 1446186) 

backout: https://hg.mozilla.org/mozilla-central/rev/24912c65419703621c93baf8757620c3045c89bc AND https://hg.mozilla.org/mozilla-central/rev/47e1787284fbfad3d32eb7081ffdda58d2b086de

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=32f45689f03377c8345812106cfcf1b82b322140

Example failure log: https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=168309404&lineNumber=7079
[task 2018-03-15T20:31:20.059Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should not be in general tab - 
[task 2018-03-15T20:31:20.059Z] 20:31:20     INFO - Leaving test bound 
[task 2018-03-15T20:31:20.060Z] 20:31:20     INFO - Entering test bound 
[task 2018-03-15T20:31:20.061Z] 20:31:20     INFO - Buffered messages logged at 20:31:18
[task 2018-03-15T20:31:20.061Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:20.062Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should not be in search results yet - 
[task 2018-03-15T20:31:20.063Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Search input should be focused when visiting preferences - 
[task 2018-03-15T20:31:20.064Z] 20:31:20     INFO - Buffered messages logged at 20:31:19
[task 2018-03-15T20:31:20.065Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:20.065Z] 20:31:20     INFO - Buffered messages finished
[task 2018-03-15T20:31:20.066Z] 20:31:20     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should be in search results - 
[task 2018-03-15T20:31:20.066Z] 20:31:20     INFO - Stack trace:
[task 2018-03-15T20:31:20.067Z] 20:31:20     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/head.js:is_element_visible:24
[task 2018-03-15T20:31:20.068Z] 20:31:20     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js:null:163
[task 2018-03-15T20:31:20.268Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:20.270Z] 20:31:20     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should not be in search results - 
[task 2018-03-15T20:31:20.336Z] 20:31:20     INFO - Leaving test bound 
[task 2018-03-15T20:31:20.339Z] 20:31:20     INFO - Entering test bound 
[task 2018-03-15T20:31:21.105Z] 20:31:21     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:21.113Z] 20:31:21     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should not be in general - 
[task 2018-03-15T20:31:21.114Z] 20:31:21     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Search input should be focused when visiting preferences - 
[task 2018-03-15T20:31:22.351Z] 20:31:22     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:22.354Z] 20:31:22     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should be in generalPane - 
[task 2018-03-15T20:31:22.418Z] 20:31:22     INFO - Leaving test bound 
[task 2018-03-15T20:31:22.418Z] 20:31:22     INFO - Entering test bound 
[task 2018-03-15T20:31:23.001Z] 20:31:23     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:23.003Z] 20:31:23     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should not be in general - 
[task 2018-03-15T20:31:23.003Z] 20:31:23     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Search input should be focused when visiting preferences - 
[task 2018-03-15T20:31:23.467Z] 20:31:23     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:23.468Z] 20:31:23     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should be hidden in search results - 
[task 2018-03-15T20:31:24.129Z] 20:31:24     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Element should not be null, when checking visibility - 
[task 2018-03-15T20:31:24.131Z] 20:31:24     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Should be in generalPane - 
[task 2018-03-15T20:31:24.194Z] 20:31:24     INFO - Leaving test bound 
[task 2018-03-15T20:31:24.194Z] 20:31:24     INFO - Entering test bound 
[task 2018-03-15T20:31:25.008Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Search input should be focused when visiting preferences - 
[task 2018-03-15T20:31:25.635Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | search input should be empty - 
[task 2018-03-15T20:31:25.636Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | No other panel should be selected - 
[task 2018-03-15T20:31:25.637Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | No other panel should be selected - 
[task 2018-03-15T20:31:25.638Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | No other panel should be selected - 
[task 2018-03-15T20:31:25.638Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | Privacy panel should be selected - 
[task 2018-03-15T20:31:25.641Z] 20:31:25     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | No other panel should be selected - 
[task 2018-03-15T20:31:25.717Z] 20:31:25     INFO - Leaving test bound 
[task 2018-03-15T20:31:25.719Z] 20:31:25     INFO - GECKO(3357) | MEMORY STAT | vsize 1210MB | residentFast 375MB | heapAllocated 109MB
[task 2018-03-15T20:31:25.719Z] 20:31:25     INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_search_within_preferences_1.js | took 13806ms
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
Target Milestone: Firefox 61 → ---
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

We decided to fix it by switching to the `<span></span>` model and use the fact that we don't support runtime retranslation yet, and if someone manages to trigger it the worst case scenario is that the sorry string will be displayed without the query (low impact).

I filed bug 1446389 to fix it properly once we add support for sth like `data-l10n-static` to Fluent, and I hope to get it fixed still within 61 cycle.

Requesting r? from Gijs on the https://reviewboard.mozilla.org/r/227202/diff/7-8/
Flags: needinfo?(gandalf)
Attachment #8958275 - Flags: review+ → review?(gijskruitbosch+bugs)
Comment on attachment 8958275 [details]
Bug 1445084 - Migrate search results pane of Preferences to Fluent.

https://reviewboard.mozilla.org/r/227202/#review234204

::: browser/locales/en-US/browser/preferences/preferences.ftl:86
(Diff revisions 7 - 8)
> -        [windows] Sorry! There are no results in Options for “{ $query }”.
> -       *[other] Sorry! There are no results in Preferences for “{ $query }”.
> +        [windows] Sorry! There are no results in Options for “<span></span>”.
> +       *[other] Sorry! There are no results in Preferences for “<span></span>”.

Should we add a localization comment to help clarify what's in the span to localizers ?
Attachment #8958275 - Flags: review?(gijskruitbosch+bugs) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 163330f95a6e2ae918f03d4a31d466dea3776c5c -d db3858c71406: rebasing 452595:163330f95a6e "Bug 1445084 - Migrate search results pane of Preferences to Fluent. r=flod,Gijs" (tip)
merging browser/components/preferences/in-content/findInPage.js
merging browser/components/preferences/in-content/preferences.xul
merging browser/components/preferences/in-content/searchResults.xul
merging browser/locales/en-US/browser/preferences/preferences.ftl
merging browser/locales/en-US/chrome/browser/preferences/preferences.dtd
merging browser/locales/en-US/chrome/browser/preferences/preferences.properties
warning: conflicts while merging browser/locales/en-US/chrome/browser/preferences/preferences.dtd! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a0b4d9ef4fd
Migrate search results pane of Preferences to Fluent. r=flod,Gijs
https://hg.mozilla.org/mozilla-central/rev/6a0b4d9ef4fd
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1446186
Duplicate of this bug: 1443793
You need to log in before you can comment on or make changes to this bug.