78021 - Copy Link Location broken for image maps and LINK elements

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   1. Go to http://www.planetarion.com/
   2. Right click on "Log In".
   3. Choose "Copy Link Location".
   4. Paste the clipboard into the location bar.

ACTUAL RESULTS
   Old contents of clipboard will be pasted.

EXPECTED RESULTS
   The Copy Link Location option should copy the link location to the clipboard.

I need this for day to day use of the browser.

Comment 1

[Dan Rosen]

There are a bunch of minor correctness issues with the code in PresShell that
figures out whether you're in an image or link, or not. This is probably the
most major of those. This will probably morph into the inevitable task of
splitting the helpers out of PresShell into a separate interface and making them
more correct.

Comment 2

[Dan Rosen]

->future, per trudelle

Comment 3

[C. Scott Ananian]

Copy link location is also broken for ftp listings.  To reproduce:

1. Go to ftp://ftp.linux-wlan.org/pub/linux-wlan-ng/
2. Right click on (say) linux-wlan-ng-0.1.7.tar.gz
3. Choose "Copy Link Location".
4. Paste the contents somewhere.

The old contents of the clipboard will be pasted, instead of the URL for
the link you selected.

Based on the comments from Dan Rosen, which indicates it is a general problem, I
added this to the comments of this bug instead of creating a new bug report for
it.  Summary should be changed to add "and ftp" to the end, but I don't have
sufficient permissions to do that.

I'm seeing this on the linux build;  platform's already been set to "All" so I
suppose the same sorts of things happen on other platforms.

Comment 4

[John Morrison]

Context menu 'copy link location' with an ftp listing worksforme on win2k
and linux (rh6.1). I don't know what specifically dr was referring to above,
but if it were the case that this did not work for ftp listings, it should 
still go in as a separate bug (if for no other reasons than I personally 
could punt on fixing this bug, but not on something as bread and butter as 
copying a simple link in HTML content).

Comment 5

[Dan Rosen]

cananian, jrgm: the ftp problem is bug 77171. cananian, we don't have a
xul-based ftp view anymore, we use html to view ftp directories. so i won't fix
that bug until we start using xul to view ftp directories again. it's related,
though...

Comment 6

[Dan Rosen]

<spam>

Netscape 6.0 went out with Copy Link Location working with imagemaps. The next
Netscape RTM should not go out with it broken. Bug 80116 and bug 78021 should be
fixed for 0.9.2.

This should be a simple task of filling in a new XPCOM API with Bill Law's
content-sniffing code in nsContextMenu.js. (More content analyzing APIs should
be filled in post-RTM as needed).

</spam>

Comment 7

[Peter Trudelle]

->0.9.3, could possibly still take a fix during 'limbo'

Comment 8

[vectro]

dr, are you actually committing to fix this by 0.9.3? If not we need a new
target milestone.

Also keywords should be fixed.

Comment 9

[Dan Rosen]

[spam] -> 0.9.5

Comment 10

[Jeremy M. Dolan]

Another page to verify when this is fixed:

http://cartoonnetwork.com/toonami/signal/dragonballz/garlic_jr.html

Back to show and Episode Guide at the bottom... one is javascript. This is
pretty annoying since I have window.open disabled, so I have to dig through view
source for the URL to get at the content.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I have a fix, taking, trying to get onto the 0.9.4 branch as the fix is safe.
This can be fixed even without 80116, although fixing that can make the code
cleaner.

This seems to be fallout from bug 64313. dr only made the new copy things work
for A elements, regressing AREA, XLinks and maybe something else as well... are
there other "link" elements in HTML besides A and AREA? I am at a loss as to
explain why the reviewers okayed the change even though it regressed everything
else beyond A.

Comment 12

[Hixie (not reading bugmail)]

the <link> element

Comment 13

[Olivier Cahagne]

*** Bug 98322 has been marked as a duplicate of this bug. ***

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

How can the user right-click on a LINK element (that is hidden inside HEAD), and
do a Copy Link Address from the context menu? I don't think we have UI for
prev/next links, and even if we did I don't see how you could right-click on a
UI button or something and expect to copy the link location...

