68985 - "Bookmark this link" in context menu doesn't show bookmark-creation dialog

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

DESCRIPTION:  When I create a bookmark using "Bookmark this link" in the
right-click context menu of an A element, I don't get a bookmark-creation
dialog.  Instead, the bookmark is silently added to the bookmarks menu using
the textual contents of the A element as the title of the bookmark.  Frankly,
this is where I think the bookmark-creation dialog is most useful, since the
textual content of the A element is less likely to be an appropriate bookmark
title than the TITLE element of the page.

STEPS TO REPRODUCE:
 * right-click a link, such as one of the ones on this page
 * click "Bookmark this link"

ACTUAL RESULTS:
 * page is silently bookmarked using the textual contents of the A element as
   the bookmark title

EXPECTED RESULTS:
 * bookmark-creation dialog shows up, with the textual contents of the A
   element as the default title

DOES NOT WORK CORRECTLY ON:
 * Linux, mozilla, build on 2001-02-14 a bit after tree reopened

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

wasn't aware of this callsite. .9

Comment 2

[Paul Chen]

Marking nsbeta1+

Comment 3

[Paul Chen]

Mass moving most of mozilla0.9 bugs to mozilla0.8.1

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Patch to fix this bug.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

note that this patch also removes the .8 hack to prevent this dialog from 
showing on mac. With the removal of the create in disclosure triangle (part of 
another patch), I think the mac crasher has been fixed. 

Comment 6

[Peter "jag" Annema]

r=jag

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Fix checked in. 

Comment 8

[Claudius Gayle]

I'm reopening this bug mostly because it doesn't work but also because I think it may be a
candidate for 'wontfix'. I think this bug was filed when bookmarking was in a transition state,
when were still wrestling w/adding a bookmark in place and adding it to a dynamic destination.
We now have both menuitems available and the current behavior from the context menu is to
add the bookmark in place with no further input.
I'm fine with that and it's 4.x compatible. However, it might be more robust to simply add the
second option into the context menu.

Ben, you can wontfix or morph this bug as you please. If you morph it we should reset the milestone
and keywords.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I would recommend wontfix -- we already have far too many context menu options.

CCing mpt for UI wisdom and Blake, who's rewriting the context menus anyway.

Comment 10

[Jesse Ruderman]

I recommend just fixing this bug again (make the "bookmark this link" context 
menu item bring up a dialog).  Links often have titles that are only meaningful 
in the context of the linking page, so it's important that users get a chance 
to choose a title when bookmarking a link.

(I think "bookmark this page" should put up a dialog, too, but mostly to 
encourage users to organize their bookmarks before they have too many.)

Comment 11

[Claudius Gayle]

jesse, remember that we've reintroduced the dialogless method of adding bookmarks so that people
can now do both, cuz sometimes you wanna file your bookmarks and sometimes you want to
click and go.
I'm satisifed with a wontfix.
I'm warming up to the idea of including both(although, yes the menu is currently insanely long)
I'm vehemently opposed to ONLY including the dialog method.
I'm interested to hear mpt's opinion, but I'm more concerned that this is currently an open nsbeta1+
and moz8.1 bug that sticks out like a sore thumb. I just want to see it re-bucketed.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

As I've said before, "Bookmark this link" is the place where you would want the
dialog *most*, since link titles are rarely appropriate for bookmark titles that
will make sense out of the context of the web page..

Comment 13

[Matthew T (active 1999-2002)]

I agree with Jesse and David. The user would benefit in a majority of cases 
from spending the time to manually provide a useful title for such bookmarks to 
link, whereas they wouldn't necessarily benefit in a majority of cases when 
adding the currently-viewed page. So the dialog should be shown in this case.

In addition to that, context menu items are considerably more likely than other 
items to be selected by mistake; so where an item makes a permanent change to 
the system, it needs to be relatively easy to reverse that change. For this 
purpose, clicking `Cancel' in the dialog is much easier than realizing that a 
bookmark has been added, opening the bookmarks manager, finding and selecting 
the added bookmark, and deleting it.

Comment 14

[Jon Granrose]

SPAM: mozilla 0.9 (and M1, and 0.8.1, etc.) has left the building.  please
update the target milestone so we can get a good idea of what's left for 0.9.1.

Comment 15

[Fabian Guisset]

Kick whoever you want, we know bring up the dialog all the time *except* when
you right-click on a link... how nice

Comment 16

[Claudius Gayle]

Jesse, David, and mpt, I think you are all correct in the reasoning on why the dialog method
is more robust and useful.
I just contend that it's obtrusive and annoying. In this situation you're talking about someone
who is absorbed enough in the page they're already looking at that they don't even want to surf
to the link, now you're going to force them to 'tidy-up'? I'll say no more on this subject, I just wanted
people to consider the viewpoint of those who use the 'Add Bookmark' feature over 'Add Bookmark As'.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

/me wonders how this keeps getting broken

Patch shortly. 

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix (again)

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Looks like I had the parameter list wrong in the calling code in the context
menu - wasn't passing a value for the charset. 

I just pass 'undefined' since if the parameter is undefined, the addBookmark
function just assumes it's the charset of the focused window.

Comment 20

[Fabian Guisset]

r=fabian

Comment 21

[Blake Ross]

sr=blake

Comment 22

[Paul Chen]

nav triage team:

Since it looks like we've got a patch and sr + r, marking nsBranch

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

Fix checked in on trunk.

Comment 24

[lchiang]

adding vtrunk keyword to indicate the bug has been fixed on the trunk.  Bug left
open for tracking checkin to branch (nsbranch) when appropriate.

Once bug has been fixed on the branch also, pls remove vtrunk keyword.

Comment 25

[Viswanath Ramachandran]

someone needs to verify this on the trunk and make sure its safe before we try
to take this on the branch. 

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

Verified on trunk (2001070604 - commercial)

Comment 27

[Claudius Gayle]

I've been doing some testing with this. Technically I'm forced to agree
that this is fixed on the trunk. That is to say, I can see this feature
working and it doesn't look like the fix broke anything - but bookmarking
from context menus is being broken by some other recent branch+trunk checkin.

I'll write a new bug but the short version is there is some large subset
of links where adding a bookmark via the context menu does not work. It
does not fire the dialog and the bookmark is not added, and it's a very
recent regression on the branch.

Comment 28

[Claudius Gayle]

wrote bug 89710 for the issue where bookmark this link doesn't fire
for some set of links.

Comment 29

[Viswanath Ramachandran]

I'm dropping this from the nsBranch list. Since its fixed on the trunk, marking 
it fixed. 

Comment 30

[Bradley Baetz (:bbaetz)]

[that was to kick the keywords cache, btw - see bug 69621. 'vtrunk' no longer
exists]