611568 - Move "File->Import" to the Library

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug is to remove "File Import..."

The rationale is that this command had extremely low usage in our usability
metrics coming from test pilot, especially relative to the surrounding very
high usage menu items.

The user has multiple other ways to perform this command, including:
-The wizard that opens when Firefox first launches from a fresh installation
-A toolbar button in the Library window (note this patch also needs to adjust that string from "Import HTML" to just "Import..." since the user has both options in the wizard anyway).

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The importer in the Library doesn't allow the user to import passwords, cookies, history, and preferences. We would loose an important import mechanism here.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Then let's just remap this import over there (I actually always thought they were the same).

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Cc'ing Dietrich and Marco, so we can make sure that gets addressed.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

The UX team is very eager to get this bug landed over the next few days in order to make Beta 11.  If anyone can get a patch for this bug posted soon, we will push hard for reviews and approval (even though this isn't blocking).

You can view all of the related bugs to clean up the traditional menu bar here: http://areweprettyyet.com/4/traditionalMenu/

Comment 5

[Brian Carpenter [:geeknik]]

Attachment: browser-menubar.inc patch for #611568 and #565564

Is this what you're looking for? If so it knocks out two bugs, this one and Bug #565564.

Comment 6

[Brian Carpenter [:geeknik]]

I just realized that in the Library window "this patch also needs to adjust that string from "Import HTML" to just "Import..." since the user has both options in the wizard anyway). However, I'm having trouble finding where this is at. I mean, I think it's in http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.xul, but...

Comment 7

[Brian Carpenter [:geeknik]]

Attachment: Patch for this bug and 4 others, see comment.

Partial for Bug #611568, total for #565564, #611569, #607227, #607230. I did an all in one for simplicity. I will knock out #607232 and #607234 next.

Comment 8

[Brian Carpenter [:geeknik]]

Attachment: Follow up patch

This is a followup patch which removes the the Bookmark All Tabs and Page Info key from browser-sets.inc.

Comment 9

[Brian Carpenter [:geeknik]]

Attachment: Removes related information from browser.dtd

This final patch removes the related information from the browser.dtd.

Comment 10

[Brian Carpenter [:geeknik]]

Attachment: Removes Import from Browser Menu

This patch removes the Import from the browser menu (browser-menubar.inc) and from the browser.dtd.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 506648 [details] [diff] [review]
Removes Import from Browser Menu

This seems to be the only caller to BrowserImport(), so you should probably remove that too (though perhaps not if this is actually going to land for 4.0). There are two references to this menu item in browser.js, and also a reference in browser_privatebrowsing_import.js .

Comment 12

[(no longer active)]

(In reply to comment #11)
> Comment on attachment 506648 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=506648
> Removes Import from Browser Menu
> 
> This seems to be the only caller to BrowserImport(), so you should probably
> remove that too (though perhaps not if this is actually going to land for 4.0).
> There are two references to this menu item in browser.js, and also a reference
> in browser_privatebrowsing_import.js .
Actually, this patch should remove the browser_privatebrowsing_import.js test completely, since its only purpose is to make sure that menu item gets disabled inside the private browsing mode.

Comment 13

[Brian Carpenter [:geeknik]]

Attachment: Updated Patch

This patch is the same as the original, but also removes the browser_privatebrowsing_import.js test and updates the Makefile.in. As far as the browser.js, which one? I counted quite a few. =)

Comment 14

[Steffen Wilberg]

This doesn't address comments 1, 2, and 11:
If we want to keep the possibility to import passwords,
cookies, history, and preferences, we need to make that available through Library --> Import and Backup button --> Import.

Both that and main menu File -> Import open chrome://browser/content/migration/migration.xul, the latter with "bookmarks" as its final argument, thus only migrating bookmarks, but also allowing to import an HTML file:
http://hg.mozilla.org/mozilla-central/annotate/6a097e294828/browser/components/places/content/places.js#l368
http://hg.mozilla.org/mozilla-central/annotate/6a097e294828/browser/base/content/browser.js#l2537

We need some input from the UX team here. I'd suggest to
1. relabel "Import as HTML..." to "Import Bookmarks as HTML...", which should directly open a file open dialog,
2. add a new menu item "Import...", opening the full migration wizard like File-->Impoort currently does.
Requires a new string though.


> As far as the browser.js, which one? I counted quite a few. =)
http://mxr.mozilla.org/mozilla-central/search?string=menu_import&find=browser.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

