1307931 - Make urls in output clickable

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Lin Clark [:linclark]]

Originally posted by:linclark

see https://github.com/devtools-html/gecko-dev/issues/348

In the current web console, clicking on URLs will open the link.

Tests:
- [ ] browser_webconsole_clickable_urls.js

Comment 2

[Lin Clark [:linclark]]

Honza, do you think this is something that should happen at the Reps level? For example, if there's a URL in JSON Viewer or debugger, should it also be clickable?

If so, what do you think would be the best course? Maybe do a similar thing as we have with Variables View, were if you pass in a handler function then it will be used as an onClick handler for a URL?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #2)
> Honza, do you think this is something that should happen at the Reps level?
> For example, if there's a URL in JSON Viewer or debugger, should it also be
> clickable?
Yes

> If so, what do you think would be the best course? Maybe do a similar thing
> as we have with Variables View, were if you pass in a handler function then
> it will be used as an onClick handler for a URL?
Passing a handler function in as a prop sounds good to me.

I am seeing two options:

1) Pass in a new prop e.g. 'urlLink' that opens a new tab.
2) Reuse the existing 'objectLink' prop and make it smart in a way the it knows what the user clicked on and whether the variables view (object inspector) or a new tab should be opened.

I tend to think that #2 is better, but the implementation should be nice & clean.

Also note that there is an existing report - bug 1245303, that has also a suggested patch that modifies the StringRep.

Honza

Comment 4

[Lin Clark [:linclark]]

Just realizing that this will potentially break given the console perf enhancement that we discussed on Monday. If we cache the results of reps as HTML, then we can't easily use click handlers on anything inside of reps.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

It looks like gamakichi on IRC started working on this and has a patch ready for review (https://pastebin.mozilla.org/8929855), but they are not there anymore and I can't find its bugzilla account (I don't know if they have one). Consider that this bug is assigned for now.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1307931 - [not commited here] Reps bundle with link for urls.

Review commit: https://reviewboard.mozilla.org/r/164520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164520/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1307931 - Make urls in output clickable; .

This passes an openLink callback to the Reps which handles the output.

Review commit: https://reviewboard.mozilla.org/r/164522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164522/

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Brian, I copied devtools/client/webconsole/test/browser_webconsole_clickable_urls.js over the new console directory for this, but I ended up simplifying the tests quite a bit. Does it still make sense to have it "copied" in the repo, or should I just create a new file ?

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #8)
> Brian, I copied
> devtools/client/webconsole/test/browser_webconsole_clickable_urls.js over
> the new console directory for this, but I ended up simplifying the tests
> quite a bit. Does it still make sense to have it "copied" in the repo, or
> should I just create a new file ?

I think copying is fine

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

https://reviewboard.mozilla.org/r/164522/#review170076

Looks good to me

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164522/diff/1-2/

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1307931 - devtools reps: update bundle to v0.12.1.

Review commit: https://reviewboard.mozilla.org/r/167136/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167136/

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164522/diff/2-3/

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

https://reviewboard.mozilla.org/r/164522/#review172304

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_clickable_urls.js:24
(Diff revision 3)
> -
> -  // 1: URL opens page using https when clicked.
> -  {
> -    input: "'https://example.com'",
> -    output: "https://example.com",
> -    expectedTab: "https://example.com/",
> +  });
> +
> +  let node = await waitFor(() => findMessage(hud, url));
> +  const link = node.querySelector("a.url");
> +
> +  const onTabLoaded = new Promise(resolve => {

Looks like there's a `BrowserTestUtils.waitForNewTab(gBrowser, url, true)` that wraps up this boilerplate around the tabContainer and browserLoaded.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8895846 [details]
Bug 1307931 - devtools reps: update bundle to v0.12.1.

https://reviewboard.mozilla.org/r/167136/#review172306

Looks like there are some try failures but r=me with those addressed

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

https://reviewboard.mozilla.org/r/164522/#review172304

> Looks like there's a `BrowserTestUtils.waitForNewTab(gBrowser, url, true)` that wraps up this boilerplate around the tabContainer and browserLoaded.

Thanks for the tip, test looks cleaner with it.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164522/diff/3-4/

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8895846 [details]
Bug 1307931 - devtools reps: update bundle to v0.12.1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167136/diff/1-2/

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8893408 [details]
Bug 1307931 - Make urls in output clickable; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164522/diff/4-5/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

> Looks like there are some try failures but r=me with those addressed
I fixed the issues and pushed to TRY again

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

TRY looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd736c39abe36116036730ef88a3b1206431b482&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87afcda8a0ff
devtools reps: update bundle to v0.12.1. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/dc8e54688912
Make urls in output clickable; r=bgrins.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/87afcda8a0ff
https://hg.mozilla.org/mozilla-central/rev/dc8e54688912

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Jan Honza Odvarko [:Honza] from comment #3)

> 
> I am seeing two options:
> 
> 1) Pass in a new prop e.g. 'urlLink' that opens a new tab.
> 2) Reuse the existing 'objectLink' prop and make it smart in a way the it
> knows what the user clicked on and whether the variables view (object
> inspector) or a new tab should be opened.
> 
> I tend to think that #2 is better, but the implementation should be nice &
> clean.
> 

I understand that the second option was chosen. In that case, can you please detail what is the expected behaviour? In what cases the variable view and a new tab are opened?

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Hello Iulia,

We don't have the concept of the variable view anymore in the console, so I thin comment 3 is obsolete.
This bug is only about being able to click on a URL when one is logged (e.g. with `console.log("http://www.google.com")`).
Clicking on the link should always open a new tab.

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thanks Nicolas for your quick answer! 
Managed to reproduce the initial issue on 52.0a1 (2016-10-22). I can confirm the bug is verified fixed on 57.0a1 (2017-08-22), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.