Closed Bug 1120110 Opened 10 years ago Closed 7 years ago

Standardize default output folder for adding bookmarks by various methods

Categories

(Firefox :: Bookmarks & History, defect, P1)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- verified

People

(Reporter: Virtual, Assigned: mikedeboer)

References

Details

(6 keywords, Whiteboard: [reserve-photon-structure])

Attachments

(1 file, 2 obsolete files)

Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder.
(deleted), text/x-review-board-request
mak
: review+
Details
When user use various methods of adding a bookmarks, they will be added to different folders. We should standardize it to be more logical and consistent, so user shouldn't be confused where the new bookmarks go to, when he use various methods of adding a bookmarks. I'm recommending that all new bookmarks should be created under Unsorted Bookmarks, if user didn't select any other path. Bookmarks that go to: 1. Bookmarks Menu by: -pressing RMB (Right Mouse Button) on tab => Bookmark All Tabs... -pressing RMB (Right Mouse Button) => star icon (Bookmark This Page) -pressing Ctrl+D (keyboard shortcut) 2. Unsorted Bookmarks by: -pressing star icon in the toolbar Requesting feedback
Flags: needinfo?(mano)
Flags: needinfo?(mak77)
I could see Bookmark All Tabs having a different behavior than the other options, and Unsorted doesn't seem like the right place for the new tab folder from Bookmark All Tabs.
Also moving a tab on star icon adds page to Bookmarks Menu with "Connecting..." name. So I think a find another bug.
So, the fact is that the current behavior is "by design", that is the UX team and the Places team originally decided this was what we wanted. Most of the reasons were bound to retro-compatibility with FX2 and avoid breaking users. I agree today we are not in the best shape, but I'm not sure just touching this is the way to go. We'd just be moving the problem elsewhere (for example by making the menu the default, we'd make it overpopulated and introduce performance issues, while by making unsorted the default, we'd break user habits with the bookmarks menu) The fact is that the whole bookmarking interaction is pretty much confusing and should be redesigned as a whole. I hope soon that is what will happen and a new team will takeover responsibility to design a new bookmarking ui.
Flags: needinfo?(mano)
Flags: needinfo?(mak77)
Blocks: 1219810
No longer blocks: 1219810
I don't think this is well-defined enough to be part of the work we're currently focused on in bug 1219810.
No longer blocks: 1219810
Hmm, I don't know how to more well-definite it to be enough. This bug is about standardize the default output folder for adding bookmarks by various methods, because now it's not consistent. Every new bookmark should be created in the same default folder, e.g. "Other Bookmarks" or at least the one chosen one.
(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #5) > or at least the last chosen one.
(In reply to Virtual_ManPL [:Virtual] - (ni? me) from comment #5) > Hmm, I don't know how to more well-definite it to be enough. See comment 3. Basically this needs a real UX specification.
Keywords: uiwanted
Yes. I know, that's why I created this bug to make it all consistent everywhere. :)
Has Regression Range: --- → irrelevant
Has STR: --- → yes
Priority: P4 → P5
Any chances for this issue to be included to Photon UI refresh project (bug #1349210)?
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [photon?]
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #9) > Any chances for this issue to be included to Photon UI refresh project (bug > #1349210)? Depends how UX feel about this. As noted in comment #3, this needs a spec. Even then, it'd be More Work, and there is a large quantity of work as it is. However, I can see why the change to the bookmark button location and library button, and moving the bookmarks in the bookmarks menu outside of the main view of the popup that opens if you click the library, might make this the right time to make a change here, and (famous last words) it might not be technically *that* difficult once there's agreement about what to actually do, assuming that means the default location will be fixed (rather than 'last used', which is a recipe for loads of pain and complexity). Bryan/Aaron, have you considered this issue as part of Photon?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
We would be interested in unifying Ctrl+D and the Star button because the act of bookmarking via a click and or its corresponding key command should be the same. So Ctrl+D should be remapped to save bookmarks to the "Other Bookmarks" folder by default. "Bookmark All Tabs..." can remain mapped to the "Bookmark Menu" for now, as it's a convenience feature geared toward getting back to a group of pages in the short term. For better or worse the best place for that type of thing, for now, is to show them in the "Bookmarks Menu." However, if we were to touch any of that code, it would be best if we inserted a useful default name, based on the <date> + <number>, rather than "[Folder Name]," which has almost zero utility.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
(In reply to :Gijs from comment #10) > might make this the right time to make a change here & (In reply to bbell from comment #11) > We would be interested in unifying Ctrl+D and the Star button because the > act of bookmarking via a click and or its corresponding key command should > be the same. So Ctrl+D should be remapped to save bookmarks to the "Other > Bookmarks" folder by default. OK. Thank you very much! So per comment #10 and comment #11, I'm adding this bug as blocker to bug #1349210. (In reply to bbell from comment #11) > "Bookmark All Tabs..." can remain mapped to the "Bookmark Menu" for now, as > it's a convenience feature geared toward getting back to a group of pages in > the short term. For better or worse the best place for that type of thing, > for now, is to show them in the "Bookmarks Menu." I was also hoping for "Bookmark All Tabs..." to be remapped to "Other Bookmarks", to be consistent with other, but looking on bottom part of reply... (In reply to bbell from comment #11) > However, if we were to > touch any of that code, it would be best if we inserted a useful default > name, based on the <date> + <number>, rather than "[Folder Name]," which has > almost zero utility. ...it's a great idea, like saving all tabs as whole session for later management in short term, not bloating next startup etc.
Blocks: photon-structure
Whiteboard: [photon?] → [photon]
Blocks: 1352110
No longer blocks: photon-structure
Flags: qe-verify+
Priority: P5 → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Moving to reserve backlog for now.
Priority: P2 → P3
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P4
No longer blocks: photon-structure
Priority: P4 → P5
Priority: P5 → P3
Taking a stab at this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Priority: P3 → P1
Attachment #8915945 - Flags: review?(mak77) → feedback?(mak77)
Comment on attachment 8915944 [details] Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. https://reviewboard.mozilla.org/r/187212/#review192684 The patch looks good, but you should probably check tests to verify they don't rely on the previous default location ::: browser/base/content/browser-sets.inc:61 (Diff revision 1) > #ifdef XP_MACOSX > <command id="cmd_findSelection" oncommand="gFindBar.onFindSelectionCommand();"/> > #endif > <!-- work-around bug 392512 --> > <command id="Browser:AddBookmarkAs" > - oncommand="PlacesCommandHook.bookmarkCurrentPage(true, PlacesUtils.bookmarksMenuFolderId);"/> > + oncommand="PlacesCommandHook.bookmarkCurrentPage(true);"/> Looks like you removed the last use of the second argument to bookmarkCurrentPage, so it could go. And at that point, since we stopped caring about add-ons compat, you could completely remove bookmarkCurrentPage and just use bookmarkPage(gBrowser.selectedBrowser, true); where necessary. Indeed, you are also removing the last consumer of the second argument to bookmarkPage(), so we could remove that second argument and make it always default to the unfiled folder. This would make it easier in the future to switch the default location through a simple pref. Provided this is not expected to be uplifted to 57 I'd prefer this little wider refactoring.
Attachment #8915944 - Flags: review?(mak77)
Comment on attachment 8915945 [details] Bug 1120110 - For the 'Bookmark All Tabs' feature, find a potentially more useful default folder name than '[Folder Name]'. https://reviewboard.mozilla.org/r/187214/#review192688 This change deserves its own bug, it's undeed unrelated to the other change. Off-hand, I think it would be nicer if the dialog would NOT show the folder name, instead it should have a placeholder text like "Insert a name for the folder" and the Add Bookmark button should be disabled until a name is provided. Then it wouldn't really matter much which temporary name we use while waiting for the actual name, [Folder name] could be fine since it's temporary. Note that involving localization of Dates here is more complex than it looks and will open a can of worms, because there are previous discussions about whether we should respect OS localization VS User OS localization VS build localization. I also don't think the deduplication looks good, "October 5 - 2" sounds more like a trivia question than a name. At a maximum it could be "October 5 (2)".
Attachment #8915945 - Flags: feedback?(mak77) → feedback-
Attachment #8915944 - Attachment is obsolete: true
Attachment #8915945 - Attachment is obsolete: true
Comment on attachment 8920199 [details] Bug 1120110 - Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. https://reviewboard.mozilla.org/r/191226/#review196700 There are a few callpoints to fix yet in browser/base/content/test/general/browser_bookmark_titles.js
Attachment #8920199 - Flags: review?(mak77) → review+
Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7f812d58e5fe Consistently save pages bookmarked using 'Bookmark This Page' anywhere into the 'Other Bookmarks' folder. r=mak
https://hg.mozilla.org/mozilla-central/rev/7f812d58e5fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 58.0a1 (2017-10-24), so I'm marking this bug as VERIFIED, except > -pressing RMB (Right Mouse Button) on tab => Bookmark All Tabs... as it intended per Comment 11 and its follow up in Comment 19. Thank you very much! \o/
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Is it meant to be that this bug didn't affect the "Bookmark This Link" option present when right clicking a link on a page? This option still defaults to using the "Bookmarks Menu" folder.
(In reply to xofe from comment #26) > Is it meant to be that this bug didn't affect the "Bookmark This Link" > option present when right clicking a link on a page? This option still > defaults to using the "Bookmarks Menu" folder. Marco? :-)
Flags: needinfo?(mak77)
The scope of this bug was to unify mouse and keyboard behavior for the "Bookmark this page" option because they were incosistent. "Bookmark this link" is a different feature and it doesn't have a direct keyboard shortcut, as such it needs a separate bug and UX evaluation, if you file such a bug I'll take care of pinging the appropriate persons to evaluate your request. Thanks.
Flags: needinfo?(mak77)
Blocks: 1436745
No longer blocks: 1436745
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: