Update the messages in the automigration undo import notification bar

VERIFIED FIXED in Firefox 52

Status

()

VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: verdi, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

52 Branch
Firefox 54
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
We have a new string for the automigration infobar:

"Pick up where you left off. We've imported these sites and your bookmarks, history, and passwords from [Browser] into Firefox."

Buttons:
[Ok, Got it]
[No Thanks]
(Assignee)

Comment 1

2 years ago
(In reply to Verdi [:verdi] from comment #0)
> We have a new string for the automigration infobar:
> 
> "Pick up where you left off. We've imported these sites and your bookmarks,
> history, and passwords from [Browser] into Firefox."

I have a question about this. We still show the message on about:home as well as about:newtab (also/particularly when we experiment on beta). Additionally, it's really not easy to determine whether the sites on about:newtab will be strictly from the user's imported data, or only the default set, or a mix - certainly not from the code showing this notification. So I think the "these sites" bit is confusing, and I would like to get rid of it. The suggested wording here is already tricky to implement because it already requires keeping track of exactly what we import (can't tell the user "we imported your passwords from Chrome" if we, well, didn't (for instance, because the user had no saved Chrome passwords), unless we want to set off user paranoia. I would prefer not to add even more complexity.
Flags: needinfo?(mverdi)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
(I've left the strings as suggested for now, but it should be trivial to update them if we do it *before landing* this change.)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 6

2 years ago
Michelle and I talked about this today. We'd like to go with the string in comment 0 for now. We tested it against other versions, including the current string, and it performed best. If you want to use version without "these sites" for about:home that would be great but if we don't think it's critical if you feel the added complexity isn't worth it.
Flags: needinfo?(mverdi)
(Assignee)

Comment 7

2 years ago
(In reply to Verdi [:verdi] from comment #6)
> Michelle and I talked about this today. We'd like to go with the string in
> comment 0 for now. We tested it against other versions, including the
> current string, and it performed best. If you want to use version without
> "these sites" for about:home that would be great but if we don't think it's
> critical if you feel the added complexity isn't worth it.

OK. I'm going to continue to omit the "these sites" for the passwords case, given that then we're *sure* there's no sites showing up on about:newtab that have been imported. :-)

Given Dão's PTO, going to move the reviews to Jared.
Summary: Update the strings in the automigration infobar → Update the messages in the automigration undo import notification bar
(Assignee)

Updated

2 years ago
Attachment #8833287 - Flags: review?(dao+bmo) → review?(jaws)
Attachment #8833288 - Flags: review?(dao+bmo) → review?(jaws)
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8833287 [details]
Bug 1335442 - deal correctly with not importing anything,

https://reviewboard.mozilla.org/r/109536/#review111766

::: browser/components/migration/AutoMigrate.jsm:417
(Diff revision 1)
>    },
>  
>    _dejsonifyUndoState(state) {
>      state = JSON.parse(state);
> +    if (!state) {
> +      return new Map([]);

self-nit: `new Map()`

Comment 9

2 years ago
mozreview-review
Comment on attachment 8833287 [details]
Bug 1335442 - deal correctly with not importing anything,

https://reviewboard.mozilla.org/r/109536/#review112158

::: browser/components/migration/tests/unit/test_automigration.js:212
(Diff revision 1)
> +  let storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
> +                                                "http://www.mozilla.org", null);
> +  Assert.equal(storedLogins.length, 0, "Should have no logins");

Can you do this also before the undo to confirm the login was added?
Attachment #8833287 - Flags: review?(jaws) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8833288 [details]
Bug 1335442 - update wording of automigration notification bar and add tests,

https://reviewboard.mozilla.org/r/109538/#review112160

::: browser/components/migration/tests/browser/browser_undo_notification_wording.js:43
(Diff revision 2)
> +  for (let subset of kSubsets) {
> +    let state = new Map(subset.map(item => [item, [{}]]));
> +    scope.AutoMigrate._setImportedItemPrefFromState(state);
> +    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url, false);
> +    let browser = tab.linkedBrowser;
> +    

i think the whitespace will fail eslint

::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
(Diff revision 2)
> -automigration.undo.unknownBrowserMessage = We automatically imported your data from another browser. Would you like to keep it?
> -automigration.undo.keep.label            = Keep
> -automigration.undo.keep.accesskey        = K
> -automigration.undo.dontkeep.label        = Don’t Keep
> -automigration.undo.dontkeep.accesskey    = D
> +automigration.undo.message.bookmarks     = Pick up where you left off. We've imported these sites and your bookmarks from %1$S into %2$S.
> +automigration.undo.message.visits        = Pick up where you left off. We've imported these sites and your history from %1$S into %2$S.
> +automigration.undo.message.logins        = Pick up where you left off. We've imported your passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.visits = Pick up where you left off. We've imported these sites and your bookmarks and history from %1$S into %2$S.
> +automigration.undo.message.logins.visits    = Pick up where you left off. We've imported these sites and your history and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.logins = Pick up where you left off. We've imported these sites and your bookmarks and passwords from %1$S into %2$S.

can you please order these alphabetically?

::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
(Diff revision 2)
>  
>  128_firefox=Windows and Tabs
>  
>  # Automigration undo notification.
> -automigration.undo.message               = We automatically imported your data from %S. Would you like to keep it?
> -automigration.undo.unknownBrowserMessage = We automatically imported your data from another browser. Would you like to keep it?
> +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> +automigration.undo.message.bookmarks     = Pick up where you left off. We've imported these sites and your bookmarks from %1$S into %2$S.

Pretty sure these strings will fail the unicode-quotes test that we have.
Attachment #8833288 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d122656ee44a
deal correctly with not importing anything, r=jaws
https://hg.mozilla.org/integration/autoland/rev/7a024b0cedfc
update wording of automigration notification bar and add tests, r=jaws

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d122656ee44a
https://hg.mozilla.org/mozilla-central/rev/7a024b0cedfc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Comment 15

2 years ago
Created attachment 8839185 [details] [diff] [review]
Aurora/beta patch

NOTE: do NOT uplift to beta until beta 8 has gone to build.

Approval Request Comment
[Feature/Bug causing the regression]: automigration tests on beta/funnelcakes
[User impact if declined]: suboptimal wording of notification bars users get
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: locally with an m-c build, I see the new strings. But no formal verification has happened.
[Needs manual test from QE? If yes, steps to reproduce]: STR:

1. open about:config on a clean named profile
2. turn the browser.migrate.automigrate.enabled pref to true
3. on your machine, make IE/Chrome/Safari/Edge the default browser
4. close nightly and run it from the commandline with:

path/to/firefox.exe --no-remote -P <profile> -migration

[List of other uplifts needed for the feature/fix]: needs the patch from bug 1335709 to be backed out (after beta 8)
[Is the change risky?]: no
[Why is the change risky/not risky?]: it has automated tests and pertains to a feature we don't ship to general release
[String changes made/needed]: there are hardcoded strings in these patches. They won't be localized. The feature won't be enabled except for en-US users on release. It is currently also enabled on beta for everyone. It will be turned off on beta after beta 8.
Attachment #8839185 - Flags: review+
Attachment #8839185 - Flags: approval-mozilla-beta?
Attachment #8839185 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8839185 - Attachment is patch: true
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Hi :flod,
For string changes made/needed part, is it ok for you?
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(brindusa.tot)

Updated

2 years ago
status-firefox52: --- → affected
status-firefox53: --- → affected
(In reply to Gerry Chang [:gchang] from comment #16)
> Hi :flod,
> For string changes made/needed part, is it ok for you?

Yes, there's no impact for l10n (strings are not exposed to localization in the patch for aurora/beta).
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 18

2 years ago
I just backed out 1335709 from beta, so this is now good to go once it has approval.
Comment on attachment 8839185 [details] [diff] [review]
Aurora/beta patch

changes to automigration, aurora53+, beta52+

Doesn't look like any of this affects the default configurations.
Attachment #8839185 - Flags: approval-mozilla-beta?
Attachment #8839185 - Flags: approval-mozilla-beta+
Attachment #8839185 - Flags: approval-mozilla-aurora?
Attachment #8839185 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/diff/7a024b0cedfc/browser/locales/en-US/chrome/browser/migration/migration.properties
> +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> +automigration.undo.message.all              = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks        = Pick up where you left off. We’ve imported these sites and your bookmarks from %1$S into %2$S.
> +automigration.undo.message.bookmarks.logins = Pick up where you left off. We’ve imported these sites and your bookmarks and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.visits = Pick up where you left off. We’ve imported these sites and your bookmarks and history from %1$S into %2$S.
> +automigration.undo.message.logins           = Pick up where you left off. We’ve imported your passwords from %1$S into %2$S.
> +automigration.undo.message.logins.visits    = Pick up where you left off. We’ve imported these sites and your history and passwords from %1$S into %2$S.
> +automigration.undo.message.visits           = Pick up where you left off. We’ve imported these sites and your history from %1$S into %2$S.
> +automigration.undo.keep2.label            = OK, Got it
> +automigration.undo.keep2.accesskey        = O
> +automigration.undo.dontkeep2.label        = No Thanks
> +automigration.undo.dontkeep2.accesskey    = N
> +automigration.undo.unknownbrowser         = Unknown Browser

Unfortunately the "Unknown Browser" replacement construct doesn't work at all for Polish because we need helper word ("application") before %1$S ("We’ve imported your passwords from application Unknown Browser into application Firefox").
(Assignee)

Comment 23

2 years ago
(In reply to Stefan Plewako [:stef] from comment #22)
> https://hg.mozilla.org/mozilla-central/diff/7a024b0cedfc/browser/locales/en-
> US/chrome/browser/migration/migration.properties
> > +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> > +automigration.undo.message.all              = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> > +automigration.undo.message.bookmarks        = Pick up where you left off. We’ve imported these sites and your bookmarks from %1$S into %2$S.
> > +automigration.undo.message.bookmarks.logins = Pick up where you left off. We’ve imported these sites and your bookmarks and passwords from %1$S into %2$S.
> > +automigration.undo.message.bookmarks.visits = Pick up where you left off. We’ve imported these sites and your bookmarks and history from %1$S into %2$S.
> > +automigration.undo.message.logins           = Pick up where you left off. We’ve imported your passwords from %1$S into %2$S.
> > +automigration.undo.message.logins.visits    = Pick up where you left off. We’ve imported these sites and your history and passwords from %1$S into %2$S.
> > +automigration.undo.message.visits           = Pick up where you left off. We’ve imported these sites and your history from %1$S into %2$S.
> > +automigration.undo.keep2.label            = OK, Got it
> > +automigration.undo.keep2.accesskey        = O
> > +automigration.undo.dontkeep2.label        = No Thanks
> > +automigration.undo.dontkeep2.accesskey    = N
> > +automigration.undo.unknownbrowser         = Unknown Browser
> 
> Unfortunately the "Unknown Browser" replacement construct doesn't work at
> all for Polish because we need helper word ("application") before %1$S
> ("We’ve imported your passwords from application Unknown Browser into
> application Firefox").

It doesn't work amazingly in English as it is. The intention of capitalizing is that we fill in a name, and in that sense it should function the same as if the string was "360SE" or "Chrome" or "Internet Explorer" or "Microsoft Edge". Obviously I don't speak Polish, but I don't see why, if "Unknown Browser" doesn't work, "Google Chrome" *does* work. They're both arbitrary named identifiers. Why is one any better than the other?

The alternative is adding another 6 strings (and associated code) for a case that we should basically never hit (we would need to be importing from a browser which we don't know about (impossible), or fail to store the information of the browser from which we import, which essentially shouldn't happen), which IMO was the wrong trade-off, so that's not what I implemented.
(In reply to :Gijs from comment #23)
> It doesn't work amazingly in English as it is. The intention of capitalizing
> is that we fill in a name, and in that sense it should function the same as
> if the string was "360SE" or "Chrome" or "Internet Explorer" or "Microsoft
> Edge". Obviously I don't speak Polish, but I don't see why, if "Unknown
> Browser" doesn't work, "Google Chrome" *does* work. They're both arbitrary
> named identifiers. Why is one any better than the other?

Because of helper word and declension it sounds worse in Polish while in English it almost (except capitalizing) forms natural sentence.

> The alternative is adding another 6 strings (and associated code) for a case
> that we should basically never hit (we would need to be importing from a
> browser which we don't know about (impossible)

I don't suggest adding another 6 strings but is that a browser then (or a thing)?

> or fail to store the
> information of the browser from which we import, which essentially shouldn't
> happen), which IMO was the wrong trade-off, so that's not what I implemented.

There are different types of information that could be stored, like name, path and so on…
Verified as fixed on Windows 10 with latest Nightly 54.0a1, Build ID 20170221030205. Received new steps from Gijs:

1. Create new profile.
2. In the Firefox installation folder, create new folder distribution, and add in it distribution.ini file with content from https://bug1326252.bmoattachments.org/attachment.cgi?id=8825884
3. Make IE/Chrome/Safari/Edge the default browser
4. close nightly and run it from the commandline with:
path/to/firefox.exe --no-remote -P <profile> -migration

The new message from automigration infobar is displayed: "Pick up where you left off. We've imported these sites and your bookmarks, history, and passwords from Google Chrome into Nightly"
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(brindusa.tot)
You need to log in before you can comment on or make changes to this bug.