Bookmark Import and Restore do not intelligently handle two different bookmark formats

VERIFIED FIXED in Firefox 3

Status

()

defect
P2
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: smrank, Assigned: dietrich)

Tracking

({late-l10n})

Trunk
Firefox 3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [9 strings])

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031323 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5pre) Gecko/2008031323 Minefield/3.0b5pre

Now that JSON bookmark backup has landed, there are two potential bookmark file formats, JSON and HTML. The interface does not correctly handle a user selecting the wrong type of file.

Reproducible: Always

Steps to Reproduce:
1. Go to Bookmarks->Organize Bookmarks.
2. Go to Import and Backup->Backup and save a JSON backup somewhere.
3. Modify your bookmarks (e.g. add some new ones or delete some).
4. Go to Import and Backup->Import and choose From File then select the backup saved in step 2.

Actual Results:  
I get a new folder called JavaScript%20Shell%201.4 (I have a bookmark called this that links to a javascript: url) and my changes made in step 3 above are not undone.

Expected Results:  
My bookmarks should be restored to how they were before step 3 above.

A similar effect can be seen in reverse by exporting your bookmarks to HTML then using Restore->Choose File (which expects a JSON file), nothing appears to happen.

I think that "Restore->Choose File" should be removed and Import made to handle either type automatically. You could also combine Export and Backup into one option and allow picking the file type from the standard "Save as type" box in the save dialog.
Reporter

Comment 1

11 years ago
Requesting blocking because this UI can appear plain broken when using the wrong filetype with the wrong import/restore option, and is otherwise pretty confusing.
Flags: blocking-firefox3?
Version: unspecified → Trunk
Blocks: 384370
Assignee

Updated

11 years ago
Assignee: nobody → dietrich
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → Firefox 3 beta5
Assignee

Comment 2

11 years ago
Aside from having the options handle only the specific format, we need a clear indication in the UI that import/export means HTML (external use) and backup/restore means JSON (internal use).

I think that explicit visual/textual communication is preferable over error messages (ie: forcing the user into using trial and error to see which menu option they actually need) or the current "just works" approach, which has unexpected results depending on which menu-option+file-format combination is selected.
Keywords: uiwanted
Agree completely with comment 2, but I'm not sure what we can do at this point beyond restricting the file types and ..:

 - in the Import dialog, change the label of "From file" to "From HTML file"
 - in the Restore submenu, change label of "Choose file" to "From file"
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: uiwanted
Target Milestone: Firefox 3 beta5 → Firefox 3
Assignee

Comment 4

