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 289346 24ff79c5fb013cd016219c91b86482d31f7e09de
parent 289345 02cf3a4bf42ae31d09d2088cb25334d13e2523ec
child 289347 cbefc1f9f9f6417e922d361b7c4ece108aa052fc
push id73798
push usercbook@mozilla.com
push dateFri, 18 Mar 2016 15:10:54 +0000
treeherdermozilla-inbound@5096e12520cd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfitzgen, bgrins
bugs1255529
milestone48.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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 > * {