Allow to copy link location

RESOLVED FIXED in Firefox 67

Status

enhancement
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: nchevobbe, Assigned: fanny, Mentored)

Tracking

(Depends on 1 bug, Blocks 1 bug, {good-first-bug})

Trunk
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox61 affected, firefox67 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Reported in https://github.com/devtools-html/devtools-core/issues/761

When I right-click a link in a web page, there is the "Copy Link Location" option.

But if I enter "http://example.com" into the console, I get "Copy message" and "Copy object".

"Copy message" seems to copy the string with quotes and with a trailing newline. "Copy object" seems to copy the string with quotes.

There should be some way to just copy the link location, without quotes.

---

We should also provide a way to copy the message location and stacktrace location from the context menu entry
(Reporter)

Updated

a year ago
Priority: -- → P3

Updated

11 months ago
Product: Firefox → DevTools
(Reporter)

Updated

3 months ago
Mentor: nchevobbe
Depends on: 1214556
Keywords: good-first-bug
(Reporter)

Updated

3 months ago
Duplicate of this bug: 1261885

Hi, i'm a gsoc aspirant and i want to try fix this bug, do you can assign to me?

(Reporter)

Comment 3

3 months ago

Hello Fanny, thanks for offering help :)

I assigned the bug to you, let me know if you have any questions, here or on slack.

Assignee: nobody → fannyvieira082
Status: NEW → ASSIGNED

IMO, before fixing this, fake links should actually be made real links, see e.g. bug 1447820 comment 9.

I thought in make a regex for check if exists http and https in url, but is not enough?

Sorry, i read the bug and if i understand well, are the following links also valid: resource:, chrome:

thinking better, my first suggestion not is good because the following links also not pass,

www.google.com
www.bugzilla.org

(Reporter)

Comment 8

3 months ago

Fanny, sorry I should have check the issue before.
From the console, I think we don't want to do any regex check. We only want to check that target element is a link (<a>) and grab its URL.

I'm going to do a quick write-up shortly.

I comprehend, i am a bit lost on the base code, where sould i make these modifications?

i'm confusing, in the console instead of string, i would pass a tag <a>?

(Reporter)

Comment 11

3 months ago

Okay, so first, you might be interested looking into the browser toolbox. This is basically devtools for Firefox itself, so you can inspect things in the console (and other places).

So now, let's focus on the message location (the link on the right of a message).
So if you go on https://nchevobbe.github.io/demo/console-test-app.html , and click on the console.log button, and then open the console, you should see a log message from console-test-script.js:47:24.

With the browser toolbox open, we can inspect the location, which show us:

<span class="message-location devtools-monospace">
  <span data-url="https://nchevobbe.github.io/demo/console-test-script.js" class="frame-link" data-line="47" data-column="24">
     <a href="https://nchevobbe.github.io/demo/console-test-script.js" class="frame-link-source" draggable="false">
       <span class="frame-link-source-inner" title="View source in Debugger → https://nchevobbe.github.io/demo/console-test-script.js:47:24">
         <span class="frame-link-filename">console-test-script.js</span>
         <span class="frame-link-line">:47:24</span>
       </span>
     </a>
  </span>
</span>

So we can spot an <a> which holds the actual URL (href="https://nchevobbe.github.io/demo/console-test-script.js").

Now, this is where we set the context menu: devtools/client/webconsole/webconsole-wrapper.js#176-224.
In there, you can see that we have a target variable.

What we may do, is check if the target has an <a> with an href attribute on it.

That could be done using const linkElement = target.closest(a[href]) for example, which will give us the closest parent (or the element itself), which match the selector.
If the result is not null, we can then get the url of the element const url = linkElement && linkElement.getAttribute("href").

Now we can pass this url variable to the function we use to create the context menu:

const menu = createContextMenu(this.webConsoleUI, this.parentNode, {
  actor,
  clipboardText,
  variableText,
  message,
  serviceContainer,
  openSidebar,
  rootActorId,
  executionPoint,
  toolbox: this.toolbox,  
});

Now we have all the pieces in place.
In devtools/client/webconsole/utils/context-menu.js#41-51 we can retrieve the url function, and then in the function body, check if the url is not null, and if not, copy it to the clipboard

if (url) {
  menu.append(new MenuItem({
    id: "console-menu-copy-url",
    // The l10n entry needs to be declared. We can copy what's done in https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/devtools/client/locales/en-US/webconsole.properties#147-151.
    label: l10n.getStr("webconsole.menu.copyURL.label"),
    // same
    accesskey: l10n.getStr("webconsole.menu.copyURL.accesskey"),
    click: () => clipboardHelper.copyString(url)
  }));
}

