Bug 1255529 - Make all of a location (source, line, column) clickable/linkable to another tool in the frame component. r=fitzgen,bgrins
authorJordan Santell <jsantell@mozilla.com>
Fri, 11 Mar 2016 14:16:47 -0800
changeset 289230 24ff79c5fb013cd016219c91b86482d31f7e09de
parent 289229 02cf3a4bf42ae31d09d2088cb25334d13e2523ec
child 289231 cbefc1f9f9f6417e922d361b7c4ece108aa052fc
push id19656
push usergwagner@mozilla.com
push dateMon, 04 Apr 2016 13:43:23 +0000
treeherderb2g-inbound@e99061fde28a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen, bgrins
bugs1255529
milestone48.0a1
Bug 1255529 - Make all of a location (source, line, column) clickable/linkable to another tool in the frame component. r=fitzgen,bgrins
devtools/client/shared/components/frame.js
devtools/client/shared/components/test/mochitest/chrome.ini
devtools/client/shared/components/test/mochitest/head.js
devtools/client/shared/components/test/mochitest/test_frame_01.html
devtools/client/shared/components/test/mochitest/test_frame_02.html
devtools/client/themes/components-frame.css
devtools/client/themes/webconsole.css
--- a/devtools/client/shared/components/frame.js
+++ b/devtools/client/shared/components/frame.js
@@ -43,72 +43,78 @@ module.exports = createClass({
 
     const { short, long, host } = getSourceNames(source);
     // Reparse the URL to determine if we should link this; `getSourceNames`
     // has already cached this indirectly. We don't want to attempt to
     // link to "self-hosted" and "(unknown)". However, we do want to link
     // to Scratchpad URIs.
     const isLinkable = !!(isScratchpadScheme(source) || parseURL(source));
     const elements = [];
+    const sourceElements = [];
+    let sourceEl;
 
     let tooltip = long;
     // Exclude all falsy values, including `0`, as even
     // a number 0 for line doesn't make sense, and should not be displayed.
     // If source isn't linkable, don't attempt to append line and column
     // info, as this probably doesn't make sense.
     if (isLinkable && line) {
       tooltip += `:${line}`;
       // Intentionally exclude 0
       if (column) {
         tooltip += `:${column}`;
       }
     }
 
-    let onClickTooltipString = l10n.getFormatStr("frame.viewsourceindebugger", tooltip);
     let attributes = {
       "data-url": long,
       className: "frame-link",
-      title: tooltip,
     };
 
-    if (isLinkable) {
-      elements.push(dom.a({
-        className: "frame-link-filename",
-        onClick,
-        title: onClickTooltipString
-      }, short));
-    } else {
-      // If source is not a URL (self-hosted, eval, etc.), don't make
-      // it an anchor link, as we can't link to it.
-      elements.push(dom.span({
-        className: "frame-link-filename"
-      }, short));
+    if (showFunctionName && frame.functionDisplayName) {
+      elements.push(
+        dom.span({ className: "frame-link-function-display-name" }, frame.functionDisplayName)
+      );
     }
 
+    sourceElements.push(dom.span({
+      className: "frame-link-filename",
+    }, short));
+
     // If source is linkable, and we have a line number > 0
     if (isLinkable && line) {
-      elements.push(dom.span({ className: "frame-link-colon" }, ":"));
-      elements.push(dom.span({ className: "frame-link-line" }, line));
+      sourceElements.push(dom.span({ className: "frame-link-colon" }, ":"));
+      sourceElements.push(dom.span({ className: "frame-link-line" }, line));
       // Intentionally exclude 0
       if (column) {
-        elements.push(dom.span({ className: "frame-link-colon" }, ":"));
-        elements.push(dom.span({ className: "frame-link-column" }, column));
+        sourceElements.push(dom.span({ className: "frame-link-colon" }, ":"));
+        sourceElements.push(dom.span({ className: "frame-link-column" }, column));
         // Add `data-column` attribute for testing
         attributes["data-column"] = column;
       }
 
       // Add `data-line` attribute for testing
       attributes["data-line"] = line;
     }
 
-    if (showFunctionName && frame.functionDisplayName) {
-      elements.unshift(
-        dom.span({ className: "frame-link-function-display-name" }, frame.functionDisplayName)
-      );
+    // If source is not a URL (self-hosted, eval, etc.), don't make
+    // it an anchor link, as we can't link to it.
+    if (isLinkable) {
+      sourceEl = dom.a({
+        onClick,
+        className: "frame-link-source",
+        title: l10n.getFormatStr("frame.viewsourceindebugger", tooltip)
+      }, sourceElements);
+    } else {
+      sourceEl = dom.span({
+        className: "frame-link-source",
+        title: tooltip,
+      }, sourceElements);
     }
+    elements.push(sourceEl);
 
     if (showHost && host) {
       elements.push(dom.span({ className: "frame-link-host" }, host));
     }
 
     return dom.span(attributes, ...elements);
   }
 });
--- a/devtools/client/shared/components/test/mochitest/chrome.ini
+++ b/devtools/client/shared/components/test/mochitest/chrome.ini
@@ -1,15 +1,14 @@
 [DEFAULT]
 support-files =
   head.js
 
 [test_HSplitBox_01.html]
 [test_frame_01.html]
-[test_frame_02.html]
 [test_tree_01.html]
 [test_tree_02.html]
 [test_tree_03.html]
 [test_tree_04.html]
 [test_tree_05.html]
 [test_tree_06.html]
 [test_tree_07.html]
 [test_tree_08.html]
--- a/devtools/client/shared/components/test/mochitest/head.js
+++ b/devtools/client/shared/components/test/mochitest/head.js
@@ -132,44 +132,35 @@ var TEST_TREE = {
     O: "N"
   },
   expanded: new Set(),
 };
 
 /**
  * Frame
  */
-function checkFrameString({ frame, file, line, column, shouldLink }) {
+function checkFrameString({ frame, file, line, column, shouldLink, tooltip }) {
   let el = frame.getDOMNode();
   let $ = selector => el.querySelector(selector);
 
+  let $source = $(".frame-link-source");
+  let $filename = $(".frame-link-filename");
   let $line = $(".frame-link-line");
   let $column = $(".frame-link-column");
 
-  is($(".frame-link-filename").textContent, file);
-
+  is($filename.textContent, file, "Correct filename");
   is(el.getAttribute("data-line"), line ? `${line}` : null, "Expected `data-line` found");
   is(el.getAttribute("data-column"), column ? `${column}` : null, "Expected `data-column` found");
-
-  if (shouldLink) {
-    ok($("a.frame-link-filename"), "Should have a linkable filename");
-  } else {
-    ok(!$("a.frame-link-filename"), "Should not have a linkable filename");
-  }
+  is($source.getAttribute("title"), tooltip, "Correct tooltip");
+  is($source.tagName, shouldLink ? "A" : "SPAN", "Correct linkable status");
 
   if (line != null) {
     is(+$line.textContent, +line);
   } else {
     ok(!$line, "Should not have an element for `line`");
   }
 
   if (column != null) {
     is(+$column.textContent, +column);
   } else {
     ok(!$column, "Should not have an element for `column`");
   }
 }
-
-function checkFrameTooltips (component, mainTooltip, linkTooltip) {
-  let el = component.getDOMNode();
-  is(el.getAttribute("title"), mainTooltip);
-  is(el.querySelector(".frame-link-filename").getAttribute("title"), linkTooltip);
-}
--- a/devtools/client/shared/components/test/mochitest/test_frame_01.html
+++ b/devtools/client/shared/components/test/mochitest/test_frame_01.html
@@ -18,184 +18,143 @@ window.onload = Task.async(function* () 
   try {
     let ReactDOM = browserRequire("devtools/client/shared/vendor/react-dom");
     let React = browserRequire("devtools/client/shared/vendor/react");
     let Frame = React.createFactory(browserRequire("devtools/client/shared/components/frame"));
     ok(Frame, "Should get Frame");
     let frame;
 
     // Check when there's a column
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 55,
-        column: 10,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: 55,
+      column: 10,
+    }, {
       file: "mahscripts.js",
       line: 55,
       column: 10,
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js:55:10",
     });
 
     // Check when there's no column
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 55,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: 55,
+    }, {
       file: "mahscripts.js",
       line: 55,
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js:55",
     });
 
     // Check when column === 0
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 55,
-        column: 0,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: 55,
+      column: 0,
+    }, {
       file: "mahscripts.js",
       line: 55,
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js:55",
     });
 
     // Check when there's no parseable URL source;
     // should not render line/columns
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "self-hosted",
-        line: 1,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "self-hosted",
+      line: 1,
+    }, {
       file: "self-hosted",
       shouldLink: false,
+      tooltip: "self-hosted",
     });
 
     // Check when there's no source;
     // should not render line/columns
