Bug 1442001 - Remove the expander icon for fonts and the used-as info; r=jdescottes draft
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 28 Feb 2018 21:45:27 +0100
changeset 761762 eb0bd83dd4111d8fb3200d6f2e29a1b7f5ee5c67
parent 760935 ee326c976eebdca48128054022c443d3993e12b0
push id100989
push userbmo:pbrosset@mozilla.com
push dateThu, 01 Mar 2018 12:22:14 +0000
reviewersjdescottes
bugs1442001
milestone60.0a1
Bug 1442001 - Remove the expander icon for fonts and the used-as info; r=jdescottes MozReview-Commit-ID: Gf19Ybo74jc
devtools/client/inspector/fonts/components/Font.js
devtools/client/inspector/fonts/components/FontPreview.js
devtools/client/inspector/fonts/test/browser.ini
devtools/client/inspector/fonts/test/browser_fontinspector.js
devtools/client/inspector/fonts/test/browser_fontinspector_expand-css-code.js
devtools/client/inspector/fonts/test/browser_fontinspector_expand-details.js
devtools/client/inspector/fonts/test/head.js
devtools/client/inspector/fonts/types.js
devtools/client/locales/en-US/font-inspector.properties
devtools/client/themes/fonts.css
--- a/devtools/client/inspector/fonts/components/Font.js
+++ b/devtools/client/inspector/fonts/components/Font.js
@@ -21,58 +21,39 @@ class Font extends PureComponent {
       onPreviewFonts: PropTypes.func.isRequired,
     };
   }
 
   constructor(props) {
     super(props);
 
     this.state = {
-      isFontExpanded: false,
       isFontFaceRuleExpanded: false,
     };
 
-    this.onFontToggle = this.onFontToggle.bind(this);
     this.onFontFaceRuleToggle = this.onFontFaceRuleToggle.bind(this);
   }
 
   componentWillReceiveProps(newProps) {
     if (this.props.font.name === newProps.font.name) {
       return;
     }
 
     this.setState({
-      isFontExpanded: false,
       isFontFaceRuleExpanded: false,
     });
   }
 
-  onFontToggle(event) {
-    this.setState({
-      isFontExpanded: !this.state.isFontExpanded
-    });
-    event.stopPropagation();
-  }
-
   onFontFaceRuleToggle(event) {
     this.setState({
       isFontFaceRuleExpanded: !this.state.isFontFaceRuleExpanded
     });
     event.stopPropagation();
   }
 
