Bug 1518727 - Add a Learn More link in the getter invoke confirm dialog; r=Honza.
authorNicolas Chevobbe <nchevobbe@mozilla.com>
Fri, 18 Jan 2019 14:41:44 +0000
changeset 454451 c586ab9cae6d139d5ce403b56f980be711e8c73c
parent 454450 bf190444c5793c7fca517d5893b12f4e2d50efd2
child 454452 0e161fee4d4483d1deaad69db6cf5a5ea59287e9
push id35397
push useropoprus@mozilla.com
push dateSat, 19 Jan 2019 03:35:41 +0000
treeherdermozilla-central@57dc8bbbc38f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersHonza
bugs1518727
milestone66.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 1518727 - Add a Learn More link in the getter invoke confirm dialog; r=Honza. The Learn More link navigates the user to an MDN page explaining why we don't automatically invoke getters on behalf of the user. The link is opened either by clicking on it, or by pressing the "?" key. A test is added to ensure this works as expected. Differential Revision: https://phabricator.services.mozilla.com/D16545
devtools/client/locales/en-US/webconsole.properties
devtools/client/themes/tooltips.css
devtools/client/webconsole/components/ConfirmDialog.js
devtools/client/webconsole/test/mochitest/browser.ini
devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_getters_learn_more_link.js
devtools/client/webconsole/test/mochitest/browser_webconsole_network_messages_status_code.js
devtools/client/webconsole/test/mochitest/head.js
--- a/devtools/client/locales/en-US/webconsole.properties
+++ b/devtools/client/locales/en-US/webconsole.properties
@@ -334,12 +334,12 @@ webconsole.reverseSearch.result.nextButt
 # LOCALIZATION NOTE (webconsole.confirmDialog.getter.label)
 # Label used for the "invoke getter" confirm dialog that appears in the console when
 # a user tries to autocomplete a property with a getter.
 # Example: given the following object `x = {get y() {}}`, when the user types `x.y.`, it
 # would return "Invoke getter y to retrieve the property list?".
 # Parameters: %S is the name of the getter.
 webconsole.confirmDialog.getter.label=Invoke getter %S to retrieve the property list?
 
-# LOCALIZATION NOTE (webconsole.confirmDialog.getter.confirmButtonLabel)
-# Label used for the Confirm button in the "invoke getter" dialog that appears in the
+# LOCALIZATION NOTE (webconsole.confirmDialog.getter.invokeButtonLabel)
+# Label used for the confirm button in the "invoke getter" dialog that appears in the
 # console when a user tries to autocomplete a property with a getter.
-webconsole.confirmDialog.getter.confirmButtonLabel=Confirm
+webconsole.confirmDialog.getter.invokeButtonLabel=Invoke
--- a/devtools/client/themes/tooltips.css
+++ b/devtools/client/themes/tooltips.css
@@ -654,47 +654,63 @@
 .onboarding-close-button::before {
   background-image: url("chrome://devtools/skin/images/close.svg");
   margin: -6px 0 0 -6px;
 }
 
 /* Tooltip: Invoke getter confirm Tooltip */
 
 .invoke-confirm {
-  font-family: var(--monospace-font-family);
   color: var(--theme-popup-color);
   border: 1px solid rgba(0,0,0, 0.1);
-  max-width: 400px;
-}
-
-.invoke-confirm .tooltip-panel {
-  display: flex;
-  flex-direction: column;
+  max-width: 212px;
 }
 
 .invoke-confirm .confirm-label {
   margin: 0;
-  padding: 8px;
-  flex-grow: 1;
+  padding: 4px;
+  background-color: var(--theme-toolbar-background-alt);
 }
 
 .invoke-confirm .emphasized {
+  font-family: var(--monospace-font-family);
   font-weight: bold;
   overflow-wrap: break-word;
 }
 
 .invoke-confirm .confirm-button {
   background-color: var(--theme-selection-background);
   color: white;
   border: none;
-  padding: 8px;
+  padding: 6px;
   display: block;
   width: 100%;
   text-align: left;
-  font-family: var(--monospace-font-family);
 }
 
 /* The button already has a "selected" style, we can remove the focus rings. */
 .confirm-button:-moz-focusring,
 .confirm-button::-moz-focus-inner {
   outline: none;
   border: none;
 }
