1214556 - Add "open in new tab" context menu entry for links

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P5)

Description

[sworddragon2]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151003161951

Steps to reproduce:

1. Start Firefox.
2. Go to the menu bar -> Tools -> "Web Developer" -> "Web Console".
3. For example execute "console.log('http://www.google.com');", completely select the first link and make a right click on it.


Actual results:

The context menu is different from a normal site and missing useful entries like to open the link in a new tab/window.


Expected results:

The context menus should be more consistent with shared entries and only extended with specific entries.

Comment 1

[Lin Clark [:linclark]]

Thanks for the suggestion. I've added this to the backlog, but we'd happily accept a patch for this if someone wants to get to it sooner.

Comment 2

[Julien Wajsberg [:julienw] (PTO -> may 13)]

We'll happily accept a patch for this.

Comment 3

[Helena Moreno (aka helenatxu)]

Hello! I'm looking for good-first-bugs to start contributing as I'm applying for the Outreachy internship. Would it be a good one to start with? In that case, I would like to try it.

Comment 4

[PhoenixAbhishek]

Hi, I am Phoenix, a current Outreachy applicant.
Is this a good bug to start with for contributions..???
I would be happy to work on this :)

Comment 5

[Rainier Go [:rfgo]]

Hi,

I'm trying to handle this bug and I have a question. I've added the "Open URL in New Tab" on the context menu. Here's a screenshot: https://paste.pics/ccd11aa2676cf3925341c6d083b1d974

Currently, I'm using a regex pattern to detect if the text selected is a url and it works but I was wondering if there's better solution. Is there some sort of attribute that I can use to enable the option only when the variableText is a url?

For example, when I console.log("http://www.google.com") the console recognizes that it's a url and I can click on it but when I console.log("www.google.com") it's not clickable. How does the console recognize that it's a link and make it clickable?

Thank you!

Comment 6

[Rainier Go [:rfgo]]

Oh I'm sorry! I didn't see the other people before me. Should I still submit the patch?

Comment 7

[Helena Moreno (aka helenatxu)]

Well, I was still waiting for an answer from Nicolas, Rainier.

Comment 8

[Rainier Go [:rfgo]]

Sounds good!

Comment 9

[Getty Orawo]

Heey. would love to take this up as a first bug. I hope its okay.

Comment 10

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Thanks a lot everyone, it's nice to see your enthusiasm :)

Helenatxu, do you want to work on this bug rather than on the one I pinged you on? You have the priority here since you commented first :)

Rainier, make sure to read the whole thread before commenting and start to work on things :)

Getty, you're coming last here :), and as discussed over email, you may work on another bug.

Comment 11

[Rainier Go [:rfgo]]

Got it!! Thanks Nicolas!

Comment 12

[Helena Moreno (aka helenatxu)]

(In reply to Nicolas Chevobbe from comment #10)

Thanks a lot everyone, it's nice to see your enthusiasm :)

Helenatxu, do you want to work on this bug rather than on the one I pinged you on? You have the priority here since you commented first :)

No problem, I would say let's leave Rainier to upload the code as she/he already worked on it. But I agree, make sure to read the whole thread before commenting and start to work on things next time.

Comment 13

[Rainier Go [:rfgo]]

Hey Helen! Thanks, but I'm not an Outreachy applicant so if you need the bug for the program I'd be more than happy to just let you handle the bug.

Comment 14

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Looks like Helena has another bug assigned, so I'm assigning it to Rainier.

Rainer, you might want to read Bug 1457111#c11 where I explained how we can get the url and add a context menu entry.

Feel free to ask any question, either here or on our Slack.

Comment 15

[Lily]

Hey there, I'm an Outreachy applicant! Can I take on this bug? Would you recommend other first bugs?

Comment 16

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

(In reply to Lily from comment #15)

Hey there, I'm an Outreachy applicant! Can I take on this bug? Would you recommend other first bugs?

Hey Lily, sorry but this bug is already assigned (even if I did not set the appropriate flags).

I'll try to find new good first bug today :)

Comment 17

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Rainier, how is it going on? Are you blocked on anything?

Comment 18

[Rainier Go [:rfgo]]

Hey Nicolas! Sorry for going MIA I've been a bit busy. I'll have something up either today or tomorrow

Comment 19

[Rainier Go [:rfgo]]

Hey Nicolas, so I was looking at Bug 1457111#c11 and you mentioned how to get the link you would need to get the href attribute of an <a> tag, but because we're console.logging a string and not a DOM element my mind goes to regex. Is there another way of going about it?

Comment 20

[Rainier Go [:rfgo]]

Hm in my mind the console would detect that it's a link and then turns it into a clickable text similar to console-test-script.js:47:24 in the example you gave for Bug 1457111.

Comment 21

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Rainier,

if you look at that devtools/client/webconsole/utils/context-menu.js#51 you can see that now we have access to an url that might be clicked on. So you should only need to check that the url param is not falsy, and create the MenuItem (like in devtools/client/webconsole/utils/context-menu.js#211-218 , you might want to create the item in the same if block).
Hope it helps :)