-  renderFontCSS(cssFamilyName) {
-    return dom.p(
-      {
-        className: "font-css-name"
-      },
-      `${getStr("fontinspector.usedAs")} "${cssFamilyName}"`
-    );
-  }
-
   renderFontCSSCode(rule, ruleText) {
     if (!rule) {
       return null;
     }
 
     // Cut the rule text in 3 parts: the selector, the declarations, the closing brace.
     // This way we can collapse the declarations by default and display an expander icon
     // to expand them again.
@@ -84,24 +65,23 @@ class Font extends PureComponent {
 
     return dom.pre(
       {
         className: "font-css-code",
       },
       this.renderFontCSSCodeTwisty(),
       leading,
       isFontFaceRuleExpanded ?
-        null
-        :
+        body :
         dom.span(
           {
-            className: "font-css-code-expander"
+            className: "font-css-code-expander",
+            onClick: this.onFontFaceRuleToggle,
           }
         ),
-      isFontFaceRuleExpanded ? body : null,
       trailing
     );
   }
 
   renderFontTypeAndURL(url, format) {
     if (!url) {
       return dom.p(
         {
@@ -124,78 +104,59 @@ class Font extends PureComponent {
         format
       )
     );
   }
 
   renderFontName(name) {
     return dom.h1(
       {
-        className: "font-name",
-        onClick: this.onFontToggle,
+        className: "font-name"
       },
       name
     );
   }
 
-  renderFontTwisty() {
-    let { isFontExpanded } = this.state;
-    return this.renderTwisty(isFontExpanded, this.onFontToggle);
-  }
-
   renderFontCSSCodeTwisty() {
     let { isFontFaceRuleExpanded } = this.state;
-    return this.renderTwisty(isFontFaceRuleExpanded, this.onFontFaceRuleToggle);
-  }
 
-  renderTwisty(isExpanded, onClick) {
     let attributes = {
       className: "theme-twisty",
-      onClick,
+      onClick: this.onFontFaceRuleToggle,
     };
-    if (isExpanded) {
+    if (isFontFaceRuleExpanded) {
       attributes.open = "true";
     }
 
     return dom.span(attributes);
   }
 
   render() {
     let {
       font,
       fontOptions,
       onPreviewFonts,
     } = this.props;
 
     let { previewText } = fontOptions;
 
     let {
-      CSSFamilyName,
       format,
       name,
       previewUrl,
       rule,
       ruleText,
       URI,
     } = font;
 
-    let { isFontExpanded } = this.state;
-
     return dom.li(
       {
-        className: "font" + (isFontExpanded ? " expanded" : ""),
+        className: "font",
       },
-      this.renderFontTwisty(),
       this.renderFontName(name),
       FontPreview({ previewText, previewUrl, onPreviewFonts }),
-      dom.div(
-        {
-          className: "font-details"
-        },
-        this.renderFontTypeAndURL(URI, format),
-        this.renderFontCSSCode(rule, ruleText),
-        this.renderFontCSS(CSSFamilyName)
-      )
+      this.renderFontTypeAndURL(URI, format),
+      this.renderFontCSSCode(rule, ruleText)
     );
   }
 }
 
 module.exports = Font;
--- a/devtools/client/inspector/fonts/components/FontPreview.js
+++ b/devtools/client/inspector/fonts/components/FontPreview.js
@@ -4,16 +4,17 @@
 
 "use strict";
 
 const { PureComponent } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
 const Types = require("../types");
+const { getStr } = require("../utils/l10n");
 
 class FontPreview extends PureComponent {
   static get propTypes() {
     return {
       previewText: Types.fontOptions.previewText.isRequired,
       previewUrl: Types.font.previewUrl.isRequired,
       onPreviewFonts: PropTypes.func.isRequired,
     };
@@ -80,15 +81,16 @@ class FontPreview extends PureComponent 
         )
         :
         null,
       dom.img(
         {
           className: "font-preview",
           src: previewUrl,
           onClick: this.onClick,
+          title: !isFocused ? getStr("fontinspector.editPreview") : "",
         }
       )
     );
   }
 }
 
 module.exports = FontPreview;
--- a/devtools/client/inspector/fonts/test/browser.ini
+++ b/devtools/client/inspector/fonts/test/browser.ini
@@ -12,12 +12,11 @@ support-files =
   !/devtools/client/inspector/test/head.js
   !/devtools/client/inspector/test/shared-head.js
   !/devtools/client/shared/test/test-actor.js
   !/devtools/client/shared/test/test-actor-registry.js
 
 [browser_fontinspector.js]
 [browser_fontinspector_edit-previews.js]
 [browser_fontinspector_expand-css-code.js]
-[browser_fontinspector_expand-details.js]
 [browser_fontinspector_other-fonts.js]
 [browser_fontinspector_text-node.js]
 [browser_fontinspector_theme-change.js]
--- a/devtools/client/inspector/fonts/test/browser_fontinspector.js
+++ b/devtools/client/inspector/fonts/test/browser_fontinspector.js
@@ -50,53 +50,43 @@ function getFormat(fontLi) {
   let link = fontLi.querySelector(".font-format-url a");
   if (!link) {
     return null;
   }
 
   return link.textContent;
 }
 
-function getCSSName(fontLi) {
-  let text = fontLi.querySelector(".font-css-name").textContent;
-
-  return text.substring(text.indexOf('"') + 1, text.lastIndexOf('"'));
-}
-
 function* testBodyFonts(inspector, viewDoc) {
   let lis = getUsedFontsEls(viewDoc);
   is(lis.length, 5, "Found 5 fonts");
 
   for (let i = 0; i < FONTS.length; i++) {
     let li = lis[i];
     let font = FONTS[i];
 
     is(getName(li), font.name, "font " + i + " right font name");
     is(isRemote(li), font.remote, "font " + i + " remote value correct");
     is(li.querySelector(".font-url").href, font.url, "font " + i + " url correct");
     is(getFormat(li), font.format, "font " + i + " format correct");
-    is(getCSSName(li), font.cssName, "font " + i + " css name correct");
   }
 
   // test that the bold and regular fonts have different previews
   let regSrc = lis[0].querySelector(".font-preview").src;
   let boldSrc = lis[1].querySelector(".font-preview").src;
   isnot(regSrc, boldSrc, "preview for bold font is different from regular");
 
   // test system font
   let localFontName = getName(lis[4]);
-  let localFontCSSName = getCSSName(lis[4]);
 
   // On Linux test machines, the Arial font doesn't exist.
   // The fallback is "Liberation Sans"
   ok((localFontName == "Arial") || (localFontName == "Liberation Sans"),
      "local font right font name");
   ok(!isRemote(lis[4]), "local font is local");
-  ok((localFontCSSName == "Arial") || (localFontCSSName == "Liberation Sans"),
-     "Arial", "local font has right css name");
 }
 
 function* testDivFonts(inspector, viewDoc) {
   let updated = inspector.once("fontinspector-updated");
   yield selectNode("div", inspector);
   yield updated;
 
   let lis = getUsedFontsEls(viewDoc);
--- a/devtools/client/inspector/fonts/test/browser_fontinspector_expand-css-code.js
+++ b/devtools/client/inspector/fonts/test/browser_fontinspector_expand-css-code.js
@@ -7,30 +7,45 @@
 // Test that the font-face css rule code is collapsed by default, and can be expanded.
 
 const TEST_URI = URL_ROOT + "browser_fontinspector.html";
 
 add_task(function* () {
   let { view } = yield openFontInspectorForURL(TEST_URI);
   let viewDoc = view.document;
 
-  info("Expanding the details section of the first font");
+  info("Checking that the css font-face rule is collapsed by default");
   let fontEl = getUsedFontsEls(viewDoc)[0];
-  yield expandFontDetails(fontEl);
-
-  info("Checking that the css font-face rule is collapsed by default");
   let codeEl = fontEl.querySelector(".font-css-code");
   is(codeEl.textContent, "@font-face {}", "The font-face rule is collapsed");
 
   info("Expanding the rule by clicking on the expander icon");
   let onExpanded = BrowserTestUtils.waitForCondition(() => {
     return codeEl.textContent === `@font-face {
   font-family: "bar";
   src: url("bad/font/name.ttf"), url("ostrich-regular.ttf") format("truetype");
 }`;
   }, "Waiting for the font-face rule");
 
   let expander = fontEl.querySelector(".font-css-code .theme-twisty");
   expander.click();
   yield onExpanded;
 
   ok(true, "Font-face rule is now expanded");
+
+  info("Expanding another rule by clicking on the [...] icon instead");
+  fontEl = getUsedFontsEls(viewDoc)[1];
+  codeEl = fontEl.querySelector(".font-css-code");
+
+  onExpanded = BrowserTestUtils.waitForCondition(() => {
+    return codeEl.textContent === `@font-face {
+  font-family: "bar";
+  font-weight: bold;
+  src: url("ostrich-black.ttf");
+}`;
+  }, "Waiting for the font-face rule");
+
+  expander = fontEl.querySelector(".font-css-code .font-css-code-expander");
+  expander.click();
+  yield onExpanded;
+
+  ok(true, "Font-face rule is now expanded too");
 });
deleted file mode 100644
--- a/devtools/client/inspector/fonts/test/browser_fontinspector_expand-details.js
+++ /dev/null
@@ -1,38 +0,0 @@
-/* vim: set ts=2 et sw=2 tw=80: */
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-// Test that fonts are collapsed by default, and can be expanded.
-
-const TEST_URI = URL_ROOT + "browser_fontinspector.html";
-
-add_task(function* () {
-  let { inspector, view } = yield openFontInspectorForURL(TEST_URI);
-  let viewDoc = view.document;
-
-  info("Checking all font are collapsed by default");
-  let fonts = getUsedFontsEls(viewDoc);
-  checkAllFontsCollapsed(fonts);
-
-  info("Clicking on the first one to expand the font details");
-  yield expandFontDetails(fonts[0]);
-
-  ok(fonts[0].querySelector(".theme-twisty").hasAttribute("open"), `Twisty is open`);
-  ok(isFontDetailsVisible(fonts[0]), `Font details is shown`);
-
-  info("Selecting a node with different fonts and checking that all fonts are collapsed");
-  yield selectNode(".black-text", inspector);
-  fonts = getUsedFontsEls(viewDoc);
-  checkAllFontsCollapsed(fonts);
-});
-
-function checkAllFontsCollapsed(fonts) {
-  fonts.forEach((el, i) => {
-    let twisty = el.querySelector(".theme-twisty");
-    ok(twisty, `Twisty ${i} exists`);
-    ok(!twisty.hasAttribute("open"), `Twisty ${i} is closed`);
-    ok(!isFontDetailsVisible(el), `Font details ${i} is hidden`);
-  });
-}
--- a/devtools/client/inspector/fonts/test/head.js
+++ b/devtools/client/inspector/fonts/test/head.js
@@ -83,31 +83,16 @@ function* updatePreviewText(view, text) 
     let update = waitForNEvents(view.inspector, "fontinspector-updated", text.length);
     EventUtils.sendString(text, doc.defaultView);
     yield update;
   }
 
   is(input.value, text, "The input now contains the correct text.");
 }
 