-    frame = ReactDOM.render(Frame({
-      frame: {
-        line: 1,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      line: 1,
+    }, {
       file: "(unknown)",
       shouldLink: false,
+      tooltip: "(unknown)",
     });
 
     // Check when there's a column, but no number;
     // no line/column info should render
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        column: 55,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      column: 55,
+    }, {
       file: "mahscripts.js",
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js",
     });
 
     // Check when line is 0; this should be an invalid
     // line option, so don't render line/column
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 0,
-        column: 55,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: 0,
+      column: 55,
+    }, {
       file: "mahscripts.js",
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js",
     });
 
     // Check when source is via Scratchpad; we should render out the
     // lines and columns as this is linkable.
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "Scratchpad/1",
-        line: 10,
-        column: 50,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "Scratchpad/1",
+      line: 10,
+      column: 50,
+    }, {
       file: "Scratchpad/1",
       line: 10,
       column: 50,
       shouldLink: true,
+      tooltip: "View source in Debugger → Scratchpad/1:10:50",
     });
 
     // Check that line and column can be strings
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: "10",
-        column: "55",
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: "10",
+      column: "55",
+    }, {
       file: "mahscripts.js",
       line: 10,
       column: 55,
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js:10:55",
     });
 
     // Check that line and column can be strings,
     // and that the `0` rendering rules apply when they are strings as well
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: "0",
-        column: "55",
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameString({
-      frame,
+    yield checkFrameComponent({
+      source: "http://myfile.com/mahscripts.js",
+      line: "0",
+      column: "55",
+    }, {
       file: "mahscripts.js",
       shouldLink: true,
+      tooltip: "View source in Debugger → http://myfile.com/mahscripts.js",
     });
 
+    function* checkFrameComponent (input, expected) {
+      let frame = ReactDOM.render(Frame({
+        frame: input,
+        onClick: () => {},
+      }), window.document.body);
+      yield forceRender(frame);
+      checkFrameString(Object.assign({ frame }, expected));
+    }
+
   } catch (e) {
     ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
   } finally {
     SimpleTest.finish();
   }
 });
 </script>
 </pre>
