The default bug view has changed. See this FAQ.

Move "File->Import" to the Library

VERIFIED FIXED in Firefox 7

Status

()

Firefox
Menus
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: faaborg, Assigned: Steffen Wilberg)

Tracking

(Blocks: 2 bugs, {user-doc-needed})

Trunk
Firefox 7
user-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +
in-moztrap ?

Firefox Tracking Flags

(status2.0 wontfix)

Details

Attachments

(1 attachment, 14 obsolete attachments)

20.33 KB, patch
dao
: review+
Details | Diff | Splinter Review
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).
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.
(Reporter)

Comment 2

7 years ago
Then let's just remap this import over there (I actually always thought they were the same).
Cc'ing Dietrich and Marco, so we can make sure that gets addressed.
Whiteboard: [target-betaN]
(Reporter)

Comment 4

6 years ago
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/
Created attachment 506633 [details] [diff] [review]
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.
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...
Assignee: nobody → brian.carpenter
Created attachment 506639 [details] [diff] [review]
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.
Attachment #506633 - Attachment is obsolete: true
Created attachment 506641 [details] [diff] [review]
Follow up patch

This is a followup patch which removes the the Bookmark All Tabs and Page Info key from browser-sets.inc.
Created attachment 506642 [details] [diff] [review]
Removes related information from browser.dtd

This final patch removes the related information from the browser.dtd.
Created attachment 506648 [details] [diff] [review]
Removes Import from Browser Menu

This patch removes the Import from the browser menu (browser-menubar.inc) and from the browser.dtd.
Attachment #506639 - Attachment is obsolete: true
Attachment #506641 - Attachment is obsolete: true
Attachment #506642 - Attachment is obsolete: true
Attachment #506648 - Flags: review?

Updated

6 years ago
Attachment #506648 - Flags: review? → review?(dolske)
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 .
Attachment #506648 - Flags: review?(dolske) → review-
(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.
Created attachment 506689 [details] [diff] [review]
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. =)
Attachment #506648 - Attachment is obsolete: true
Attachment #506689 - Flags: review?(dolske)
(Assignee)

Comment 14

6 years ago
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
Keywords: uiwanted
Version: unspecified → Trunk
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.)
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)?
(Assignee)

Comment 17