+
+.invoke-confirm .learn-more-link {
+  color: var(--theme-highlight-blue);
+  padding-inline-end: 4px;
+  display: flex;
+  align-items: center;
+  justify-content: end;
+  min-height: 20px;
+  cursor: pointer;
+}
+
+.invoke-confirm .learn-more-link::after {
+  content: "";
+  width: 14px;
+  height: 14px;
+  fill: currentColor;
+  -moz-context-properties: fill;
+  background-image: url(chrome://devtools/skin/images/help.svg);
+  background-size: contain;
+  background-repeat: no-repeat;
+  margin-inline-start: 4px;
+}
\ No newline at end of file
--- a/devtools/client/webconsole/components/ConfirmDialog.js
+++ b/devtools/client/webconsole/components/ConfirmDialog.js
@@ -13,16 +13,24 @@ loader.lazyRequireGetter(this, "createPo
 const { Component } = require("devtools/client/shared/vendor/react");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const { connect } = require("devtools/client/shared/vendor/react-redux");
 
 const {getAutocompleteState} = require("devtools/client/webconsole/selectors/autocomplete");
 const autocompleteActions = require("devtools/client/webconsole/actions/autocomplete");
 const { l10n } = require("devtools/client/webconsole/utils/messages");
 
+const utmParams = new URLSearchParams({
+  "utm_source": "mozilla",
+  "utm_medium": "devtools-webconsole",
+  "utm_campaign": "default",
+});
+const LEARN_MORE_URL =
+  `https://developer.mozilla.org/docs/Tools/Web_Console/Invoke_getters_from_autocomplete?${utmParams}`;
+
 class ConfirmDialog extends Component {
   static get propTypes() {
     return {
       // Console object.
       hud: PropTypes.object.isRequired,
       // Update autocomplete popup state.
       autocompleteUpdate: PropTypes.func.isRequired,
       autocompleteClear: PropTypes.func.isRequired,
@@ -35,16 +43,17 @@ class ConfirmDialog extends Component {
   constructor(props) {
     super(props);
 
     const { hud } = props;
     hud.confirmDialog = this;
 
     this.cancel = this.cancel.bind(this);
     this.confirm = this.confirm.bind(this);
+    this.onLearnMoreClick = this.onLearnMoreClick.bind(this);
   }
 
   componentDidMount() {
     const doc = this.props.hud.document;
     const toolbox = gDevTools.getToolbox(this.props.hud.owner.target);
     const tooltipDoc = toolbox ? toolbox.doc : doc;
     // The popup will be attached to the toolbox document or HUD document in the case
     // such as the browser console which doesn't have a toolbox.
@@ -65,16 +74,20 @@ class ConfirmDialog extends Component {
     }
   }
 
   componentDidThrow(e) {
     console.error("Error in ConfirmDialog", e);
     this.setState(state => ({...state, hasError: true}));
   }
 
+  onLearnMoreClick(e) {
+    this.props.serviceContainer.openLink(LEARN_MORE_URL, e);
+  }
+
   cancel() {
     this.tooltip.hide();
     this.props.autocompleteClear();
   }
 
   confirm() {
     this.tooltip.hide();
     this.props.autocompleteUpdate(this.props.getterPath);
@@ -91,16 +104,22 @@ class ConfirmDialog extends Component {
     const {getterPath} = this.props;
     const getterName = getterPath.join(".");
 
     // We deliberately use getStr, and not getFormatStr, because we want getterName to
     // be wrapped in its own span.
     const description = l10n.getStr("webconsole.confirmDialog.getter.label");
     const [descriptionPrefix, descriptionSuffix] = description.split("%S");
 
+    const learnMoreElement = dom.a({
+      className: "learn-more-link",
+      title: LEARN_MORE_URL.split("?")[0],
+      onClick: this.onLearnMoreClick,
+    }, l10n.getStr("webConsoleMoreInfoLabel"));
+
     return createPortal([
       dom.p({
         className: "confirm-label",
       },
         dom.span({}, descriptionPrefix),
         dom.span({className: "emphasized"}, getterName),
         dom.span({}, descriptionSuffix)
       ),
@@ -114,24 +133,30 @@ class ConfirmDialog extends Component {
             event.stopPropagation();
             return;
           }
 
           if (["Tab", "Enter", " "].includes(key)) {
             this.confirm();
             event.stopPropagation();
           }
+
+          if (key === "?") {
+            this.onLearnMoreClick();
+            event.stopPropagation();
+          }
         },
         // We can't use onClick because it would respond to Enter and Space keypress.
         // We don't want that because we have a Ctrl+Space shortcut to force an
         // autocomplete update; if the ConfirmDialog need to be displayed, since
         // we automatically focus the button, the keyup on space would fire the onClick
         // handler.
         onMouseDown: this.confirm,
-      }, l10n.getStr("webconsole.confirmDialog.getter.confirmButtonLabel")),
+      }, l10n.getStr("webconsole.confirmDialog.getter.invokeButtonLabel")),
+      learnMoreElement,
     ], this.tooltip.panel);
   }
 }
 
 // Redux connect
 function mapStateToProps(state) {
   const autocompleteData = getAutocompleteState(state);
   return {
--- a/devtools/client/webconsole/test/mochitest/browser.ini
+++ b/devtools/client/webconsole/test/mochitest/browser.ini
@@ -196,16 +196,17 @@ skip-if = verify
 [browser_jsterm_autocomplete_commands.js]
 [browser_jsterm_autocomplete_control_space.js]
 [browser_jsterm_autocomplete_crossdomain_iframe.js]
 [browser_jsterm_autocomplete_escape_key.js]
 [browser_jsterm_autocomplete_extraneous_closing_brackets.js]
 [browser_jsterm_autocomplete_getters_cache.js]
 [browser_jsterm_autocomplete_getters_cancel.js]
 [browser_jsterm_autocomplete_getters_confirm.js]
+[browser_jsterm_autocomplete_getters_learn_more_link.js]
 [browser_jsterm_autocomplete_helpers.js]
 [browser_jsterm_autocomplete_in_chrome_tab.js]
 [browser_jsterm_autocomplete_in_debugger_stackframe.js]
 [browser_jsterm_autocomplete_inside_text.js]
 [browser_jsterm_autocomplete_native_getters.js]
 [browser_jsterm_autocomplete_nav_and_tab_key.js]
 [browser_jsterm_autocomplete_paste_undo.js]
 [browser_jsterm_autocomplete_return_key_no_selection.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/webconsole/test/mochitest/browser_jsterm_autocomplete_getters_learn_more_link.js
@@ -0,0 +1,58 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that accessing properties with getters displays a "learn more" link in the confirm
+// dialog that navigates the user to the expected mdn page.
+
+const TEST_URI = `data:text/html;charset=utf-8,
+<head>
+  <script>
+    /* Create a prototype-less object so popup does not contain native
+     * Object prototype properties.
+     */
+    window.foo = Object.create(null, Object.getOwnPropertyDescriptors({
+      get bar() {
+        return "hello";
+      }
+    }));
+  </script>
+</head>
+<body>Autocomplete popup - invoke getter usage test</body>`;
+
+const MDN_URL =
+  "https://developer.mozilla.org/docs/Tools/Web_Console/Invoke_getters_from_autocomplete";
+
+add_task(async function() {
+  // Run test with legacy JsTerm
+  await pushPref("devtools.webconsole.jsterm.codeMirror", false);
+  await performTests();
+  // And then run it with the CodeMirror-powered one.
+  await pushPref("devtools.webconsole.jsterm.codeMirror", true);
+  await performTests();
+});
+
+async function performTests() {
+  const hud = await openNewTabAndConsole(TEST_URI);
+  const { jsterm } = hud;
+  const target = await TargetFactory.forTab(gBrowser.selectedTab);
+  const toolbox = gDevTools.getToolbox(target);
+
+  const tooltip = await setInputValueForGetterConfirmDialog(toolbox, jsterm,
+    "window.foo.bar.");
+  const labelEl = tooltip.querySelector(".confirm-label");
+  is(labelEl.textContent, "Invoke getter window.foo.bar to retrieve the property list?",
+    "Dialog has expected text content");
+  const learnMoreEl = tooltip.querySelector(".learn-more-link");
+  is(learnMoreEl.textContent, "Learn More", `There's a "Learn more" link`);
+
+  info(`Check that clicking on the "Learn more" link navigates to the expected page`);
+  const expectedUri = MDN_URL + GA_PARAMS;
+  let { link } = await simulateLinkClick(learnMoreEl);
+  is(link, expectedUri, `Click on "Learn More" link navigates user to ${expectedUri}`);
+
+  info(`Check that hitting "?" navigates to the Learn more target page`);
+  link = (await overrideOpenLink(() => EventUtils.synthesizeKey("?"))).link;
+  is(link, expectedUri, `Hitting "?" navigates user to ${expectedUri}`);
+}
--- a/devtools/client/webconsole/test/mochitest/browser_webconsole_network_messages_status_code.js
+++ b/devtools/client/webconsole/test/mochitest/browser_webconsole_network_messages_status_code.js
@@ -6,17 +6,17 @@
 const TEST_FILE = "test-network-request.html";
 const TEST_PATH = "http://example.com/browser/devtools/client/webconsole/" +
                   "test/mochitest/";
 const TEST_URI = TEST_PATH + TEST_FILE;
 
 const NET_PREF = "devtools.webconsole.filter.net";
 const XHR_PREF = "devtools.webconsole.filter.netxhr";
 const { l10n } = require("devtools/client/webconsole/utils/messages");
-const LEARN_MORE_URI = "https://developer.mozilla.org/docs/Web/HTTP/Status/200" + STATUS_CODES_GA_PARAMS;
+const LEARN_MORE_URI = "https://developer.mozilla.org/docs/Web/HTTP/Status/200" + GA_PARAMS;
 
 pushPref(NET_PREF, true);
 pushPref(XHR_PREF, true);
 
 add_task(async function task() {
   const hud = await openNewTabAndConsole(TEST_URI);
 
   const currentTab = gBrowser.selectedTab;
--- a/devtools/client/webconsole/test/mochitest/head.js
+++ b/devtools/client/webconsole/test/mochitest/head.js
@@ -27,17 +27,17 @@ Services.scriptloader.loadSubScript(
 
 var {HUDService} = require("devtools/client/webconsole/hudservice");
 var WCUL10n = require("devtools/client/webconsole/webconsole-l10n");
 const DOCS_GA_PARAMS = `?${new URLSearchParams({
   "utm_source": "mozilla",
   "utm_medium": "firefox-console-errors",
   "utm_campaign": "default",
 })}`;
-const STATUS_CODES_GA_PARAMS = `?${new URLSearchParams({
+const GA_PARAMS = `?${new URLSearchParams({
   "utm_source": "mozilla",
   "utm_medium": "devtools-webconsole",
   "utm_campaign": "default",
 })}`;
 
 const wcActions = require("devtools/client/webconsole/actions/index");
 
 registerCleanupFunction(async function() {
@@ -655,36 +655,53 @@ async function closeConsole(tab = gBrows
  *          A Promise that is resolved when the link click simulation occured or
  *          when the click is not dispatched.
  *          The promise resolves with an object that holds the following properties
  *          - link: url of the link or null(if event not fired)
  *          - where: "tab" if tab is active or "tabshifted" if tab is inactive
  *            or null(if event not fired)
  */
 function simulateLinkClick(element, clickEventProps) {
+  return overrideOpenLink(() => {
+    if (clickEventProps) {
+      // Click on the link using the event properties.
+      element.dispatchEvent(clickEventProps);
+    } else {
+      // Click on the link.
+      element.click();
+    }
+  });
+}
+
+/**
+ * Override the browserWindow open*Link function, executes the passed function and either
+ * wait for:
+ * - the link to be "opened"
+ * - 1s before timing out
+ * Then it puts back the original open*Link functions in browserWindow.
+ *
+ * @returns {Promise<Object>}: A promise resolving with an object of the following shape:
+ * - link: The link that was "opened"
+ * - where: If the link was opened in the background (null) or not ("tab").
+ */
+function overrideOpenLink(fn) {
   const browserWindow = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
 
   // Override LinkIn methods to prevent navigating.
   const oldOpenTrustedLinkIn = browserWindow.openTrustedLinkIn;
   const oldOpenWebLinkIn = browserWindow.openWebLinkIn;
 
   const onOpenLink = new Promise((resolve) => {
     const openLinkIn = function(link, where) {
       browserWindow.openTrustedLinkIn = oldOpenTrustedLinkIn;
       browserWindow.openWebLinkIn = oldOpenWebLinkIn;
       resolve({link: link, where});
     };
     browserWindow.openWebLinkIn = browserWindow.openTrustedLinkIn = openLinkIn;
-    if (clickEventProps) {
-      // Click on the link using the event properties.
-      element.dispatchEvent(clickEventProps);
-    } else {
-      // Click on the link.
-      element.click();
-    }
+    fn();
   });
 
   // Declare a timeout Promise that we can use to make sure openTrustedLinkIn or
   // openWebLinkIn was not called.
   let timeoutId;
   const onTimeout = new Promise(function(resolve) {
     timeoutId = setTimeout(() => {
       browserWindow.openTrustedLinkIn = oldOpenTrustedLinkIn;