The default bug view has changed. See this FAQ.

links in "view source" should have "copy link location" in context menu

RESOLVED FIXED in mozilla13

Status

()

Toolkit
View Source
--
enhancement
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Ori Avtalion (salty-horse), Assigned: darktrojan)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Right clicking on links should have a "Copy link location" item in the
context menu, just like the "regular" browser window.

Since this feature is missing, copying links is now very hard. I have to be careful to click and drag from one edge of the link to the other, while being careful not to trigger the link by pressing it directly.
(Reporter)

Comment 1

8 years ago
This might relate to bug 304081

Updated

8 years ago
Blocks: 467530

Updated

8 years ago
Hardware: x86 → All
I've been realizing the same thing.  I've been right clicking on URL's and expecting to see the copy link location functionality.

cc'ing cbartley, who tackled bug 17612.  Doesn't seem like many others have given view-source much love in the past several years.
Duplicate of this bug: 522321
Created attachment 406604 [details] [diff] [review]
patch, part1

This is what I have, it seems to work well. Perhaps there should also be a "Save Link As.."?
I tried so hard to get one patch, but for some reason, that is impossible with my tree, so that's why I'm posting it as two parts.
Created attachment 406605 [details] [diff] [review]
patch, part2 nsContextMenu.js file

Updated

8 years ago
Attachment #406604 - Attachment is patch: true
Attachment #406604 - Attachment mime type: application/octet-stream → text/plain
We probably don't want to have the view-source: protocol added, so the goDoCommand('cmd_copyLink'); thing might have to be changed in a copyLink function that removes the view-source: protocol or something.
(In reply to comment #5)
> Created an attachment (id=406605) [details]
> patch, part2 nsContextMenu.js file

Martijn: Did you write the code in nsContextMenu.prototype.setTarget, or is that just copied from elsewhere?  Either way, that function would really benefit from being factored out into multiple functions.  It's not that the function is all that long, it just feels like the the nesting is excessively deep:

  while {
    if {
      if {
        while {
          try {
            if {
Yes, I copied it from another nsContextMenu.js. Indeed, there is a lot of stuff that could probably be made simpler in there, still.
(In reply to comment #8)
> Yes, I copied it from another nsContextMenu.js. Indeed, there is a lot of stuff
> that could probably be made simpler in there, still.

It kind of had that look :-).

I think it's probably reasonable to dispense with the separate nsContextMenu.js file and just incorporate that code directly into viewSource.js.  If you want to keep the separate file, you should probably rename it to viewSourceContextMenu.js.

I'll try this patch out later today, and get back to you with some more substantive feedback.
Duplicate of this bug: 560789

Updated

7 years ago
Duplicate of this bug: 561612

Comment 12

6 years ago
mark bug 645780 as duplicate?

Comment 13

6 years ago
So any news about this? The patch was released two years ago, and still no implementation.

Updated

6 years ago
Duplicate of this bug: 645780

Updated

6 years ago
Depends on: 680651

Updated

6 years ago
Blocks: 680651
No longer depends on: 680651

Updated

6 years ago
Duplicate of this bug: 687799
Duplicate of this bug: 694711
(Assignee)

Updated

6 years ago
Assignee: nobody → geoff
(Assignee)

Comment 17

6 years ago
Created attachment 572775 [details] [diff] [review]
new patch
Attachment #406604 - Attachment is obsolete: true
Attachment #406605 - Attachment is obsolete: true
Attachment #572775 - Flags: review?(bugs)
Comment on attachment 572775 [details] [diff] [review]
new patch

Some toolkit peer should review this. dao or gavin perhaps
Attachment #572775 - Flags: review?(bugs) → review?(dao)
(Assignee)

Comment 19

6 years ago
Created attachment 573155 [details] [diff] [review]
patch, v2

My original test fails most of the time on try. This new version seems to pass on all the platforms (some haven't finished yet): https://tbpl.mozilla.org/?tree=Try&rev=69a908e2cd7a
Attachment #572775 - Attachment is obsolete: true
Attachment #573155 - Flags: review?(dao)
Attachment #572775 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Depends on: 702448
(Assignee)

Comment 20

5 years ago
Created attachment 578727 [details] [diff] [review]
patch, v3

This is the same patch as the previous one, updated to go with the new view source tests.
Attachment #573155 - Attachment is obsolete: true
Attachment #578727 - Flags: review?(dao)
Attachment #573155 - Flags: review?(dao)
Comment on attachment 578727 [details] [diff] [review]
patch, v3

You need to update viewPartialSource.xul as well.

Also replace the deprecated document.popupNode with popup.triggerNode.

nit: let those function names start with a small letter
Attachment #578727 - Flags: review?(dao) → review-
(Assignee)

Comment 22

5 years ago
Created attachment 582949 [details] [diff] [review]
patch v4

I ran into bug 669845 while testing the partial source window, so I've disabled that test on debug.
Attachment #578727 - Attachment is obsolete: true
Attachment #582949 - Flags: review?(dao)
(Assignee)

Comment 23

5 years ago
Dão, can I get an ETA on this review? Thanks.
(Assignee)

Updated

5 years ago
Depends on: 728655
(Assignee)

Comment 24

5 years ago
Created attachment 598636 [details] [diff] [review]
patch v5

I've rewritten the tests for partial view source using the functions created in bug 728655.
Attachment #582949 - Attachment is obsolete: true
Attachment #598636 - Flags: review?(dao)
Attachment #582949 - Flags: review?(dao)
Comment on attachment 598636 [details] [diff] [review]
patch v5

>+      if (partialTestRunning)
>+        return finish();

      if (partialTestRunning) {
        finish();
        return;
      }
Attachment #598636 - Flags: review?(dao) → review+
(Assignee)

Comment 26

5 years ago
On inbound with that nit fixed and also bug 728655 comment 20, which slipped through the cracks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d8fc342348b
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/467940965809 - so far, OS X 10.5 and 10.6 debug have timed out, "browser_contextmenu.js | application timed out after 330 seconds with no output," which might wind up meaning it's broken on OS X, or very well might mean it's broken on the slow things, and will eventually time out on WinXP too.
Target Milestone: mozilla13 → ---
Win7 was a bit more interesting in https://tbpl.mozilla.org/php/getParsedLog.php?id=9552718&tree=Mozilla-Inbound&full=1 detecting a deadlock in nsWindowMediator.mListLock and taking down the browser over it.
(Assignee)

Comment 29

5 years ago
Took out the offending waitForFocus call, checked on Try server, and relanded.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c99485521cd
https://hg.mozilla.org/mozilla-central/rev/5c99485521cd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Updated

5 years ago
No longer blocks: 467530
Comment on attachment 598636 [details] [diff] [review]
patch v5

>+    if (gContextMenu.triggerNode.href.indexOf('view-source:') == 0)
For a known string, /^view-source:/.test is probably our best version of startsWith. (For an unknown string, .lastIndexOf(needle, 0) == 0 works best.)
You need to log in before you can comment on or make changes to this bug.