Closed Bug 1214556 Opened 5 years ago Closed 11 months ago

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

Categories

(DevTools :: Console, enhancement, P5)

41 Branch
enhancement

Tracking

(firefox72 fixed)

RESOLVED FIXED
Firefox 72
Tracking Status
firefox72 --- fixed

People

(Reporter: sworddragon2, Assigned: raymond.liu.dev, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [btpp-backlog])

Attachments

(4 files)

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.
Severity: normal → enhancement
Component: Untriaged → Developer Tools: Console
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.
Priority: -- → P3
Whiteboard: [btpp-backlog]
Product: Firefox → DevTools
We'll happily accept a patch for this.
Priority: P3 → P5
Mentor: nchevobbe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Different context menus should have more shared entries → Add "open in new tab" context menu entry for links

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.

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 :)

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!

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

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

Sounds good!

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

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.

Got it!! Thanks Nicolas!

(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.

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.

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.

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

(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 :)

Assignee: nobody → rainier.f.go
Status: NEW → ASSIGNED

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

Flags: needinfo?(rainier.f.go)

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

Flags: needinfo?(rainier.f.go)

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?

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.

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 :)

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 :) )

Flags: needinfo?(rainier.f.go)

clearing assignee

Assignee: rainier.f.go → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(rainier.f.go)

Hello Nicolas

I would like to work on this bug.

Olena

Flags: needinfo?(nchevobbe)

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.

Flags: needinfo?(nchevobbe)
Assignee: nobody → mrs.velena
Status: NEW → ASSIGNED

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?
Flags: needinfo?(nchevobbe)

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!

Flags: needinfo?(nchevobbe)

Hello Olena,

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

Flags: needinfo?(mrs.velena)

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

Flags: needinfo?(mrs.velena)

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.

Assignee: mrs.velena → nobody
Status: ASSIGNED → NEW

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

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.

Attached patch openURL.patchSplinter Review

(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.

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).

Assignee: nobody → raymond.liu.dev
Status: NEW → ASSIGNED

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

Attachment #9109885 - Attachment description: Bug 1214556 - Add open url MenuItem for links in Web Console and an associated test. → Bug 1214556 - Add open url MenuItem for links in Web Console. r=nchevobbe
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/26e47abeac8a
Add open url MenuItem for links in Web Console. r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
You need to log in before you can comment on or make changes to this bug.