Closed Bug 921102 Opened 11 years ago Closed 9 years ago

[markup view] Make URLs in href attributes and the like clickable

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: seth, Assigned: pbro)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [polish-backlog][difficulty=medium][firebug-gap])

Attachments

(4 files, 1 obsolete file)

Right now in the Inspector's HTML view I can't click on URLs in |href| attributes to follow the link. This is particularly aggravating when debugging SVG content which uses <use href="..."> frequently; there's no easy way to see what <use> is referencing with our dev tools.

We should be sure to implement this in a less brain-dead way than Chrome's current implementation. Clicking URLs works for them, but it always opens a new page. This is useless for cases like <use>. What I'd really like to see happen when the URL is just a fragment identifier (e.g. <use href="#foo">) is that we should jump in the HTML view to the referenced node in the document.
Blocks: firebug-gaps
Whiteboard: [devedition-40][firebug-gap]
Whiteboard: [devedition-40][firebug-gap] → [devedition-40][firebug-gap][difficulty=hard]
Assigning P1 priority for some devedition-40 bugs. 

Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
I'm actually not sure how much work this is going to be.    Cc'ing a few people who may have an idea.

There's a comment in markup-view.js that indicates that it's hooking up <a> tags generated by the output parser: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#1817.  But as far as I remember, we don't use the output parser on the markup view anymore.  I'm not sure the best approach here - do we want to use a limited version of the output parser (that just linkifies URLs), or maybe a small linkify function?
Summary: Make URLs in href attributes and the like clickable → [markup view] Make URLs in href attributes and the like clickable
Whiteboard: [devedition-40][firebug-gap][difficulty=hard] → [devedition-40][firebug-gap]
We could handle *only* the case where there is an attribute value that is entirely a URL (i.e. <img src="foo.png" />).  This would allow us to do a simple regex check and turn that into a clickable link, without using the output parser.

It wouldn't handle <div style="background-image: url(foo.png);"></div>, but it might be quite a bit easier to implement.
I'm in favor of not using the output-parser for this. At least yet. The output-parser, even though it started as a way to parse any kind of output, is now clearly meant for css values only. And we intend to make this even more the case with the refactoring using the css lexer (bug 1153305).
I'm also in favor of only supporting src, href attributes and the likes for now (so, attributes that are urls only).
I'm not so sure about using a simple regex though. Links can come in a wide variety of shapes and forms.
Also, one of the requirements in this bug was that local ID links like <use href="#foo"> would select the #foo element instead of opening a new tab.
So I think we first need some logic to know if an attribute is a link, and then a second part that decides what to do based on the type of link.
This, by the way, could very well be at /framework or /shared level
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Whiteboard: [devedition-40][firebug-gap] → [devedition-40][difficulty=medium][firebug-gap]
I started working on this bug. I'm currently building a utility module that takes in a documentType, tagName, attributeName and attributeValue, and based on those, spits out a document fragment with links in the right places.

The number of attributes that support some form of links (either local to the document or external) and the number of types of links is such that I'm convinced we need a separate reusable) module to handle this.
This could go into output-parser by the way, as a separate function.
Blocks: 1158822
My plan so far, and I have patches that work, is to have a utility that parses attributes, and generates a small, AST-like, data structure that will be sent along with the list of attributes in each NodeActor sent to the devtools front-end.
Based on this information, the markup-view shows the attributes as links.
As for actually following the links, I'm currently implementing this as an item in the contextual menu, to avoid breaking attribute edition/selection with the mouse.
I plan on having 2 items: open link, and copy link to clipboard.
I will have 2 types of links:
- links that open a URL in a new tab
- links that select another node in the inspector (<label for="an-id">, <key command="command-id"> for example)
There is in theory a 3rd type of link which is opening a resource in the style-editor or debugger, but I've decided to file follow-up bug 1158822 to do this.
Blocks: 1158831
Blocks: 1159725
/r/7845 - Bug 921102 - 1 - Linkify URIs in the inspector
/r/7847 - Bug 921102 - 2 - Markup-view tests for attribute links
/r/7849 - Bug 921102 - 3 - Open/copy markup-view attribute links
/r/7851 - Bug 921102 - 4 - Tests for the open/copy links on markup-view attributes