I hope it's clear enough :)
Once you have something working, ping me again here, and we'll discuss about the test we will need to write for this feature!

(In reply to Nicolas Chevobbe from comment #11)

So we can spot an <a> which holds the actual URL (href="https://nchevobbe.github.io/demo/console-test-script.js").

Note this is not the case for STR in comment 0, then it's just a fake link without href attribute:

<a class="url" title="http://example.com" draggable="false">http://example.com</a>

Fixing this bug won't be much useful until reps uses real links, that's what I meant in comment 4.

If the result is not null, we can then get the url of the element const url = linkElement && linkElement.getAttribute("href").

I recommend linkElement.href instead of linkElement.getAttribute("href") to guarantee getting a properly serialized absolute URL.

(In reply to Nicolas Chevobbe from comment #11)

Okay, so first, you might be interested looking into the browser toolbox. This is basically devtools for Firefox itself, so you can inspect things in the console (and other places).

So now, let's focus on the message location (the link on the right of a message).
So if you go on https://nchevobbe.github.io/demo/console-test-app.html , and click on the console.log button, and then open the console, you should see a log message from console-test-script.js:47:24.

With the browser toolbox open, we can inspect the location, which show us:

<span class="message-location devtools-monospace">
  <span data-url="https://nchevobbe.github.io/demo/console-test-script.js" class="frame-link" data-line="47" data-column="24">
     <a href="https://nchevobbe.github.io/demo/console-test-script.js" class="frame-link-source" draggable="false">
       <span class="frame-link-source-inner" title="View source in Debugger → https://nchevobbe.github.io/demo/console-test-script.js:47:24">
         <span class="frame-link-filename">console-test-script.js</span>
         <span class="frame-link-line">:47:24</span>
       </span>
     </a>
  </span>
</span>

So we can spot an <a> which holds the actual URL (href="https://nchevobbe.github.io/demo/console-test-script.js").

Now, this is where we set the context menu: devtools/client/webconsole/webconsole-wrapper.js#176-224.
In there, you can see that we have a target variable.

What we may do, is check if the target has an <a> with an href attribute on it.

That could be done using const linkElement = target.closest(a[href]) for example, which will give us the closest parent (or the element itself), which match the selector.
If the result is not null, we can then get the url of the element const url = linkElement && linkElement.getAttribute("href").

Now we can pass this url variable to the function we use to create the context menu:

const menu = createContextMenu(this.webConsoleUI, this.parentNode, {
  actor,
  clipboardText,
  variableText,
  message,
  serviceContainer,
  openSidebar,
  rootActorId,
  executionPoint,
  toolbox: this.toolbox,  
});

Now we have all the pieces in place.
In devtools/client/webconsole/utils/context-menu.js#41-51 we can retrieve the url function, and then in the function body, check if the url is not null, and if not, copy it to the clipboard

if (url) {
  menu.append(new MenuItem({
    id: "console-menu-copy-url",
    // The l10n entry needs to be declared. We can copy what's done in https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/devtools/client/locales/en-US/webconsole.properties#147-151.
    label: l10n.getStr("webconsole.menu.copyURL.label"),
    // same
    accesskey: l10n.getStr("webconsole.menu.copyURL.accesskey"),
    click: () => clipboardHelper.copyString(url)
  }));
}

I hope it's clear enough :)
Once you have something working, ping me again here, and we'll discuss about the test we will need to write for this feature!

Yes, it was very useful, from what I tested here, I was able to implement, see this image

Before this change, when we tried to select a URL to copy,
the label "Copy Message" or "Copy Object" was displayed which
does not represent the context. You can now detect if a link exists
and the label is "Copy link address"

(Reporter)

Comment 15

3 months ago

(In reply to Oriol Brufau [:Oriol] from comment #12)

Fixing this bug won't be much useful until reps uses real links, that's what I meant in comment 4.

We're trying to build a future-proof solution so it will work for links in reps as well :)

I recommend linkElement.href instead of linkElement.getAttribute("href") to guarantee getting a properly serialized absolute URL.

Thanks for the tip Oriol!

Comment 16

3 months ago
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4565c0c6aea8
Implement copy link adress option. r=nchevobbe

Comment 17

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
(Reporter)

Updated

3 months ago
Duplicate of this bug: 1377429
(Reporter)

Updated

a month ago
Duplicate of this bug: 952806
You need to log in before you can comment on or make changes to this bug.