Comment 22

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Rainier,

IS there anything blocking you on this bug? (if you don't have the time to work on it, that's perfectly fine :) )

Comment 23

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

clearing assignee

Comment 24

[Olena Velychko]

Hello Nicolas

I would like to work on this bug.

Olena

Comment 25

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Olena, thanks for offering help :) The bug is now assigned to you.
You can take a look at http://docs.firefox-dev.tools/getting-started/ to setup the work environment.
Then, you can follow what I was saying on Comment 21 to see what needs to be done to make this work.
Don't hesitate to ask any question, either here or on Slack.

Comment 26

[Olena Velychko]

Attachment: context-menu.patch Added 'Open URL in New Tab' menu item

Hello Nicolas
So I added new menu item to the context menu. The patch is attached. But I have a couple of questions :)

1. What is the right way to check if we are dealing with the url? Some menu items use property 'visible' to check on some conditions to determine if the item should be shown or not. And some use 'if blocks' for this purpose. Shouldn't it be consistent?
2. This context menu already has 'Open URL in New Tab' for the network requests. And this menu item situated at the top part of the menu while those link-connected menu items are at the bottom part. Maybe, whenever this menu item is visible it should be at the same line of the menu?

Comment 27

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Olena, thanks for working on this!

What is the right way to check if we are dealing with the url? Some menu items use property 'visible' to check on some conditions to determine if the item should be shown or not. And some use 'if' blocks for this purpose. Shouldn't it be consistent?

Yes, we should be consistent. My preference would be to put those into if blocks: if the menu item won't be visible, we shouldn't create it at all (because we're spending much needed time for something that will be useless).

This context menu already has 'Open URL in New Tab' for the network requests. And this menu item situated at the top part of the menu while those link-connected menu items are at the bottom part. Maybe, whenever this menu item is visible it should be at the same line of the menu?

Yeah, it would be nice to re-organize the context menu, as it grew a bit organically and could be better.
We should do that in a follow-up bug, as this one should only be related to the new entry.

---

By testing the patch, I see that it doesn't open the link in the background, which it should do (if we want to be similar to Firefox's links context menu). We also want the tab to open right-next to the current one.

You can do that by passing options to openContentLink:

openContentLink(url, {
      inBackground: true,
      relatedToCurrent: true,
    })

After that, we'll need a test to make sure the context menu entry appears when we right-click on a link.

You can check devtools/client/webconsole/test/mochitest/browser_webconsole_context_menu_copy_link_location.js to get some inspiration for checking the context menu entry, and devtools/client/webconsole/test/mochitest/browser_webconsole_clickable_urls.js to see how to check that the link was opened in the background.

When all of this is done, your patch will be ready for review, and you can follow https://docs.firefox-dev.tools/contributing/code-reviews.html to know how to ask for review on a patch :)

Thank you again!

Comment 28

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Olena,

Do you still plan to work on this one? That's totally okay if you can't/don't want :)

Comment 29

[Olena Velychko]

Yes, I'm sorry. I still plan to work on it. I'll try to finish it in the nearest time.

Comment 30

[Patrick Brosset <:pbro>]

No activity for 2 months. Unassigning for now. But feel free to claim it again if/when you plan on working on it again. In the meantime let's open this up to other people.

Comment 31

[Raymond Liu]

Hello, I am a University of Toronto student interested in picking this bug up as my first Mozilla bug. Can I work on this?

Comment 32

[Raymond Liu]

I followed Nicolas' recommendations above. Since browser_webconsole_clickable_urls.js already has some tests for checking for "Open URL in New Tab", I added a new test for this change to that file.

Comment 33

[Raymond Liu]

Attachment: openURL.patch

Comment 34

[Raymond Liu]

(In reply to Raymond Liu from comment #32)

I followed Nicolas' recommendations above. Since browser_webconsole_clickable_urls.js already has some tests for checking for "Open URL in New Tab", I added a new test for this change to that file.

That is a typo, I meant browser_webconsole_context_menu_open_url.js.

Comment 35

[Nicolas Chevobbe [:nchevobbe][PTO - back on Monday May 13th]]

Hello Raymond, thanks for taking this one. The bug is now assigned to you.

Could you send the patch to phabricator please? https://docs.firefox-dev.tools/contributing/making-prs.html (make sure to use moz-phab and not arc as stated in the doc).

Comment 36

[Raymond Liu]

I can't seem to run moz-phab. I get this error 'Failed to run /home/rayml/.mozbuild/moz-phab/arcanist/bin/arc.'
I installed moz-phab using pip3 install --user MozPhab

Comment 37

[Raymond Liu]

Attachment: Bug 1214556 - Add open url MenuItem for links in Web Console. r=nchevobbe

Comment 38

[Raymond Liu]

Attachment: Bug 1214556 - Add open url MenuItem for links in Web Console. r=nchevobbe

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26e47abeac8a
Add open url MenuItem for links in Web Console. r=nchevobbe

Comment 40

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/26e47abeac8a