I know we've already discussed various options in bugzilla somewhere, but I'm not entirely sure which bug now (looks like it wasn't this one, which would have been handy).

In addition to removing the menu item, we should rename "Import as HTML..." to just "Import..." and (same string), and only give users access to the full migration wizard.  That work can be handled in a separate bug.

This somewhat hides the feature, but the feature doesn't really work in the current marketplace (we don't support chrome, or delicious, etc.)

Comment 16

[Alex Faaborg [:faaborg] (Firefox UX)]

Marco: do you know if we have a bug for adjusting the library to invoke the full wizard (and changing which string we use there)?

Comment 17

[Steffen Wilberg]

(In reply to comment #15)
> we should rename "Import as HTML..." to
> just "Import..." and (same string), and only give users access to the full
> migration wizard.
Besides the label change, that's a one-liner, see comment 14.
But we'd lose the ability to import/export the bookmarks file as HTML, as the full migration wizard doesn't support that. Do we want to do that?

Comment 18

[Alex Faaborg [:faaborg] (Firefox UX)]

Wasn't aware of that, yeah, let's go with two import commands in the library window.

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

The fact that "Import HMTL" just means bookmarks isn't ideal, but probably isn't worth breaking string freeze.  We need to significantly revisit our import UI at some point soon anyway.

Comment 20

[Steffen Wilberg]

Brian, do you want to take that as well?

Comment 21

[Justin Dolske [:Dolske]]

Comment on attachment 506689 [details] [diff] [review]
Updated Patch

Still need to figure out what where the "import from another browser" functionality should move to and strings impact. The Places organizer doesn't seem like a very good place for it, but between this not being likely to be used and already of marginal use in the current marketplace (comment 15), I'm kind of meh on where it ends up. A button somewhere under Options --> Advanced might be a nice dumping ground, but I don't see a very good fit there either (and it's cramped).

>--- a/browser/base/content/browser-menubar.inc	Mon Jan 24 19:47:52 2011 -0800
...
>                 <menuitem id="menu_print"
>                           label="&printCmd.label;"
>                           accesskey="&printCmd.accesskey;"
>                           key="printKb"
>                           command="cmd_print"/>
>-                <menuseparator/>

Should keep this menuseparator, so the printing related items are demarked from the items above and below them.

>-                <menuitem id="menu_import"

Need to fix onEnterPrivateBrowsing() and onExitPrivateBrowsing() [in browser.js], to not attempt to disable/enable this item.

Comment 22

[Justin Dolske [:Dolske]]

Talked with Faaborg jsut to be clear, current recommendation is for the places organizer to have both "Import From HTML" (does what it does right now), and "Import" (does what File --> Import did).

So, sigh, that would mean keeping the existing strings, making the places organizer use them for the new command, and fixing the private browsing stuff to ensure this command is disabled while in PB mode.

How about just getting a driver-level call on flat out removing this (and maybe someone write a tiny Jetpack to allow reexpose it)? I think most of us agree that the import functionality is already very lacking, and isn't likely to be missed much.

Comment 23

[(no longer active)]