Comment 15

[Hixie (not reading bugmail)]

He can't choose "Copy Link Location", cos it doesn't appear. It should, though.
As to how he can right click on a <link> element... See:
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/link2.html

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Wow, I didn't know that! This is cool.

Ok, I now have a patch that fixes the context menu for AREA (image maps), LINK
and simple XLinks as well (Copy Link Address item and other context menu items
dependent that are link specific). 

A was working already. There is one bug with the LINKs: if you right-click on
the label, it does not work. Clicking on the image or anywhere else does work.
DOM Inspector thinks the element is <browser> which completely whacks up the
whole context menu - it thinks this is a frameset document, even. I don't really
know how to go after that, but I'll file a new bug on context menu experts.

While testing, I noticed the LINK links do not actually work; if you click on
them nothing happens. Sometimes they depress as buttons when you click, but
usually they don't. Just opening that page, and mousing over and clicking the
buttons produces a bunch of assertions:

###!!! ASSERTION: aOuter is not an nsILink: 'link != nsnull', file c:\builds\moz
illa\content\html\content\src\nsGenericHTMLElement.cpp, line 1289
###!!! Break: at file c:\builds\mozilla\content\html\content\src\nsGenericHTMLEl
ement.cpp, line 1289

Is that a regression, and is there are bug already filed?

Also, mousing over the LINK buttons does not show anything in the statusbar. Is
that a regression, any bugs open?

Btw, IE6 displays the page funny: it adds a new non-functional scrollbar to the
right, and does not display the LINK links.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 1

Comment 18

[manojd]

Heikki Toivonen 2001-09-06 11:51 - "There is one bug with the LINKs: if you
right-click on the label, it does not work.": I had recently filed this one
(#98322) - which someone marked as duplicate of this one. Works correctly in
Netscape Communicator 4.7.

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

manojd, and others: I may have caused some confusion with terminology, so let me
try to clear it up. Currently we get the correct context menu items for <a> and
<area> (imagemap link) elements. Copy Link Address does not work for the <area>
element, though.

After my fix, Copy Link Address works for <area> (imagemap link) as well.
Additionally, after the fix, the context menu will have the correct entries and
works correctly for <link> elements (this is the weird element in HTML <head>
that most people don't know can be used for linking purposes). Look at the page
Hixie provided; I bet you have never seen anything like that before. There is
still a bug with <link> elements, though: right-clicking on the button label
(like 'index') will not give you link-specific context menu items and therefore
you cannot even try to do Copy Link Address. Also the <link> links do not seem
to work, but that is a different bug.

Comment 20

[harishd]

Comment on attachment 48445 [details] [diff] [review]
Proposed fix 1

r=harishd

Comment 21

[vidur (gone)]

Comment on attachment 48445 [details] [diff] [review]
Proposed fix 1

sr=vidur

A couple of nits:
1) In DocumentViewerImpl::GetPopupLinkNode, you could do atom comparisons on the local name instead of QIs.
2) In the PresShell patch, you can use the inline function NS_NewURI instead of invoking the service directly.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

1. I decided not to; the benefit is not that much in speed.
2. I also decided against this; NS_NewURI expands into the same code.

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This affects top100 sites, including http://home.netscape.com "Autos", "Browser
Central" etc. are actually made with image maps and Copy Link Address does
currently not work for them.

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on the trunk, leaving open for checkin on the 0.9.4 branch.

Comment 25

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Seems like 0.9.4 went/is going... moving on. This should still get on the branch
after mozilla.org is done with it.

Comment 26

[Jaime Rodriguez, Jr.]

Please check it in by 3 p.m. today. PDT+

Comment 27

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on 0.9.4 branch as well, although I couldn't land it before 3 pm. That
would have required me to go back in time, seeing that the PDT+ was given after
4 pm ;)

Comment 28

[John Morrison]

verified fixed for branch 10/1 builds on linux/mac/win32.