Closed Bug 1522856 Opened 9 months ago Closed 7 months ago

Add an "import" option to the file menu to make browser migration discoverable.

Categories

(Firefox :: Migration, enhancement, P3)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 67
Tracking Status
relnote-firefox --- 67+
firefox67 --- verified
firefox68 --- verified

People

(Reporter: mhoye, Assigned: soeren.hentzschel, Mentored)

Details

(Whiteboard: [lang=xul] [lang=js])

Attachments

(1 file)

If the user misses the opportunity to import their data from other browsers during the install, or because they've got a very old profile and don't meet the other criteria we have to kick off the import process, it's difficult to discover later.

I propose that we add an "import" menu option under the File menu to let new or returning users kick off the import process directly.

Note that the "File" menu is a macOS-only thing. Please think about accessibility on other platforms too.

I would be fine with this since that seems like a natural place compared to other applications and that menu isn't too full. The other option would be the Tools menu.

(In reply to Mike Hommey [:glandium] from comment #1)

Note that the "File" menu is a macOS-only thing. Please think about accessibility on other platforms too.

Let's start by exposing it there and see how it goes. Putting it in the menu panel is a bigger deal. The menu is available on other platforms with the Alt key still I believe, yes, I get that it's not very discoverable.

Implementation:


document.getElementById("cmd_…").doCommand();

ok(Services.wm.getMostRecentWindow("Browser:MigrationWizard"), "Migrator window opened")

Mentor: MattN+bmo
Severity: normal → enhancement
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [lang=xul] [lang=js]

Does this look good-first-buggish to you?

I had it on there originally but then once I typed all the steps up I worried that a new contributor could get tripped up at any of those steps so ended up removing it.

Hi,

I submitted a patch, but it's still WIP. I have a few issues / questions:

  1. The method updateFileMenuImportUIVisibility() does not work as expected. Setting the hidden attribute to the menuitem if the DisableProfileImport policy is enabled works. But setting the disabled attribute instead of the hidden attribute does not work, the menuitem is still enabled. Why does is not work?

  2. The browser-chrome test is still missing. Where do I add the test?

  3. In https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/browser/components/migration/MigrationUtils.jsm#1133-1138 there are already six entry points, but in in the description in https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/toolkit/components/telemetry/Histograms.json#6327,6335 there are only five. I only added the new entry point. Should the missing entry point (which is unrelated to this bug) also be added in this patch?

  4. The menuitem works. I can click the menuitem and the import wizard opens. But there is an error in the browser console when opening the wizard:

Handler function DebuggerProgressListener.prototype.onWindowCreated threw an exception: TypeError: window is undefined Stack: getWindowID@resource://devtools/server/actors/targets/browsing-context.js:54:3 DebuggerProgressListener.prototype.onWindowCreated<@resource://devtools/server/actors/targets/browsing-context.js:1572:21 exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14 _fireEvent@chrome://global/content/bindings/wizard.xml:418:26 set_currentPage@chrome://global/content/bindings/wizard.xml:94:11 advance@chrome://global/content/bindings/wizard.xml:281:15 wizard_XBL_Constructor@chrome://global/content/bindings/wizard.xml:196:9 Line: 54, column: 3

Is this something I have to fix or this error unrelated?

Thank you!

Thanks for the WIP patch! I'll take a look at this later… using needinfo to remind me.

Flags: needinfo?(MattN+bmo)

(In reply to Sören Hentzschel from comment #6)

I submitted a patch, but it's still WIP. I have a few issues / questions:

Awesome, thanks!

  1. The method updateFileMenuImportUIVisibility() does not work as expected. Setting the hidden attribute to the menuitem if the DisableProfileImport policy is enabled works. But setting the disabled attribute instead of the hidden attribute does not work, the menuitem is still enabled. Why does is not work?

Hmm… I'm not sure… which OS are you on? Did you try use the setter instead (….disabled =)?

  1. The browser-chrome test is still missing. Where do I add the test?

Normally I would say in the same directory as https://searchfox.org/mozilla-central/source/browser/base/content/test/general/browser_page_style_menu.js but we're trying not to add more there so I guess you can make a new "menubar" directory adjacent to "general"?

  1. In https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/browser/components/migration/MigrationUtils.jsm#1133-1138 there are already six entry points, but in in the description in https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/toolkit/components/telemetry/Histograms.json#6327,6335 there are only five. I only added the new entry point. Should the missing entry point (which is unrelated to this bug) also be added in this patch?

Yes, please.

  1. The menuitem works. I can click the menuitem and the import wizard opens. But there is an error in the browser console when opening the wizard:

Handler function DebuggerProgressListener.prototype.onWindowCreated threw an exception: TypeError: window is undefined Stack: getWindowID@resource://devtools/server/actors/targets/browsing-context.js:54:3 DebuggerProgressListener.prototype.onWindowCreated<@resource://devtools/server/actors/targets/browsing-context.js:1572:21 exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14 _fireEvent@chrome://global/content/bindings/wizard.xml:418:26 set_currentPage@chrome://global/content/bindings/wizard.xml:94:11 advance@chrome://global/content/bindings/wizard.xml:281:15 wizard_XBL_Constructor@chrome://global/content/bindings/wizard.xml:196:9 Line: 54, column: 3

Is this something I have to fix or this error unrelated?

I think that's unrelated and can be ignored assuming tests on Try server don't fail.

Flags: needinfo?(MattN+bmo)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

(In reply to Sören Hentzschel from comment #6)

  1. The method updateFileMenuImportUIVisibility() does not work as expected. Setting the hidden attribute to the menuitem if the DisableProfileImport policy is enabled works. But setting the disabled attribute instead of the hidden attribute does not work, the menuitem is still enabled. Why does is not work?

Hmm… I'm not sure… which OS are you on?

I am on macOS Mojave (10.14.3).

Did you try use the setter instead (….disabled =)?

Yes, I tried it, but the menu item is still enabled.

I also added a console.log(menu); before and after the setter.

Before:

<menuitem id="menu_importFromAnotherBrowser" label="Import from Another Browser…" accesskey="I" command="cmd_importFromAnotherBrowser" checked="false">

after:

<menuitem id="menu_importFromAnotherBrowser" label="Import from Another Browser…" accesskey="I" command="cmd_importFromAnotherBrowser" checked="false" disabled="true">

… but the inspector doesn't show the disabled attribute and the menuitem is not disabled. :-/

ni? for comment #9. Do you have an idea?

I also need some help with the test. I updated the revision in Phabricator where you can find the current state. As you can see I already added the test and I can execute the test. But I don't know how I can test the behaviour.

That's the current code:

add_task(async function file_menu_import_wizard() {
    function sleep(ms) {
      return new Promise(resolve => setTimeout(resolve, ms));
    }

    document.getElementById("menu_importFromAnotherBrowser").doCommand();

    await sleep(3000);

    ok(Services.wm.getMostRecentWindow("Browser:MigrationWizard"), "Migrator window opened");
});

I added the following temporary code:

function sleep(ms) {
  return new Promise(resolve => setTimeout(resolve, ms));
}

and

await sleep(3000);

… only to see the import wizard for a few seconds so that I can verify that the following line works:

document.getElementById("menu_importFromAnotherBrowser").doCommand();

Otherwise the wizard will be closed immediately. This will, of course, not be in the final patch.

When I execute the test with ./mach mochitest -f browser browser/base/content/test/menubar/browser_file_menu_import_wizard.js I get the following result:

FAIL Found an unexpected Browser:MigrationWizard at the end of test run -

I am not familar with browser chrome tests at all. Do you have any hint? ;-)

Flags: needinfo?(MattN+bmo)

I replied on Phabricator… I think the issue is that you're using @command but then setting @disabled directly on the <menuitem> instead of the <command>.

Flags: needinfo?(MattN+bmo)
Assignee: nobody → soeren.hentzschel

This is ready to land… do you have level 1 commit access to do a try push? If so, I would recommend running all mochitests on opt builds.

Flags: needinfo?(soeren.hentzschel)

No, I don't have level 1 commit access (as far as I know).

Flags: needinfo?(soeren.hentzschel)

Looks like there are test failures, can you investigate them?

Flags: needinfo?(soeren.hentzschel)

Can you help me to investigate these failures? The tests work on my local machine with macOS and the try run don't show failures on macOS, only on Windows and Linux. But I don't have Windows and Linux and I have no idea what's wrong with the tests. :-/

Flags: needinfo?(soeren.hentzschel) → needinfo?(MattN+bmo)

Looks like neither of the fixes worked so I'll have to do some more investigation on a Windows machine.

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfeea1a4375a
Add an "import" option to the file menu to make browser migration discoverable. r=MattN,felipe
Flags: needinfo?(MattN+bmo)
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

Added a Nightly release note with this wording:
The Import Data from Another Browser feature is now also available from the File menu

I confirm that the "Import Data from Another Browser" button is now available in Nightly v68.0a1, Beta v67.0b3, but not Release v66.0, on all three main OSes. Considering all the above, I will validate this bug. Thank you.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.