Pull down these commits:

hg pull -r 1eadaf647dd0042faaf0affafa07240723038e00 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599315 - Flags: review?(ttromey)
Attachment #8599315 - Flags: review?(mratcliffe)
Attachment #8599315 - Flags: review?(bgrinstead)
https://reviewboard.mozilla.org/r/7845/#review6671

::: browser/devtools/shared/node-attribute-parser.js:10
(Diff revision 1)
> + * This module contains a small element attribute value parser. It's primary
> + * goal is to extra link information from attribute values like the href in
> + * <a href="/some/link.html">.
> + * There are several types of linkable attribute values:
> + * - URI (<a href>)
> + * - List of URIs (<a ping>)
> + * - ID reference (<label for>, <key command>)
> + * - List of ID references (<output for>)

I think it would be better if this documented the meaning of each TYPE\_ constant.  Right now a couple are missing, like TYPE_RESOURCE_URI.

There were parts of the markup-view.js change that I didn't understand.  Someone else should probably take a look at it.  Otherwise I just have the one nit.
(In reply to Tom Tromey :tromey from comment #9)
> https://reviewboard.mozilla.org/r/7845/#review6671
> 
> ::: browser/devtools/shared/node-attribute-parser.js:10
> (Diff revision 1)
> > + * This module contains a small element attribute value parser. It's primary
> > + * goal is to extra link information from attribute values like the href in
> > + * <a href="/some/link.html">.
> > + * There are several types of linkable attribute values:
> > + * - URI (<a href>)
> > + * - List of URIs (<a ping>)
> > + * - ID reference (<label for>, <key command>)
> > + * - List of ID references (<output for>)
> 
> I think it would be better if this documented the meaning of each TYPE\_
> constant.  Right now a couple are missing, like TYPE_RESOURCE_URI.
Thanks Tom, I have updated the comments throughout the file, thanks for pointing this out.
> There were parts of the markup-view.js change that I didn't understand. 
> Someone else should probably take a look at it.  Otherwise I just have the
> one nit.
I'll ask Mike to review the markup-view changes in part 1.
https://reviewboard.mozilla.org/r/7845/#review6675

> I think it would be better if this documented the meaning of each TYPE\_ constant.  Right now a couple are missing, like TYPE_RESOURCE_URI.

I have updated the comment.

Will ask Mike to review these.
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

/r/7845 - Bug 921102 - 1 - Linkify URIs in the inspector
/r/7847 - Bug 921102 - 2 - Markup-view tests for attribute links
/r/7849 - Bug 921102 - 3 - Open/copy markup-view attribute links
/r/7851 - Bug 921102 - 4 - Tests for the open/copy links on markup-view attributes

Pull down these commits:

hg pull -r 3cac04fa31dae4b4ebb265eb14eccc880533547e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599315 - Flags: review?(mratcliffe) → review+
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

https://reviewboard.mozilla.org/r/7843/#review6677

Just a couple of nits:
- Add any other TYPES that may come up as Tom mentioned.
- Type in comment.
- It is up to you whether you change resolveRelativeURL but we really should avoid creating elements when we don't need to.

::: browser/devtools/inspector/inspector-panel.js:695
(Diff revision 1)
> +    // Enable/Disable the link opne/copy items.

s/opne/open/

::: toolkit/devtools/server/actors/inspector.js:3484
(Diff revision 1)
> +  resolveRelativeURL: method(function(url, node) {

Creating an element to get the relative URL seems wrong when we already have nsIURI.

You are probably better using this:
```
function resolveRelativeURL(url, node) {
  let document;
  
  if (!node) {
    document = this.window.document;
  } else {
    document = node.rawNode.ownerDocument;
  }
  
  let makeURI = function(aURL, aOriginCharset, aBaseURI) {
    return Services.io.newURI(aURL, document.characterSet, aBaseURI);
  }

  let base = makeURI(document.location.href);
  return makeURI(url, null, base)  
}
```
I meant:
```
function resolveRelativeURL(url, node) {
  let document;
  
  if (!node) {
    document = this.window.document;
  } else {
    document = node.rawNode.ownerDocument;
  }
  
  let makeURI = function(aURL, aOriginCharset, aBaseURI) {
    return Services.io.newURI(aURL, document.characterSet, aBaseURI);
  }

  let base = makeURI(document.location.href);
  return makeURI(url, document.characterSet, base)  
}
```
Thanks Mike, I will proceed with the changes.
Pending try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4ef398f4a39
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

/r/7845 - Bug 921102 - 1 - Linkify URIs in the inspector
/r/7847 - Bug 921102 - 2 - Markup-view tests for attribute links
/r/7849 - Bug 921102 - 3 - Open/copy markup-view attribute links
/r/7851 - Bug 921102 - 4 - Tests for the open/copy links on markup-view attributes

Pull down these commits:

hg pull -r 037b3292b071f67dcb8e8d3e3dcba0d5fae3a9a3 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8599315 - Flags: review+ → review?(mratcliffe)
https://reviewboard.mozilla.org/r/7843/#review6679

> s/opne/open/

fixed.

> Creating an element to get the relative URL seems wrong when we already have nsIURI.
> 
> You are probably better using this:
> ```
> function resolveRelativeURL(url, node) {
>   let document;
>   
>   if (!node) {
>     document = this.window.document;
>   } else {
>     document = node.rawNode.ownerDocument;
>   }
>   
>   let makeURI = function(aURL, aOriginCharset, aBaseURI) {
>     return Services.io.newURI(aURL, document.characterSet, aBaseURI);
>   }
> 
>   let base = makeURI(document.location.href);
>   return makeURI(url, null, base)  
> }
> ```

I had never used this API, thanks for pointing it out, it's much better than create DOM nodes indeed.
I have changed the code.
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

Carrying over Mike's R+.
Also, I believe comment 9 was implicit R+ from Tom for the changes in node-attribute-parser.js (part 1 patch). Tom let me know if I misunderstood.
Attachment #8599315 - Flags: review?(ttromey)
Attachment #8599315 - Flags: review?(mratcliffe)
Attachment #8599315 - Flags: review+
https://reviewboard.mozilla.org/r/7849/#review6643

r=me with the updates below

::: browser/devtools/inspector/inspector-panel.js:695
(Diff revision 1)
> +    // Enable/Disable the link opne/copy items.

Typo - 'open'

::: browser/devtools/inspector/inspector-panel.js:1105
(Diff revision 3)
> +      this.inspector.resolveRelativeURL(link, this.selection.nodeFront).then(url => {

This should have a comment that backwards compat and tab target have already been handled when setting the visibility of the button that calls this function.

::: browser/devtools/inspector/inspector-panel.js:1113
(Diff revision 3)
> +          this.selection.setNodeFront(node);

We should be tolerant of bad input here.  Right now it ends up setting a null node front which can cause some weirdness with the marku view and errors in the browser console.  It'd be great if we could actually disable the button in the case of invalid input, but I'd be fine with just checking to make sure that the node exists before setting the front

::: toolkit/devtools/server/tests/mochitest/test_inspector-resolve-url.html:36
(Diff revision 3)
> +addTest(function testLargeImage() {

Nit: All of these function names are testLargeImage

::: toolkit/devtools/server/actors/inspector.js:3485
(Diff revision 3)
> +    let document;

We should use the isNodeDead and nodeDocument functions for error tolerance and to handle if a document is passed in as node:

Something like:

let document = isNodeDead(node) ? this.window.document : nodeDocument(node.rawNode);
https://reviewboard.mozilla.org/r/7851/#review6777

Looks good, just a couple of additional cases to test

::: browser/devtools/markupview/test/browser_markupview_links_05.js:37
(Diff revision 3)
> +  info("Select a node with a IDREF attribute");

We should also handle the case where a node with a bad idref value is chosen.  I guess in that case we expect that the selection won't have changed (or that the menu item will be disabled).

::: browser/devtools/markupview/test/browser_markupview_links_05.js:57
(Diff revision 3)
> +    // Skip load event for about:blank

Is opening about:blank part of this test case, or is this function just copied in from somewhere else?

::: browser/devtools/markupview/test/browser_markupview_links_04.js:46
(Diff revision 3)
> +  selector: "output",

We should also have a check for an idref attribute like `for` on a node that doesn't support it (like `<div>`) to make sure that the menu item doesn't show up

::: browser/devtools/markupview/test/browser_markupview_links_04.js:86
(Diff revision 3)
> +    yield inspector.target.actorHasMethod("inspector", "resolveRelativeURL");

If the implementation of showing/hiding the menu items ends up changing, we may want to just emit an event when the menu is ready.  For now it's ok though
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

https://reviewboard.mozilla.org/r/7843/#review6781

Ship It!
Attachment #8599315 - Flags: review?(bgrinstead) → review+
https://reviewboard.mozilla.org/r/7851/#review6813

> Is opening about:blank part of this test case, or is this function just copied in from somewhere else?

I don't open about:blank on purpose in this test, but adding a "load" event listener on a tab makes it be called several times with about:blank before it's called for the expected URL, so I'm forced to skip these. I think I've seen this in many other tests too.
Attachment #8599315 - Flags: review?(ttromey)
Attachment #8599315 - Flags: review?(mratcliffe)
Attachment #8599315 - Flags: review?(bgrinstead)
Attachment #8599315 - Flags: review+
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

/r/7845 - Bug 921102 - 1 - Linkify URIs in the inspector
/r/7847 - Bug 921102 - 2 - Markup-view tests for attribute links
/r/7849 - Bug 921102 - 3 - Open/copy markup-view attribute links
/r/7851 - Bug 921102 - 4 - Tests for the open/copy links on markup-view attributes

Pull down these commits:

hg pull -r d03061e861ce17e73c9bbca15b9e62fbeed11e9e https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8599315 [details]
MozReview Request: bz://921102/pbrosset

Damn, the interactions between mozreview and bugzilla are sooooo confusing right now. I never get this right.
Anyway, I uploaded new patches that should address all remaining review comments.
Setting R+ again.
Also, new try push (last one was all green and I don't expect the last changes to cause any oranges): https://treeherder.mozilla.org/#/jobs?repo=try&revision=7124dc5b6134
Attachment #8599315 - Flags: review?(ttromey)
Attachment #8599315 - Flags: review?(mratcliffe)
Attachment #8599315 - Flags: review?(bgrinstead)
Attachment #8599315 - Flags: review+
(In reply to Seth Fowler [:seth] - PTO until 6/12 from comment #0)
> We should be sure to implement this in a less brain-dead way than Chrome's
> current implementation. Clicking URLs works for them, but it always opens a
> new page. This is useless for cases like <use>. What I'd really like to see
> happen when the URL is just a fragment identifier (e.g. <use href="#foo">)
> is that we should jump in the HTML view to the referenced node in the
> document.

What happened to this requirement? It seems those URLs are also opened in a new tab. (Only tried with anchor links on this page, not within an SVG.)

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #27)
> (In reply to Seth Fowler [:seth] - PTO until 6/12 from comment #0)
> > We should be sure to implement this in a less brain-dead way than Chrome's
> > current implementation. Clicking URLs works for them, but it always opens a
> > new page. This is useless for cases like <use>. What I'd really like to see
> > happen when the URL is just a fragment identifier (e.g. <use href="#foo">)
> > is that we should jump in the HTML view to the referenced node in the
> > document.
> 
> What happened to this requirement? It seems those URLs are also opened in a
> new tab. (Only tried with anchor links on this page, not within an SVG.)
> 
> Sebastian
So, right now we have:
- all links to js and css resources are opened in the debugger and style-editor (<script src>, <link href>, ...)
- all links to other resources are opened in a new tab (<a href>, ...)
- all links to other elements select that element in the inspector (<div contextmenu>, ...)

When it comes to element references in SVG, they're not handled yet, see bug 1158831.
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #28)
> When it comes to element references in SVG, they're not handled yet, see bug
> 1158831.

Does that bug also cover anchor links within HTML (<a href="...#...">)?

Sebastian
Attachment #8599315 - Attachment is obsolete: true
Attachment #8618045 - Flags: review+
Attachment #8618046 - Flags: review+
Attachment #8618047 - Flags: review+
Attachment #8618048 - Flags: review+
Whiteboard: [devedition-40][difficulty=medium][firebug-gap] → [polish-backlog][difficulty=medium][firebug-gap]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.