Firefox Suggest history scenario l10n
Categories
(Firefox :: Address Bar, task, P1)
Tracking
()
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(4 files)
|
138.29 KB,
image/png
|
Details | |
|
48 bytes,
text/x-phabricator-request
|
Details | Review | |
|
98.44 KB,
image/png
|
Details | |
|
34.63 KB,
patch
|
RyanVM
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
Firefox Suggest screenshot
|
Assignee | |
Comment 1•3 years ago
|
||
Zibi, I have a question about Fluent I hope you can answer. I'm trying to internationalize the "Firefox Suggest" label in the attached screenshot. Right now it's a hardcoded en string. It's implemented by setting a label attribute on the row below the label, and then in the CSS, we have a ::before pseudo-element on the row with content: attr(label).
Each time you type a character in the urlbar, we update all the rows -- usually more than once. That means that one second a row may not need the Firefox Suggest label and then a split second later it might, or vice versa.
I have a patch that adds a Fluent string like this (simplified):
urlbar-group-label-firefox-suggest =
.label = Firefox Suggest
And then my JS uses document.l10n.setAttributes on the row.
The problem I'm seeing is that the Firefox Suggest label pops in. i.e., there's a row that should have the label, and one second it doesn't and then a split second later it does. It happens each time there's a row with a label. That doesn't happen with a hardcoded string -- the row and label always appear together. It creates a noticable flickering because it changes the height of the panel and offsets all the rows below it. (I've made some screen recordings and stepped through them to verify. While doing that I noticed it's also a problem for some existing strings in the panel, but the flickering is not as obvious.)
Presumably it's because the string fetch from Fluent is async. What's the best way to avoid that?
I'm considering caching the string. That's an easy fix for this one label, but we also have a "{$searchEngine} Suggestions" label that depends on the user's current search engine. We could cache that string too per search engine but it's getting a little awkward.
There's Localization.formatValueSync and the docs say it uses sync I/O, which makes it unacceptable -- but is it really doing I/O to parse the ftl for a single string every time it's called?
Any other ideas?
|
||
Comment 2•3 years ago
|
||
You should think of sync Fluent the way you think of sync XMLHttpRequest - avoid highly, but it has its uses. Maybe it is one?
But frankly, my guess is that it is not. The places where you want sync behavior are places where you are forced by the logic of the UI to act synchronously, and here, you can afford to be async - you just want to be synchronized in your asynchronicity :)
One solution you can try is to synchronize the refresh of DOM together with localization:
async function updateMyPopup() {
let row = root.querySelector(...);
document.l10n.setAttributes(row, ...);
await document.l10n.translateElements([row]);
.. other operations
}
If needed, you can even create a new DOM fragment (or clone it) detached from document, ensure everything is ready and then swap the one attached to the document with the new one.
It sounds like an expensive operation, but it really shouldn't be and may even provide you more control to minimize the "jumpieness" of the UI as the data is populated.
|
|
Assignee | |
Comment 3•3 years ago
|
||
This adds strings for the "Firefox Suggest" and "{ $engine } Suggestions"
labels.
It also adds a temporary -firefox-suggest-brand-name string that should not
land as is, but this is ready for review otherwise. We're blocked on Product for
final localization instructions, and I'll update this revision when ready.
Regardless of the instructions, I imagine we'll want a
-firefox-suggest-brand-name string, and I'd like some guidance on where it
should live. In browser/locales/en-US/browser/branding? In brandings.ftl or a
separate file like sync-brand.ftl?
I ran into a pop-in/flickering problem described in the bug due to the async
nature of the Fluent lookup. IMO it wasn't acceptable. The labels not only pop
in, they also change the height of the panel and push all the results below them
downward since they take up an entire row of horizontal space themselves. To
work around it, I added a whole system for caching strings so they can be used
synchronously.
I also worked on an approach along the lines of what Zibi suggested in the bug:
Fetch the strings asynchronously, but block the view update on it. Note that in
this revision I do fetch the strings before the view update, I just don't block
the view update on them, so hypothetically it's possible the label strings won't
be fetched in time to use them in the view. I haven't seen that yet on my
machine, and even if it happens it'll only affect the very first view update(s)
after app startup because after that the strings will always be cached.
I do think Zibi's suggestion is probably the right idea, but the changes
required are a little scary to uplift. We need to make onQueryResults async so
that it can wait for the strings fetch, which then requires onQueryFinished to
also be async. That's a fundamental change to the architecture and it affects
everyone, not only people who see the Firefox Suggest labels.
I have a patch that seems to work, but I don't think it's wise to uplift it. So
I'd like to land and uplift this revision first, and then I can come back to it
and let it bake on Nightly. The patch builds on this revision because we still
need to cache strings so they can be used syncly during the view update.
While I was working on this and making screen recordings to check the flicker, I
noticed some of our existing strings pop in too due to our use of
document.l10n.setAttributes when the view is open, like the "Search with
{$engine}" string in results and the "Switch to Tab" chiclet text. That existing
pop-in isn't nearly as noticeable as the Firefox Suggest label because it
doesn't disrupt the view as much. We could potentially use this caching system
for these other strings to reduce overall flicker.
This is tested by the existing browser_groupLabels.js.
|
|
Assignee | |
Comment 4•3 years ago
|
||
|
|
Assignee | |
Comment 5•3 years ago
|
||
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 6•3 years ago
|
||
|
|
Assignee | |
Comment 7•3 years ago
|
||
|
|
Assignee | |
Comment 8•3 years ago
|
||
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/931a7c66c02f Internationalize the Firefox Suggest address bar label. r=harry,flod
|
||
Comment 10•3 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 11•3 years ago
|
||
STR for QA:
This bug enables the localization of the "Firefox Suggest" group heading/label in the address bar panel that appears above bookmarks/history (see screenshot), so to verify, just use some non-en-US locales and verify that the string appears and is (hopefully) localized, but that depends on whether particular localizers have localized it.
If I'm not mistaken, based on Slack conversation about the Romainian localization (bug 1728595) Cosmin has already verified this.
|
|
Assignee | |
Comment 12•3 years ago
•
|
||
[Tracking Requested - why for this release]: Some urgent Firefox Suggest patches we want to uplift to a 92 dot release depend on this bug.
This patch adds a string that is exposed to localizers. Bug 1729591 will remove that string so that it is not exposed on 92 and will also need to be uplifted as part of this Firefox Suggest uplift stack. [Edit: Strings plural I should say. My comment here applies to all strings exposed in this patch.]
|
||
Comment 13•3 years ago
|
||
- I have verified this issue using the latest Nightly 94.0a1 (Build ID: 20210908213905) and Beta 93.0b2 (Build ID: 20210907190438) l10n builds across platforms.
- The "Firefox Suggest" label is displayed just above the history/bookmarks results from the Address Bar.
Environments:
Windows 10 x64:
- de, fr (localized strings)
- ja, ro (not localized strings)
macOS 11.5.2:
- es-ES, ar (not localized strings)
Ubuntu 20.04 x64:
- pt-PT, ru, it (not localized strings)
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 15•3 years ago
•
|
||
Carmen, please note that the fix for this bug will not be present on 92 once all patches are uplifted to 92. That might be clear already but I just want to make sure it is. On 92, the label should be shown only for en-* locales. So for 92, the verification for this bug is the same as for bug 1729591. I'll comment there too.
|
|
Assignee | |
Comment 16•3 years ago
|
||
Approval Request Comment
[Feature/Bug causing the regression]: Firefox Suggest offline/online rollouts (this bug is not directly related to the offline/online rollouts but code that is directly related depends on it)
[User impact if declined]: Needed for important rollouts on 93 and 92
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, please see comment 11 and comment 15
[List of other uplifts needed for the feature/fix]: See uplift coordination spreadsheet
[Is the change risky?]: Low risk relative to other uplifts needed in the patch stack
[Why is the change risky/not risky?]: This patch localizes 3 strings used by the Firefox Suggest feature that were previously hardcoded, and there are no visible changes. That means that this patch exposes new strings to localizers. However, bug 1729591, which is later in this patch stack, will revert the part of this patch that exposes the strings so that ultimately no new strings will be exposed on 92.
[String changes made/needed]:
|
||
Updated•3 years ago
|
|
||
Comment 17•3 years ago
|
||
Comment on attachment 9240783 [details] [diff] [review]
92/mozilla-release patch
Approved for 92.0.1.
|
|
||
Comment 18•3 years ago
|
||
| bugherder uplift | ||
|
|
||
Updated•3 years ago
|
Description
•