deleted file mode 100644
--- a/devtools/client/shared/components/test/mochitest/test_frame_02.html
+++ /dev/null
@@ -1,85 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<!--
-Test the formatting of the tooltips in the frame component.
--->
-<head>
-  <meta charset="utf-8">
-  <title>Frame component test</title>
-  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
-  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
-</head>
-<body>
-<pre id="test">
-<script src="head.js" type="application/javascript;version=1.8"></script>
-<script type="application/javascript;version=1.8">
-window.onload = Task.async(function* () {
-  try {
-    let ReactDOM = browserRequire("devtools/client/shared/vendor/react-dom");
-    let React = browserRequire("devtools/client/shared/vendor/react");
-    let Frame = React.createFactory(browserRequire("devtools/client/shared/components/frame"));
-    ok(Frame, "Should get Frame");
-    let frame;
-
-    // Check when there's a column
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 55,
-        column: 10,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameTooltips(frame,
-      "http://myfile.com/mahscripts.js:55:10",
-      "View source in Debugger → http://myfile.com/mahscripts.js:55:10");
-
-    // Check when there's no column
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "http://myfile.com/mahscripts.js",
-        line: 55,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameTooltips(frame,
-      "http://myfile.com/mahscripts.js:55",
-      "View source in Debugger → http://myfile.com/mahscripts.js:55");
-
-    // Check when there's no parseable URL source
-    frame = ReactDOM.render(Frame({
-      frame: {
-        source: "self-hosted",
-        line: 1,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameTooltips(frame,
-      "self-hosted",
-      null);
-
-    // Check when there's no source
-    frame = ReactDOM.render(Frame({
-      frame: {
-        line: 1,
-      },
-      onClick: ()=>{},
-    }), window.document.body);
-    yield forceRender(frame);
-    checkFrameTooltips(frame,
-      "(unknown)",
-      null);
-
-  } catch(e) {
-    ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
-  } finally {
-    SimpleTest.finish();
-  }
-});
-</script>
-</pre>
-</body>
-</html>
--- a/devtools/client/themes/components-frame.css
+++ b/devtools/client/themes/components-frame.css
@@ -8,32 +8,37 @@
  * Styles for React component at `devtools/client/shared/components/frame.js`
  */
 
 .frame-link {
   margin-left: 7px;
   display: flex;
 }
 
+.frame-link .frame-link-source {
+  display: flex;
+}
+.frame-link a.frame-link-source {
+  cursor: pointer;
+  text-decoration: none;
+}
+.frame-link a.frame-link-source:hover {
+  text-decoration: underline;
+}
+
 .frame-link .frame-link-filename {
+  color: var(--theme-highlight-blue);
   text-overflow: ellipsis;
   overflow: hidden;
   flex: 1;
   text-align: right;
   /* overrides styling some tools have with anchors */
   text-decoration: none;
   font-style: normal;
 }
-.frame-link a.frame-link-filename {
-  cursor: pointer;
-  color: var(--theme-highlight-blue);
-}
-.frame-link a.frame-link-filename:hover {
-  text-decoration: underline;
-}
 
 .frame-link .frame-link-host {
   margin-inline-start: 5px;
   font-size: 90%;
   color: var(--theme-content-color2);
 }
 
 .frame-link .frame-link-function-display-name {
@@ -42,16 +47,15 @@
 
 .frame-link .frame-link-column,
 .frame-link .frame-link-line,
 .frame-link .frame-link-colon {
   color: var(--theme-highlight-orange);
   display: block;
 }
 
-.focused a.frame-link-filename,
-.focused span.frame-link-filename,
-.focused .frame-link-column,
-.focused .frame-link-line,
-.focused .frame-link-host,
-.focused .frame-link-colon {
+.focused .frame-link .frame-link-filename,
+.focused .frame-link .frame-link-column,
+.focused .frame-link .frame-link-line,
+.focused .frame-link .frame-link-host,
+.focused .frame-link .frame-link-colon {
   color: var(--theme-selection-color);
 }
--- a/devtools/client/themes/webconsole.css
+++ b/devtools/client/themes/webconsole.css
@@ -97,17 +97,17 @@ a {
   white-space: nowrap;
 }
 
 .message-location:hover,
 .message-location:focus {
   text-decoration: underline;
 }
 
-.message-location > .frame-link {
+.message-location > .frame-link .frame-link-source {
   width: 10em;
 }
 
 .message-flex-body {
   display: flex;
 }
 
 .message-body > * {