11 years ago
(In reply to comment #3)
> Agree completely with comment 2, but I'm not sure what we can do at this point
> beyond restricting the file types and ..:
> 

What do you think of this proposal?

https://bugzilla.mozilla.org/show_bug.cgi?id=405936#c9
Status: NEW → ASSIGNED
(In reply to comment #4)
> https://bugzilla.mozilla.org/show_bug.cgi?id=405936#c9

I like it, but suggest the following tweaks:

Import and Backup
 Backup...                     --> leads to file picker[1]
 Restore...>  YYYY-MM-DD 
              YYYY-MM-DD
              YYYY-MM-DD
              YYYY-MM-DD
              ----------
              Choose file...   --> leads to file picker[2]
 ------------
 Import Bookmarks...           --> leads to Import Wizard[3]
 Export Bookmarks...           --> leads to file picker[4]

[1] file type should be restricted to <blank> (as JSON isn't an option, aiui)
[2] we should add "(HTML)" to the "From file" option in this wizard
[3] file type should be restricted to HTM, HTML
[4] file type should be restricted to HTM, HTML

Assignee

Updated

11 years ago
Keywords: late-l10n
Assignee

Comment 6

11 years ago
Posted patch fix v1 (obsolete) — Splinter Review
Attachment #311423 - Flags: ui-review?(beltzner)
Attachment #311423 - Flags: review?(mano)
Assignee

Updated

11 years ago
Whiteboard: [has patch][needs review mano]
Assignee

Comment 7

11 years ago
Comment on attachment 311423 [details] [diff] [review]
fix v1

this implements beltzner's suggestion in comment #5, so removing ui-r request.
Attachment #311423 - Flags: ui-review?(beltzner)
Assignee

Updated

11 years ago
Blocks: 424389
Assignee

Updated

11 years ago
No longer blocks: 424389
Shouldn't that be "From a HTML File"?
It should be, yes.

Also, bug 424534 points out that we don't need the ellipses on "Restore...". That's a good point.
Assignee

Comment 10

11 years ago
Posted patch fix v2 (comments fixed) (obsolete) — Splinter Review
Attachment #311423 - Attachment is obsolete: true
Attachment #311465 - Flags: review?(mano)
Attachment #311423 - Flags: review?(mano)
Comment on attachment 311465 [details] [diff] [review]
fix v2 (comments fixed)

>Index: browser/locales/en-US/chrome/browser/places/places.dtd
>-<!ENTITY cmd.restore.label              "Restore…">
>+<!ENTITY cmd.restore.label              "Restore">

That'd leave 47 ellipsisees unchanged, though, http://mxr.mozilla.org/l10n/search?string=cmd.restore.label&find=places%2Fplaces\.dtd%24&findi=\.dtd%24&filter=&tree=l10n

Updated

11 years ago
Whiteboard: [has patch][needs review mano] → [has patch][needs review mano][9 strings]
+      if (!f.isHidden() && f.leafName.match(/^bookmarks-.+json?$/))

why json? with optional n, is .jso a valid extension?
Comment on attachment 311465 [details] [diff] [review]
fix v2 (comments fixed)

r=mano
Attachment #311465 - Flags: review?(mano) → review+
Assignee

Comment 14

11 years ago
Comment on attachment 311465 [details] [diff] [review]
fix v2 (comments fixed)

Requesting driver approval for late-l10n.
Attachment #311465 - Flags: approval1.9?
Assignee

Updated

11 years ago
Whiteboard: [has patch][needs review mano][9 strings] → [has patch][has review][needs approval][9 strings]
Assignee

Comment 15

11 years ago
(In reply to comment #12)
> +      if (!f.isHidden() && f.leafName.match(/^bookmarks-.+json?$/))
> 
> why json? with optional n, is .jso a valid extension?
> 

no, i'll post a new patch.
Assignee

Comment 16

11 years ago
Posted patch fix v3 (addresses comment #12 (obsolete) — Splinter Review
Attachment #311465 - Attachment is obsolete: true
Attachment #312930 - Flags: review?(mano)
Attachment #311465 - Flags: approval1.9?
Assignee

Updated

11 years ago
Whiteboard: [has patch][has review][needs approval][9 strings] → [9 strings][has patch][needs review mano]
Assignee

Updated

11 years ago
Attachment #312930 - Flags: approval1.9?
Assignee

Updated

11 years ago
Whiteboard: [9 strings][has patch][needs review mano] → [9 strings][has patch][needs review mano][needs approval]
Comment on attachment 312930 [details] [diff] [review]
fix v3 (addresses comment #12

r=mano, don't you want to include the part I've reviewed on the other bug here?
Attachment #312930 - Flags: review?(mano) → review+
Assignee

Comment 18

11 years ago
(In reply to comment #17)
> (From update of attachment 312930 [details] [diff] [review])
> r=mano, don't you want to include the part I've reviewed on the other bug here?
> 

After looking at that, I think it might be better to move the error prompt in PlacesUtils.restoreBookmarksFromFile() to the front-end. This means that:

1. no UI being generated out of the back-end, toolkit consumers can handle errors in their own way
2. we can use the proper window for the prompt
3. no restrictions on file extension on the backend, toolkit consumers can make their own determination about what's supported

If this sounds good, I'll combine these two patches and re-attach here.
Whiteboard: [9 strings][has patch][needs review mano][needs approval] → [9 strings][has patch][has reviews][needs approval]
Assignee

Comment 19

11 years ago
Posted patch fix v4 (obsolete) — Splinter Review
implements solution in comment #18, in addition to a few issues found while testing this:

- folds in fix for bug 424654
- moves error UI to the front-end
- removes a couple of unused duplicate strings
- removes latest backup when replacing (followup to bug 424389)
Attachment #312930 - Attachment is obsolete: true
Attachment #313144 - Flags: review?(mano)
Attachment #312930 - Flags: approval1.9?
Assignee

Comment 20

11 years ago
verification steps:

- menu re-arrange per comment #5
- migration screen says "From a HTML File"
- the file picker when importing, filters for HTML files
- the restore menu only shows JSON backups from the bookmarksbackup dir
- the file picker when restoring, filters for JSON files
- when restoring from a non json file, get the format not supported error prompt
- when restoring from a json file that's not parse-able, get the parse error prompt
- the file picker when backing up, filters for JSON files
- restore menu item does not have ellipsis
- new profile, immediately open library, open restore menu, confirm that bookmarksbackup directory is a directory
- back to a file on your desktop twice. confirm that the second backup replaces the first one in the bookmarksbackup directory
Assignee

Updated

11 years ago
Whiteboard: [9 strings][has patch][has reviews][needs approval] → [9 strings][has patch][needs review mano][needs approval]
Assignee

Updated

11 years ago
Attachment #313144 - Flags: approval1.9?
Comment on attachment 313144 [details] [diff] [review]
fix v4


>Index: browser/components/places/content/places.js
>===================================================================

>+  showErrorAlert: function PO_showErrorAlert(aMsg) {

nit: prefix it with an underscore.

>+    const BRANDING_BUNDLE_URI = "chrome://branding/locale/brand.properties";
>+    var brandShortName = Cc["@mozilla.org/intl/stringbundle;1"].
>+                         getService(Ci.nsIStringBundleService).
>+                         createBundle(BRANDING_BUNDLE_URI).
>+                         GetStringFromName("brandShortName");
>+

I'd prefer using a <stringbundle/>

>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+    var win = wm.getMostRecentWindow("Places:Organizer");
>+
>+    Cc["@mozilla.org/embedcomp/prompt-service;1"].
>+      getService(Ci.nsIPromptService).
>+      alert(win, brandShortName, aMsg);


you're already in the context of an organizer window, why do you use the window manger?


>Index: browser/locales/en-US/chrome/browser/places/places.dtd
>===================================================================

> <!ENTITY cmd.backup.label               "Backup…">
> <!ENTITY cmd.backup.accesskey           "B">
>-<!ENTITY cmd.restore.label              "Restore…">
>+<!ENTITY cmd.restore.label              "Restore">

reve the entity name please.
Attachment #313144 - Flags: review?(mano)
Attachment #313144 - Flags: review-
Attachment #313144 - Flags: approval1.9?
Whiteboard: [9 strings][has patch][needs review mano][needs approval] → [9 strings]
Assignee

Comment 22

11 years ago
Posted patch fix v5 - comments addressed (obsolete) — Splinter Review
Attachment #313144 - Attachment is obsolete: true
Attachment #313203 - Flags: review?(mano)
Assignee

Updated

11 years ago
Whiteboard: [9 strings] → [9 strings][has patch][needs review mano]
Comment on attachment 313203 [details] [diff] [review]
fix v5 - comments addressed

>-<!ENTITY importFromFile.label           "From File">
>-<!ENTITY importFromFile.accesskey       "F">
>+<!ENTITY importFromHTMLFile.label       "From a HTML File">
>+<!ENTITY importFromHTMLFile.accesskey   "F">

"From an HTML file"
Attachment #313203 - Flags: ui-review?(beltzner)
Comment on attachment 313203 [details] [diff] [review]
fix v5 - comments addressed

can fix the a/an/ on checkin, it wouldn't be late-l10n anyway :)
Attachment #313203 - Flags: ui-review?(beltzner) → ui-review+
Comment on attachment 313203 [details] [diff] [review]
fix v5 - comments addressed

r=mano
Attachment #313203 - Flags: review?(mano) → review+
Assignee

Updated

11 years ago
Attachment #313203 - Flags: approval1.9?
Assignee

Updated

11 years ago
Whiteboard: [9 strings][has patch][needs review mano] → [9 strings][has patch][needs approval]
Comment on attachment 313203 [details] [diff] [review]
fix v5 - comments addressed

a1.9=beltzner
Attachment #313203 - Flags: approval1.9? → approval1.9+
Assignee

Updated

11 years ago
Whiteboard: [9 strings][has patch][needs approval] → [9 strings][has patch]
Assignee

Comment 27

11 years ago
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.322; previous revision: 1.321
done
Checking in browser/components/migration/content/migration.xul;
/cvsroot/mozilla/browser/components/migration/content/migration.xul,v  <--  migration.xul
new revision: 1.30; previous revision: 1.29
done
Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.152; previous revision: 1.151
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.126; previous revision: 1.125
done
Checking in browser/components/places/tests/unit/test_384370.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v  <--  test_384370.js
new revision: 1.16; previous revision: 1.15
done
Checking in browser/locales/en-US/chrome/browser/migration/migration.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/migration/migration.dtd,v  <--  migration.dtd
new revision: 1.8; previous revision: 1.7
done
Checking in browser/locales/en-US/chrome/browser/places/places.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v  <--  places.dtd
new revision: 1.43; previous revision: 1.42
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.39; previous revision: 1.38
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js,v  <--  test_424958-json-quoted-folders.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/locales/en-US/chrome/places/places.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v  <--  places.properties
new revision: 1.7; previous revision: 1.6
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Assignee

Updated

11 years ago
Blocks: 424654
Assignee

Comment 28

11 years ago
Posted patch as checked inSplinter Review
- s/a/an/
- s/restoreBookmarksFromFile/restoreBookmarksFromJSONFile/ in a test
Attachment #313203 - Attachment is obsolete: true
> -<!ENTITY cmd.restore.label              "Restore…">
> +<!ENTITY cmd.restore2.label             "Restore">
> <!ENTITY cmd.restore.accesskey          "R">

accesskey entity name should be changed too
Assignee

Comment 30

11 years ago
Attachment #313466 - Flags: review?(beltzner)
Attachment #313466 - Flags: approval1.9?
Attachment #313466 - Flags: review?(beltzner)
Attachment #313466 - Flags: review+
Attachment #313466 - Flags: approval1.9?
Attachment #313466 - Flags: approval1.9+

Comment 31

11 years ago
now
"Import" is changed to "Import HTML"
"Export" is changed to "Export HTML"

so these menus should be changed too ? 
"Backup" >> "Backup JSON"
"Restore" >> "Restore JSON"
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.127; previous revision: 1.126
done
Checking in browser/locales/en-US/chrome/browser/places/places.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v  <--  places.dtd
new revision: 1.44; previous revision: 1.43
done
Keywords: checkin-needed
Whiteboard: [9 strings][has patch] → [9 strings]
Flags: in-litmus?
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre)
Gecko/2008031806 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
Test case https://litmus.mozilla.org/show_test.cgi?id=6853 has been updated for regression testing this bug.
Flags: in-litmus? → in-litmus+
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.