-async function expandFontDetails(fontEl) {
-  info("Expanding a font details section");
-
-  let onExpanded = BrowserTestUtils.waitForCondition(() => isFontDetailsVisible(fontEl),
-                                                     "Waiting for font details");
-  let twisty = fontEl.querySelector(".theme-twisty");
-  twisty.click();
-  await onExpanded;
-}
-
-function isFontDetailsVisible(fontEl) {
-  return [...fontEl.querySelectorAll(".font-css-name, .font-css-code, .font-format-url")]
-         .every(el => el.getBoxQuads().length);
-}
-
 /**
  * Get all of the <li> elements for the fonts used on the currently selected element.
  *
  * @param  {document} viewDoc
  * @return {Array}
  */
 function getUsedFontsEls(viewDoc) {
   return viewDoc.querySelectorAll("#font-container > .fonts-list > li");
--- a/devtools/client/inspector/fonts/types.js
+++ b/devtools/client/inspector/fonts/types.js
@@ -5,19 +5,16 @@
 "use strict";
 
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 
 /**
  * A single font.
  */
 const font = exports.font = {
-  // The name of the font family
-  CSSFamilyName: PropTypes.string,
-
   // The format of the font
   format: PropTypes.string,
 
   // The name of the font
   name: PropTypes.string,
 
   // URL for the font preview
   previewUrl: PropTypes.string,
--- a/devtools/client/locales/en-US/font-inspector.properties
+++ b/devtools/client/locales/en-US/font-inspector.properties
@@ -1,26 +1,27 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 # LOCALIZATION NOTE This file contains the Font Inspector strings.
 # The Font Inspector is a panel accessible in the Inspector sidebar.
 
-# LOCALIZATION NOTE (fontinspector.usedAs) This label introduces the name used to refer to
-# the font in a stylesheet.
-fontinspector.usedAs=Used as:
-
 # LOCALIZATION NOTE (fontinspector.system) This label indicates that the font is a local
 # system font.
 fontinspector.system=system
 
 # LOCALIZATION NOTE (fontinspector.remote) This label indicates that the font is a remote
 # font.
 fontinspector.remote=remote
 
 # LOCALIZATION NOTE (fontinspector.noFontsOnSelectedElement): This label is shown when
 # no fonts found on the selected element.
 fontinspector.noFontsOnSelectedElement=No fonts were found for the current element.
 
 # LOCALIZATION NOTE (fontinspector.otherFontsInPageHeader): This is the text for the
 # header of a collapsible section containing other fonts used in the page.
-fontinspector.otherFontsInPageHeader=Other fonts in page
\ No newline at end of file
+fontinspector.otherFontsInPageHeader=Other fonts in page
+
+# LOCALIZATION NOTE (fontinspector.editPreview): This is the text that appears in a
+# tooltip on hover of a font preview string. Clicking on the string opens a text input
+# where users can type to change the preview text.
+fontinspector.editPreview=Click to edit preview
--- a/devtools/client/themes/fonts.css
+++ b/devtools/client/themes/fonts.css
@@ -20,32 +20,29 @@
   margin: 0;
   list-style: none;
 }
 
 .font {
   border: 1px solid var(--theme-splitter-color);
   border-width: 0 1px 1px 0;
   display: grid;
-  grid-template-columns: 14px auto 1fr;
-  grid-template-rows: 50px;
-  grid-column-gap: 10px;
-  padding: 0 10px 0 5px;
+  grid-template-columns: 1fr auto;
+  padding: 10px 20px;
 }
 
 #font-container .theme-twisty {
   display: inline-block;
   cursor: pointer;
-  place-self: center;
-  vertical-align: text-top;
+  vertical-align: bottom;
 }
 
 .font-preview-container {
-  grid-column: 3 / -1;
-  grid-row: 1;
+  grid-column: 2;
+  grid-row: 1 / span 2;
   overflow: hidden;
   display: grid;
   place-items: center end;
   position: relative;
 }
 
 .font-preview {
   height: 50px;
@@ -56,54 +53,49 @@
   cursor: text;
   background-image: linear-gradient(to right,
     var(--grey-40) 3px, transparent 3px, transparent);
   background-size: 6px 1px;
   background-repeat: repeat-x;
   background-position-y: 45px;
 }
 
