Bug 1006875 - Better styles for graphs in web audio editor, r=vp

browser/devtools/webaudioeditor/test/browser.ini

@@ -19,6 +19,7 @@ support-files =
 [browser_wa_graph-click.js]
 [browser_wa_graph-render-01.js]
 [browser_wa_graph-render-02.js]
+[browser_wa_graph-markers.js]
 
 [browser_wa_properties-view.js]
 [browser_wa_properties-view-edit.js]

browser/devtools/webaudioeditor/test/browser_wa_graph-markers.js

@@ -0,0 +1,76 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/**
+ * Tests that the SVG marker styling is updated when devtools theme changes.
+ */
+
+function spawnTest() {
+  let [target, debuggee, panel] = yield initWebAudioEditor(SIMPLE_CONTEXT_URL);
+  let { panelWin } = panel;
+  let { gFront, $, $$, EVENTS, MARKER_STYLING } = panelWin;
+
+  let currentTheme = Services.prefs.getCharPref("devtools.theme");
+
+  ok(MARKER_STYLING.light, "Marker styling exists for light theme.");
+  ok(MARKER_STYLING.dark, "Marker styling exists for dark theme.");
+
+  let started = once(gFront, "start-context");
+
+  reload(target);
+
+  let [actors] = yield Promise.all([
+    get3(gFront, "create-node"),
+    waitForGraphRendered(panelWin, 3, 2)
+  ]);
+
+  is(getFill($("#arrowhead")), MARKER_STYLING[currentTheme],
+    "marker initially matches theme.");
+
+  // Switch to light
+  setTheme("light");
+  is(getFill($("#arrowhead")), MARKER_STYLING.light,
+    "marker styling matches light theme on change.");
+
+  // Switch to dark
+  setTheme("dark");
+  is(getFill($("#arrowhead")), MARKER_STYLING.dark,
+    "marker styling matches dark theme on change.");
+
+  // Switch to dark again
+  setTheme("dark");
+  is(getFill($("#arrowhead")), MARKER_STYLING.dark,
+    "marker styling remains dark.");
+
+  // Switch to back to light again
+  setTheme("light");
+  is(getFill($("#arrowhead")), MARKER_STYLING.light,
+    "marker styling switches back to light once again.");
+
+  yield teardown(panel);
+  finish();
+}
+
+/**
+ * Returns a hex value found in styling for an element. So parses
+ * <marker style="fill: #abcdef"> and returns "#abcdef"
+ */
+function getFill (el) {
+  return el.getAttribute("style").match(/(#.*)$/)[1];
+}
+
+/**
+ * Mimics selecting the theme selector in the toolbox;
+ * sets the preference and emits an event on gDevTools to trigger
+ * the themeing.
+ */
+function setTheme (newTheme) {
+  let oldTheme = Services.prefs.getCharPref("devtools.theme");
+  info("Setting `devtools.theme` to \"" + newTheme + "\"");
+  Services.prefs.setCharPref("devtools.theme", newTheme);
+  gDevTools.emit("pref-changed", {
+    pref: "devtools.theme",
+    newValue: newTheme,
+    oldValue: oldTheme
+  });
+}

browser/devtools/webaudioeditor/webaudioeditor-controller.js

@@ -8,6 +8,7 @@ const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
+Cu.import("resource:///modules/devtools/gDevTools.jsm");
 
 // Override DOM promises with Promise.jsm helpers
 const { defer, all } = Cu.import("resource://gre/modules/Promise.jsm", {}).Promise;
@@ -37,6 +38,9 @@ const EVENTS = {
   // On a node parameter's change.
   CHANGE_PARAM: "WebAudioEditor:ChangeParam",
 
+  // When the devtools theme changes.
+  THEME_CHANGE: "WebAudioEditor:ThemeChange",
+
   // When the UI is reset from tab navigation.
   UI_RESET: "WebAudioEditor:UIReset",
 
@@ -141,6 +145,7 @@ let WebAudioEditorController = {
    */
   initialize: function() {
     this._onTabNavigated = this._onTabNavigated.bind(this);
+    this._onThemeChange = this._onThemeChange.bind(this);
     gTarget.on("will-navigate", this._onTabNavigated);
     gTarget.on("navigate", this._onTabNavigated);
     gFront.on("start-context", this._onStartContext);
@@ -149,6 +154,11 @@ let WebAudioEditorController = {
     gFront.on("disconnect-node", this._onDisconnectNode);
     gFront.on("change-param", this._onChangeParam);
 
+    // Hook into theme change so we can change
+    // the graph's marker styling, since we can't do this
+    // with CSS
+    gDevTools.on("pref-changed", this._onThemeChange);
+
     // Set up events to refresh the Graph view
     window.on(EVENTS.CREATE_NODE, this._onUpdatedContext);
     window.on(EVENTS.CONNECT_NODE, this._onUpdatedContext);
@@ -169,6 +179,7 @@ let WebAudioEditorController = {
     window.off(EVENTS.CREATE_NODE, this._onUpdatedContext);
     window.off(EVENTS.CONNECT_NODE, this._onUpdatedContext);
     window.off(EVENTS.DISCONNECT_NODE, this._onUpdatedContext);
+    gDevTools.off("pref-changed", this._onThemeChange);
   },
 
   /**
@@ -179,6 +190,15 @@ let WebAudioEditorController = {
     WebAudioGraphView.draw();
   },
 
+  /**
+   * Fired when the devtools theme changes (light, dark, etc.)
+   * so that the graph can update marker styling, as that
+   * cannot currently be done with CSS.
+   */
+  _onThemeChange: function (event, data) {
+    window.emit(EVENTS.THEME_CHANGE, data.newValue);
+  },
+
   /**
    * Called for each location change in the debugged tab.
    */
@@ -318,4 +338,3 @@ function getViewNodeByActor (actor) {
 function getViewNodeById (id) {
   return getViewNodeByActor({ actorID: id });
 }
-

browser/devtools/webaudioeditor/webaudioeditor-view.js

@@ -25,6 +25,12 @@ const HEIGHT = 400;
 const ARROW_HEIGHT = 5;
 const ARROW_WIDTH = 8;
 
+// Styles for markers as they cannot be done with CSS.
+const MARKER_STYLING = {
+  light: "#AAA",
+  dark: "#CED3D9"
+};
+
 const GRAPH_DEBOUNCE_TIMER = 100;
 
 const GENERIC_VARIABLES_VIEW_SETTINGS = {
@@ -45,8 +51,12 @@ let WebAudioGraphView = {
    */
   initialize: function() {
     this._onGraphNodeClick = this._onGraphNodeClick.bind(this);
+    this._onThemeChange = this._onThemeChange.bind(this);
+
     this.draw = debounce(this.draw.bind(this), GRAPH_DEBOUNCE_TIMER);
     $('#graph-target').addEventListener('click', this._onGraphNodeClick, false);
+
+    window.on(EVENTS.THEME_CHANGE, this._onThemeChange);
   },
 
   /**
@@ -57,6 +67,7 @@ let WebAudioGraphView = {
       this._zoomBinding.on("zoom", null);
     }
     $('#graph-target').removeEventListener('click', this._onGraphNodeClick, false);
+    window.off(EVENTS.THEME_CHANGE, this._onThemeChange);
   },
 
   /**
@@ -175,10 +186,14 @@ let WebAudioGraphView = {
     // Override Dagre-d3's post render function by passing in our own.
     // This way we can leave styles out of it.
     renderer.postRender(function (graph, root) {
-      // TODO change arrowhead color depending on theme-dark/theme-light
-      // and possibly refactor rendering this as it's ugly
-      // Bug 994256
-      // let color = window.classList.contains("theme-dark") ? "#f5f7fa" : "#585959";
+      // We have to manually set the marker styling since we cannot
+      // do this currently with CSS, although it is in spec for SVG2
+      // https://svgwg.org/svg2-draft/painting.html#VertexMarkerProperties
+      // For now, manually set it on creation, and the `_onThemeChange`
+      // function will fire when the devtools theme changes to update the
+      // styling manually.
+      let theme = Services.prefs.getCharPref("devtools.theme");
+      let markerColor = MARKER_STYLING[theme];
       if (graph.isDirected() && root.select("#arrowhead").empty()) {
         root
           .append("svg:defs")
@@ -191,7 +206,7 @@ let WebAudioGraphView = {
           .attr("markerWidth", ARROW_WIDTH)
           .attr("markerHeight", ARROW_HEIGHT)
           .attr("orient", "auto")
-          .attr("style", "fill: #f5f7fa")
+          .attr("style", "fill: " + markerColor)
           .append("svg:path")
           .attr("d", "M 0 0 L 10 5 L 0 10 z");
       }
@@ -219,6 +234,17 @@ let WebAudioGraphView = {
    * Event handlers
    */
 
+  /**
+   * Fired when the devtools theme changes.
+   */
+  _onThemeChange: function (eventName, theme) {
+    let markerColor = MARKER_STYLING[theme];
+    let marker = $("#arrowhead");
+    if (marker) {
+      marker.setAttribute("style", "fill: " + markerColor);
+    }
+  },
+
   /**
    * Fired when a node in the svg graph is clicked. Used to handle triggering the AudioNodePane.
    *

browser/themes/shared/devtools/webaudioeditor.inc.css

@@ -46,39 +46,40 @@ svg {
 
 /* Edges in graph */
 .edgePath path {
-  stroke-width: 1.5px;
+  stroke-width: 1px;
   fill: none;
 }
 
-.theme-dark .edgePath path { stroke: #f5f7fa; }
-.theme-light .edgePath path { stroke: #585959; }
+.theme-dark .edgePath path {
+  stroke: #b6babf; /* Grey foreground text */
+}
+.theme-light .edgePath path {
+  stroke: #aaaaaa; /* Splitters */
+}
 
 /* Audio Nodes */
 .nodes rect {
-  stroke-width: 2px;
+  stroke-width: 1px;
   cursor: pointer;
 }
 
 .theme-dark .nodes rect {
-  stroke: #585959;
-  fill: #f5f7fa;
+  stroke: #252c33; /* Tab toolbar */
+  fill: #343c45; /* Toolbars */
 }
 
 .theme-light .nodes rect {
-  fill: #585959;
-  stroke: #f5f7fa;
+  stroke: #ebeced; /* Tab toolbar */
+  fill: #f0f1f2; /* Toolbar */
 }
 
-.nodes g.selected rect {
-  stroke-width: 1.5px;
-  animation-duration: 1s;
-  animation-iteration-count: infinite;
-  animation-timing-function: ease-in-out;
-  animation-direction: alternate;
+.theme-dark .nodes g.selected rect {
+  fill: #1d4f73; /* Select Highlight Blue */
 }
 
-.theme-dark .nodes g.selected rect { animation-name: dark-selected-glow; }
-.theme-light .nodes g.selected rect { animation-name: light-selected-glow; }
+.theme-light .nodes g.selected rect {
+  fill: #4c9ed9; /* Select Highlight Blue */
+}
 
 /* Text in nodes */
 text {
@@ -89,23 +90,13 @@ text {
 }
 
 .theme-dark text {
-  fill: #333;
-  /* text-shadow: 0 1px 0 #fff, 1px 0 0 #fff, 0 -1px 0 #fff, -1px 0 0 #fff; */
+  fill: #b6babf; /* Grey foreground text */
 }
 .theme-light text {
-  fill: #111;
+  fill: #585959; /* Grey foreground text */
 }
-
-/**
- * Animations for graphs
- */
-@keyframes dark-selected-glow {
-  0% { fill: #1d4f73; }
-  100% { fill: #3CA3ED; }
-}
-@keyframes light-selected-glow {
-  0% { fill: #4c9ed9; }
-  100% { fill: #59BAFF; }
+.theme-light g.selected text {
+  fill: #f0f1f2; /* Toolbars */
 }
 
 /**