6 years ago
(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?
Wasn't aware of that, yeah, let's go with two import commands in the library window.
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.
Flags: in-litmus?
(Assignee)

Comment 20

6 years ago
Brian, do you want to take that as well?
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.
Attachment #506689 - Flags: review?(dolske) → review-
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.
(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.
(Assignee)

Updated

6 years ago
Assignee: brian.carpenter → steffen.wilberg
Keywords: uiwanted
Summary: Remove "File Import..." from the File menu → Move "File->Import" to the Library
(Assignee)

Comment 24

6 years ago
Created attachment 507673 [details] [diff] [review]
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.
Attachment #506689 - Attachment is obsolete: true
Attachment #507673 - Flags: review?(gavin.sharp)
Attachment #507673 - Flags: feedback?(dolske)
(Assignee)

Comment 25

6 years ago
Created attachment 507686 [details] [diff] [review]
move "Import" to Library, v1.1

Removed debug leftover :-/
Attachment #507673 - Attachment is obsolete: true
Attachment #507686 - Flags: review?(gavin.sharp)
Attachment #507686 - Flags: feedback?(dolske)
Attachment #507673 - Flags: review?(gavin.sharp)
Attachment #507673 - Flags: feedback?(dolske)
(Assignee)

Comment 26

6 years ago
Created attachment 508871 [details] [diff] [review]
v1.1, unbitrotted

Unbitrotted (context has changed).
Attachment #507686 - Attachment is obsolete: true
Attachment #508871 - Flags: review?(gavin.sharp)
Attachment #508871 - Flags: feedback?(dolske)
Attachment #507686 - Flags: review?(gavin.sharp)
Attachment #507686 - Flags: feedback?(dolske)
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Attachment #508871 - Flags: review?(gavin.sharp)
Attachment #508871 - Flags: review+
Attachment #508871 - Flags: feedback?(dolske)
Attachment #508871 - Flags: approval2.0+
Beta 12 will be closing soon, please land ASAP.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9698ac3f1c61
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed → user-doc-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
Blocks: 633716

Comment 29

6 years ago
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?
-> backed out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 31

6 years ago
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.
Apparently this killed mochitest-other on OS X, as well.
status2.0: --- → wontfix
Target Milestone: Firefox 4.0b12 → ---

Updated

6 years ago
Attachment #508871 - Flags: approval2.0+

Updated

6 years ago
Duplicate of this bug: 513167
(Assignee)

Comment 34

6 years ago
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?
(Assignee)

Comment 35

6 years ago
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.
History and Bookmark menus don't have access key in order to allow history entries and bookmarks to be accessed with a key stroke.
(Assignee)

Comment 37

6 years ago
Ah, I see.

So how about adding "Import" without an accesskey?
I think the window of opportunity for this bug has closed. It can be fixed properly after Firefox 4 has branched.
(Assignee)

Comment 39

6 years ago
Created attachment 523399 [details] [diff] [review]
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)
Attachment #508871 - Attachment is obsolete: true
Attachment #523399 - Flags: review?(dolske)
(Assignee)

Comment 40

6 years ago
Created attachment 528069 [details] [diff] [review]
v1.2, unbitrotted

Alex, please comment on the strings I proposed in comment 39.
Attachment #523399 - Attachment is obsolete: true
Attachment #528069 - Flags: ui-review?(faaborg)
Attachment #523399 - Flags: review?(dolske)

Updated

6 years ago
Whiteboard: [target-betaN]
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 on attachment 528069 [details] [diff] [review]
v1.2, unbitrotted

ui-r+ with the strings tweaked from the last comment
Attachment #528069 - Flags: ui-review?(faaborg) → ui-review+
(Assignee)

Comment 43

6 years ago
Created attachment 538365 [details] [diff] [review]
patch v1.3, with new string from comment 42

"Import from Another Browser…" (accesskey A) per comment 42.
Attachment #528069 - Attachment is obsolete: true
Attachment #538365 - Flags: ui-review+
Attachment #538365 - Flags: review?(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?
Attachment #538365 - Flags: review?(dao) → review-
(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.
(Assignee)

Comment 46

6 years ago
Created attachment 540359 [details] [diff] [review]
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.
Attachment #538365 - Attachment is obsolete: true
Attachment #540359 - Flags: review?(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
Attachment #540359 - Flags: review?(dao) → review-
(Assignee)

Comment 48

6 years ago
Created attachment 540918 [details] [diff] [review]
patch v2.1

Addressed comment 47.
Attachment #540359 - Attachment is obsolete: true
Attachment #540918 - Flags: review?(dao)
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.
(Assignee)

Comment 50

6 years ago
Created attachment 540928 [details] [diff] [review]
patch v2.2

Thanks for the comments.
Attachment #540918 - Attachment is obsolete: true
Attachment #540928 - Flags: review?(dao)
Attachment #540918 - Flags: review?(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.
Attachment #540928 - Flags: review?(dao) → review+
(Assignee)

Comment 52

6 years ago
I went with "Import Data from Another Browser…".
http://hg.mozilla.org/integration/mozilla-inbound/rev/08734613c1ab
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/08734613c1ab
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Depends on: 668647
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.
Status: RESOLVED → VERIFIED
Once we have merged to mozilla-aurora we will have to update the Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=16699
Flags: in-litmus? → in-litmus?(simona.marcu)
Depends on: 672459
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
https://litmus.mozilla.org/show_test.cgi?id=16699 - the test case is updated
Flags: in-litmus?(simona.marcu) → in-litmus+
(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.
Flags: in-litmus+ → in-litmus?(simona.marcu)
I wonder if this even needs an update in our new manual testcase management system.
Flags: in-moztrap?(simona.marcu)
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.
Flags: in-litmus?(simona.marcu) → in-litmus+
You need to log in before you can comment on or make changes to this bug.