-.font-preview-input {
+#font-container .font-preview-input {
   position: absolute;
-  top: 0;
+  top: 5px;
   left: 0;
   width: calc(100% - 5px);
-  height: calc(100% - 2px);
+  height: calc(100% - 10px);
   background: transparent;
   color: transparent;
+  border-radius: 0;
+  padding: 0;
 }
 
 .font-preview-input::-moz-selection {
   background: transparent;
   color: transparent;
 }
 
 .font-name {
   margin: 0;
-  font-size: 1em;
+  font-size: 1.2em;
+  font-weight: normal;
   white-space: nowrap;
-  grid-column: 2;
-  place-self: center start;
-}
-
-.font-details {
-  grid-column: 2 / 4;
-  padding-inline-end: 14px;
-  width: 100%;
 }
 
 .font-css-code {
   direction: ltr;
-  padding: 5px;
   margin: 0;
-  border: 1px solid var(--theme-splitter-color);
-  border-radius: 3px;
   overflow: hidden;
   text-overflow: ellipsis;
   color: var(--theme-toolbar-color);
+  grid-column: span 2;
+  position: relative;
+  offset-inline-start: -4px;
 }
 
 .font-css-code-expander::before {
   content: "\2026";
   display: inline-block;
   width: 12px;
   height: 8px;
   margin: 0 2px;
@@ -113,22 +105,22 @@
   border-style: solid;
   border-width: 1px;
   text-align: center;
   vertical-align: middle;
 }
 
 .font-format-url {
   text-transform: capitalize;
-  margin-block-start: 0;
+  margin-top: .2em;
+  color: var(--grey-50);
 }
 
 .font-url {
-  margin-inline-start: 1em;
-  text-transform: uppercase;
+  margin-inline-start: .5em;
   text-decoration: underline;
   color: var(--theme-highlight-blue);
   background: transparent;
   border: none;
   cursor: pointer;
 }
 
 .font-url::after {
@@ -140,14 +132,11 @@
   vertical-align: middle;
   background-image: url(chrome://devtools-shim/content/aboutdevtools/images/external-link.svg);
   background-repeat: no-repeat;
   background-size: 13px 13px;
   -moz-context-properties: fill;
   fill: var(--blue-60);
 }
 
-
-.font:not(.expanded) .font-css-name,
-.font:not(.expanded) .font-css-code,
-.font:not(.expanded) .font-format-url {
-  display: none;
+#font-container .devtools-sidepanel-no-result + .accordion {
+  border-block-start: 1px solid var(--theme-splitter-color);
 }