(In reply to comment #22)
> So, sigh, that would mean keeping the existing strings, making the places
> organizer use them for the new command, and fixing the private browsing stuff
> to ensure this command is disabled while in PB mode.

The command inside the places should not be disabled inside the private browsing mode, as it's not right now.  The original rationale for this was that the command is not available through the primary UI.

Comment 24

[Steffen Wilberg]

Attachment: move Import to the Library, v1

"Import HTML..." used to let you choose between either importing from another browser or and importing an HTML file. If you chose the latter, it would open a file picker. Now it directly opens the file picker to let you import the HTML file.

The new "Import..." in the Library does the same as the old File->Import, letting you choose which sorts of data to import, including bookmarks.
I moved BrowserImport() to places.js, added a comment, used the Cc/Ci shorthands, but left it untouched otherwise.

Using the existing string from browser.dtd (which is already referenced by places.xul), there's no string change required. Ideally, "Import HTML..." would be relabeled to "Import Bookmarks HTML...", but that's not worth breaking string freeze, see comment 19.

(In reply to comment #21)
> Should keep this menuseparator, so the printing related items are
> demarked from the items above and below them.
Done, and removed the #ifndef XP_MACOSX <menuseparator/> instead.

> >-                <menuitem id="menu_import"
> 
> Need to fix onEnterPrivateBrowsing() and onExitPrivateBrowsing() [in
> browser.js], to not attempt to disable/enable this item.
Done.

(In reply to comment #23)
> The command inside the places should not be disabled inside the private
> browsing mode, as it's not right now.  The original rationale for this was
> that the command is not available through the primary UI.
Didn't replicate the menu item disabling in PB mode, and removed the test.

Comment 25

[Steffen Wilberg]

Attachment: move "Import" to Library, v1.1

Removed debug leftover :-/

Comment 26

[Steffen Wilberg]

Attachment: v1.1, unbitrotted

Unbitrotted (context has changed).

Comment 27

[Justin Dolske [:Dolske]]

Beta 12 will be closing soon, please land ASAP.

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/9698ac3f1c61

Comment 29

[u60234]

This has localization impact. In en-US and sv-SE, and probably other locales, there are now two menu items under "Import and Backup" that has "I" as access key. Fix or notify localizers?

Comment 30

[Dão Gottwald [:dao]]

-> backed out

Comment 31

[Steffen Wilberg]

Ah crap, I tried to fix this using existing strings, but didn't notice the double accesskey.

String changes are out of the question after "the last string landed".
So the only way to fix this bug for Firefox 4 would be to tolerate the double accesskey in this rather hidden place. You could still reach the menu item by pressing Alt+i to open the menu, and pressing "i" twice: once for "Import HTML..." and once for "Import..." (tested on Windows and Linux).

Either way, I'll fix this properly for the next release.

Comment 32

[Dão Gottwald [:dao]]

Apparently this killed mochitest-other on OS X, as well.

Comment 34

[Steffen Wilberg]

Comment on attachment 508871 [details] [diff] [review]
v1.1, unbitrotted

Right, I messed that up as well. ;-(
I uncommented the #ifdef XP_MACOSX, resulting in a broken Library window, and the syntax error pointing right at it:

>+  browserImport: function PO_browserImport() {
>+#ifdef XP_MACOSX
>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             .getService(Ci.nsIWindowMediator);
The double "." before getService causes the syntax error breaking the Mac tests.

I then searched the log.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Test timed out

is not helpful at all. But several lines earlier, I finally found it:

TEST-INFO | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js | Console message: [JavaScript Error: "syntax error" {file: "chrome://browser/content/places/places.js" line: 373 column: 13 source: "             .getService(Ci.nsIWindowMediator);
"}]

Why do tests not fail when a syntax error occurs, but time out later on?

Comment 35

[Steffen Wilberg]

Dao or Dolske, can we do without the accesskey instead of duplicating it?

I just noticed that the entire History and Bookmarks menus lack accesskeys altogether, and I can't even find a bug about that...

Users can still navigate the menu using the arrow keys. You don't import bookmarks + passwords + cookies + history + preferences every day.

Comment 36

[Dão Gottwald [:dao]]

History and Bookmark menus don't have access key in order to allow history entries and bookmarks to be accessed with a key stroke.

Comment 37

[Steffen Wilberg]

Ah, I see.

So how about adding "Import" without an accesskey?

Comment 38

[Dão Gottwald [:dao]]

I think the window of opportunity for this bug has closed. It can be fixed properly after Firefox 4 has branched.

Comment 39

[Steffen Wilberg]

Attachment: v 1.2, new strings

Suggested new strings are:
Import Bookmarks from HTML… (accesskey I)
Export Bookmarks to HTML… (E)
Import Bookmarks, History, Passwords… (m)

Comment 40

[Steffen Wilberg]

Attachment: v1.2, unbitrotted

Alex, please comment on the strings I proposed in comment 39.

Comment 41

[Alex Faaborg [:faaborg] (Firefox UX)]

We can't really use commas in a menu command (at least I can't think of any existing menu commands that use commas)  How about:

Suggested new strings are:
Import Bookmarks from HTML… (accesskey I)
Export Bookmarks to HTML… (E)
Import from Another Browser (A)

Comment 42

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 528069 [details] [diff] [review]
v1.2, unbitrotted

ui-r+ with the strings tweaked from the last comment

Comment 43

[Steffen Wilberg]

Attachment: patch v1.3, with new string from comment 42

"Import from Another Browser…" (accesskey A) per comment 42.

Comment 44

[Dão Gottwald [:dao]]

Comment on attachment 538365 [details] [diff] [review]
patch v1.3, with new string from comment 42

It looks like this would regress bug 464736... comment 23 seems wrong, since importing history and cookies actually didn't work in PB mode.

>   /**
>-   * Show the migration wizard for importing from a file.
>+   * Show the migration wizard for importing passwords,
>+     cookies, history, preferences, and bookmarks.
>    */

nit: add another asterisk

>-<!ENTITY cmd.exportHTML.label           "Export HTMLâ¦">
>+<!ENTITY cmd.exportHTML2.label          "Export Bookmarks to HTMLâ¦">

Can you change the entity name to something more sensible like exportBookmarksToHTML?

Comment 45

[(no longer active)]

(In reply to comment #44)
> Comment on attachment 538365 [details] [diff] [review] [review]
> patch v1.3, with new string from comment 42
> 
> It looks like this would regress bug 464736... comment 23 seems wrong, since
> importing history and cookies actually didn't work in PB mode.

Ah, right, yes.  This menu item should continue to be disabled in private browsing mode.  Sorry for my mistake.

Comment 46

[Steffen Wilberg]

Attachment: patch v2: menu item disabled in PB mode

Added the code to disable the menu item in private browsing mode, and rewrote the test.

The test actually tests the #OrganizerCommand_browserImport command instead of the #browserImport menuitem; the latter somehow failed some of the tests.

(In reply to comment #44)
> >   /**
> >-   * Show the migration wizard for importing from a file.
> >+   * Show the migration wizard for importing passwords,
> >+     cookies, history, preferences, and bookmarks.
> >    */
> 
> nit: add another asterisk
Done.

> >-<!ENTITY cmd.exportHTML.label           "Export HTMLâ¦">
> >+<!ENTITY cmd.exportHTML2.label          "Export Bookmarks to HTMLâ¦">
> 
> Can you change the entity name to something more sensible like
> exportBookmarksToHTML?
Done: importBookmarksFromHTML.label, exportBookmarksToHTML.label, importOtherBrowser.label.

Comment 47

[Dão Gottwald [:dao]]

Comment on attachment 540359 [details] [diff] [review]
patch v2: menu item disabled in PB mode

>+  browserImport: function PO_browserImport() {
>+#ifdef XP_MACOSX
>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+    var win = wm.getMostRecentWindow("Browser:MigrationWizard");

use Services.wm

>+let gPOPrivateBrowsingUI = {

"gPO" is an irritatingly obscure prefix. "PO" seems redundant, too, since this is places.js.

>+  _privateBrowsingService: null,
>+  _cmd_import: null,
>+  _obs: null,
>+
>+  init: function PO_PB_init() {
>+    this._cmd_import = document.getElementById("OrganizerCommand_browserImport");
>+
>+    this._privateBrowsingService = Cc["@mozilla.org/privatebrowsing;1"].
>+                                   getService(Ci.nsIPrivateBrowsingService);
>+    if (this._privateBrowsingService.privateBrowsingEnabled)
>+      this.updateUI(true);

You're not using this._privateBrowsingService outside of this method.

>+    this._obs = Cc["@mozilla.org/observer-service;1"].
>+                getService(Ci.nsIObserverService);
>+    this._obs.addObserver(this, "private-browsing", false);
>+  },
>+
>+  uninit: function PO_PB_uninit() {
>+    this._obs.removeObserver(this, "private-browsing");

use Services.obs

Comment 48

[Steffen Wilberg]

Attachment: patch v2.1

Addressed comment 47.

Comment 49

[Marco Bonardo [:mak]]

Comment on attachment 540918 [details] [diff] [review]
patch v2.1

Review of attachment 540918 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for taking care of this, just some drive-by comments

::: browser/components/places/content/places.js
@@ +373,2 @@
>     */
> +  browserImport: function PO_browserImport() {

since we have importFromFile, while is not this called importFromBrowser?

@@ +378,5 @@
> +      win.focus();
> +    else {
> +      window.openDialog("chrome://browser/content/migration/migration.xul",
> +                        "migration", "centerscreen,chrome,resizable=no");
> +    }

if you brace one part of an if/elseif/else you want to brace all of them.

@@ +382,5 @@
> +    }
> +#else
> +    window.openDialog("chrome://browser/content/migration/migration.xul",
> +                      "migration", "modal,centerscreen,chrome,resizable=no");
> +#endif

Btw, I'd rather (pseudocode)
ifdef MAC
// On Mac the window is not modal.
if (win) {
  win.focus();
  return;
}
let features = "..."
#else
let features = "..."
#endif
window.openDialog("chrome://browser/content/migration/migration.xul",
		  "migration", features);

@@ +1352,5 @@
> +/**
> + * Disables the "Import and Backup->Import From Another Browser" menu item
> + * in private browsing mode.
> + */
> +let gPrivateBrowsingUI = {

this name makes no sense, this is not a private browsing ui.
gPrivateBrowsingListener or gPrivateBrowsingHandler sounds better to me.

@@ +1372,5 @@
> +  },
> +
> +  observe: function PO_PB_observe(aSubject, aTopic, aData) {
> +    if (aTopic != "private-browsing")
> +      return;

this seems useless, tests would fail if this would ever happen.

Comment 50

[Steffen Wilberg]

Attachment: patch v2.2

Thanks for the comments.

Comment 51

[Dão Gottwald [:dao]]

Comment on attachment 540928 [details] [diff] [review]
patch v2.2

>+<!ENTITY importBookmarksFromHTML.label     "Import Bookmarks from HTML…">
>+<!ENTITY importBookmarksFromHTML.accesskey "I">
>+<!ENTITY exportBookmarksToHTML.label       "Export Bookmarks to HTML…">
>+<!ENTITY exportBookmarksToHTML.accesskey   "E">
>+<!ENTITY importOtherBrowser.label          "Import from Another Browser…">

The last one should probably be called "Import Data from Another Browser…" or similar for it to contrast with "Bookmarks" from above.

Comment 52

[Steffen Wilberg]

I went with "Import Data from Another Browser…".
http://hg.mozilla.org/integration/mozilla-inbound/rev/08734613c1ab

Comment 53

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/08734613c1ab

Comment 54

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 5.1; rv:7.0a1) Gecko/20110630 Firefox/7.0a1

Verified that "Import" from the File menu is now moved to the Library, on Windows XP, Windows 7, MacOS X 10.6 and Ubuntu.

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Once we have merged to mozilla-aurora we will have to update the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=16699

Comment 56

[Simona Badau, Desktop QA]

https://litmus.mozilla.org/show_test.cgi?id=16699 - the test case is updated

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Simona B [QA] from comment #56)
> https://litmus.mozilla.org/show_test.cgi?id=16699 - the test case is updated

This should not only be updated for Aurora. Given that this bug has been fixed for Firefox 7, we should also update the beta, latest release, and the esr10 branch.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I wonder if this even needs an update in our new manual testcase management system.

Comment 59

[Simona Badau, Desktop QA]

The test case was updated in Litmus long time ago (2011-07-22 02:21:07) for all the branches, just forgot to change the Litmus flag.

The test case has not been imported to MozTrap yet, as most of the test cases from Litmus. I'm not sure yet if all the test cases will be imported. I shall investigate further on this.

Comment 60

[Simona Badau, Desktop QA]

Long-time ago about the moztrap request. Removing it since Moztrap is no longer a used tool.