Bug 1520449 - Do the search hand-off on keydown instead of mouse click (#4650)

common/Actions.jsm

@@ -49,7 +49,6 @@ for (const type of [
   "DISCOVERY_STREAM_SPOCS_UPDATE",
   "DOWNLOAD_CHANGED",
   "FILL_SEARCH_TERM",
-  "FOCUS_SEARCH",
   "HANDOFF_SEARCH_TO_AWESOMEBAR",
   "HIDE_SEARCH",
   "INIT",

common/Reducers.jsm

@@ -491,10 +491,8 @@ function Search(prevState = INITIAL_STATE.Search, action) {
   switch (action.type) {
     case at.HIDE_SEARCH:
       return Object.assign({...prevState, hide: true});
-    case at.FOCUS_SEARCH:
-      return Object.assign({...prevState, focus: true});
     case at.SHOW_SEARCH:
-      return Object.assign({...prevState, hide: false, focus: false});
+      return Object.assign({...prevState, hide: false});
     default:
       return prevState;
   }

content-src/components/Search/Search.jsx

@@ -31,17 +31,19 @@ export class _Search extends React.PureComponent {
 
   onSearchHandoffClick(event) {
     // When search hand-off is enabled, we render a big button that is styled to
-    // look like a search textbox. If the button is clicked with the mouse, we style
-    // the button as if it was a focused search box and show a fake cursor but
-    // really focus the awesomebar without the focus styles.
+    // look like a search textbox. If the button is clicked with the mouse, we
+    // focus it. If the user types, transfer focus to awesomebar.
     // If the button is clicked from the keyboard, we focus the awesomebar normally.
     // This is to minimize confusion with users navigating with the keyboard and
     // users using assistive technologoy.
+    event.preventDefault();
     const isKeyboardClick = event.clientX === 0 && event.clientY === 0;
-    const hiddenFocus =  !isKeyboardClick;
-    this.props.dispatch(ac.OnlyToMain({type: at.HANDOFF_SEARCH_TO_AWESOMEBAR, data: {hiddenFocus}}));
-    this.props.dispatch({type: at.FOCUS_SEARCH});
-    this.props.dispatch(ac.UserEvent({event: "SEARCH_HANDOFF"}));
+    if (isKeyboardClick) {
+      this.props.dispatch(ac.OnlyToMain({type: at.HANDOFF_SEARCH_TO_AWESOMEBAR}));
+      this.props.dispatch(ac.UserEvent({event: "SEARCH_HANDOFF"}));
+    } else {
+      this._searchHandoffButton.focus();
+    }
   }
 
   onSearchHandoffKeyDown(event) {
@@ -49,6 +51,7 @@ export class _Search extends React.PureComponent {
       // We only care about key strokes that will produce a character.
       const text = event.key;
       this.props.dispatch(ac.OnlyToMain({type: at.HANDOFF_SEARCH_TO_AWESOMEBAR, data: {text}}));
+      this.props.dispatch({type: at.HIDE_SEARCH});
       this.props.dispatch(ac.UserEvent({event: "SEARCH_HANDOFF"}));
     }
   }
@@ -63,6 +66,7 @@ export class _Search extends React.PureComponent {
     event.preventDefault();
     const text = event.clipboardData.getData("Text");
     this.props.dispatch(ac.OnlyToMain({type: at.HANDOFF_SEARCH_TO_AWESOMEBAR, data: {text}}));
+    this.props.dispatch({type: at.HIDE_SEARCH});
     this.props.dispatch(ac.UserEvent({event: "SEARCH_HANDOFF"}));
   }
 
@@ -124,7 +128,6 @@ export class _Search extends React.PureComponent {
     const wrapperClassName = [
       "search-wrapper",
       this.props.hide && "search-hidden",
-      this.props.focus && "search-active",
     ].filter(v => v).join(" ");
 
     return (<div className={wrapperClassName}>

content-src/components/Search/_Search.scss

@@ -160,8 +160,7 @@ $glyph-forward: url('chrome://browser/skin/forward.svg');
     box-shadow: $shadow-secondary, 0 0 0 1px $black-25;
   }
 
-  &:focus,
-  .search-active & {
+  &:focus {
     border: $input-border-active;
     box-shadow: var(--newtab-textbox-focus-boxshadow);
 

lib/PlacesFeed.jsm

@@ -286,42 +286,22 @@ class PlacesFeed {
   handoffSearchToAwesomebar({_target, data, meta}) {
     const urlBar = _target.browser.ownerGlobal.gURLBar;
 
-    if (!data.hiddenFocus && !data.text) {
-      // Do a normal focus of awesomebar and reset the in content search (remove fake focus styles).
+    if (!data.text) {
+      // Do a normal focus of awesomebar.
       urlBar.focus();
-      this.store.dispatch(ac.OnlyToOneContent({type: at.SHOW_SEARCH}, meta.fromTarget));
       // We are done here. return early.
       return;
     }
 
-    if (data.text) {
-      // Pass the provided text to the awesomebar.
-      urlBar.search(data.text);
-      this.store.dispatch(ac.OnlyToOneContent({type: at.HIDE_SEARCH}, meta.fromTarget));
-    } else {
-      // Focus the awesomebar without the style changes.
-      urlBar.hiddenFocus();
-    }
+    // Pass the provided text to the awesomebar.
+    urlBar.search(data.text);
 
-    const onKeydown = event => {
-      // We only care about key strokes that will produce a character.
-      if (event.key.length === 1 && !event.altKey && !event.ctrlKey && !event.metaKey) {
-        // Once the user starts typing, we want to hide the in content search box
-        // and show the focus styles on the awesomebar.
-        this.store.dispatch(ac.OnlyToOneContent({type: at.HIDE_SEARCH}, meta.fromTarget));
-        urlBar.removeHiddenFocus();
-        urlBar.removeEventListener("keydown", onKeydown);
-      }
-    };
     const onDone = () => {
       // When done, let's cleanup everything.
       this.store.dispatch(ac.OnlyToOneContent({type: at.SHOW_SEARCH}, meta.fromTarget));
-      urlBar.removeHiddenFocus();
-      urlBar.removeEventListener("keydown", onKeydown);
       urlBar.removeEventListener("mousedown", onDone);
       urlBar.removeEventListener("blur", onDone);
     };
-    urlBar.addEventListener("keydown", onKeydown);
     urlBar.addEventListener("mousedown", onDone);
     urlBar.addEventListener("blur", onDone);
   }

test/unit/common/Reducers.test.js

@@ -698,13 +698,8 @@ describe("Reducers", () => {
       const nextState = Search(undefined, {type: "HIDE_SEARCH"});
       assert.propertyVal(nextState, "hide", true);
     });
-    it("should set focus to true on FOCUS_SEARCH", () => {
-      const nextState = Search(undefined, {type: "FOCUS_SEARCH"});
-      assert.propertyVal(nextState, "focus", true);
-    });
     it("should set focus and hide to false on SHOW_SEARCH", () => {
       const nextState = Search(undefined, {type: "SHOW_SEARCH"});
-      assert.propertyVal(nextState, "focus", false);
       assert.propertyVal(nextState, "hide", false);
     });
   });

test/unit/content-src/components/Search.test.jsx

@@ -80,33 +80,23 @@ describe("<Search>", () => {
       assert.ok(wrapper.exists());
       assert.equal(wrapper.find(".search-handoff-button").length, 1);
     });
-    it("should hand-off search when button is clicked with mouse", () => {
+    it("should focus search hand-off button when clicked with mouse", () => {
       const dispatch = sinon.spy();
       const wrapper = shallowWithIntl(<Search {...DEFAULT_PROPS} handoffEnabled={true} dispatch={dispatch} />);
-      wrapper.find(".search-handoff-button").simulate("click", {clientX: 101, clientY: 102});
-      assert.calledThrice(dispatch);
-      assert.calledWith(dispatch, {
-        data: {hiddenFocus: true},
-        meta: {from: "ActivityStream:Content", skipLocal: true, to: "ActivityStream:Main"},
-        type: "HANDOFF_SEARCH_TO_AWESOMEBAR",
-      });
-      assert.calledWith(dispatch, {type: "FOCUS_SEARCH"});
-      const [action] = dispatch.thirdCall.args;
-      assert.isUserEventAction(action);
-      assert.propertyVal(action.data, "event", "SEARCH_HANDOFF");
+      wrapper.instance()._searchHandoffButton = {focus: sinon.spy()};
+      wrapper.find(".search-handoff-button").simulate("click", {clientX: 101, clientY: 102, preventDefault: () => {}});
+      assert.calledOnce(wrapper.instance()._searchHandoffButton.focus);
     });
     it("should hand-off search when button is clicked with keyboard", () => {
       const dispatch = sinon.spy();
       const wrapper = shallowWithIntl(<Search {...DEFAULT_PROPS} handoffEnabled={true} dispatch={dispatch} />);
-      wrapper.find(".search-handoff-button").simulate("click", {clientX: 0, clientY: 0});
-      assert.calledThrice(dispatch);
+      wrapper.find(".search-handoff-button").simulate("click", {clientX: 0, clientY: 0, preventDefault: () => {}});
+      assert.calledTwice(dispatch);
       assert.calledWith(dispatch, {
-        data: {hiddenFocus: false},
         meta: {from: "ActivityStream:Content", skipLocal: true, to: "ActivityStream:Main"},
         type: "HANDOFF_SEARCH_TO_AWESOMEBAR",
       });
-      assert.calledWith(dispatch, {type: "FOCUS_SEARCH"});
-      const [action] = dispatch.thirdCall.args;
+      const [action] = dispatch.secondCall.args;
       assert.isUserEventAction(action);
       assert.propertyVal(action.data, "event", "SEARCH_HANDOFF");
     });
@@ -114,13 +104,14 @@ describe("<Search>", () => {
       const dispatch = sinon.spy();
       const wrapper = shallowWithIntl(<Search {...DEFAULT_PROPS} handoffEnabled={true} dispatch={dispatch} />);
       wrapper.find(".search-handoff-button").simulate("keydown", {key: "f"});
-      assert.calledTwice(dispatch);
+      assert.calledThrice(dispatch);
       assert.calledWith(dispatch, {
         data: {text: "f"},
         meta: {from: "ActivityStream:Content", skipLocal: true, to: "ActivityStream:Main"},
         type: "HANDOFF_SEARCH_TO_AWESOMEBAR",
       });
-      const [action] = dispatch.secondCall.args;
+      assert.calledWith(dispatch, {type: "HIDE_SEARCH"});
+      const [action] = dispatch.thirdCall.args;
       assert.isUserEventAction(action);
       assert.propertyVal(action.data, "event", "SEARCH_HANDOFF");
     });
@@ -152,13 +143,14 @@ describe("<Search>", () => {
         },
         preventDefault: () => {},
       });
-      assert.calledTwice(dispatch);
+      assert.calledThrice(dispatch);
       assert.calledWith(dispatch, {
         data: {text: "some copied text"},
         meta: {from: "ActivityStream:Content", skipLocal: true, to: "ActivityStream:Main"},
         type: "HANDOFF_SEARCH_TO_AWESOMEBAR",
       });
-      const [action] = dispatch.secondCall.args;
+      assert.calledWith(dispatch, {type: "HIDE_SEARCH"});
+      const [action] = dispatch.thirdCall.args;
       assert.isUserEventAction(action);
       assert.propertyVal(action.data, "event", "SEARCH_HANDOFF");
     });

test/unit/lib/PlacesFeed.test.js

@@ -336,77 +336,14 @@ describe("PlacesFeed", () => {
       };
       listeners = {};
     });
-    it("should properly handle hiddenFocus=false", () => {
+    it("should properly handle normal focus (no text passed in)", () => {
       feed.handoffSearchToAwesomebar({
         _target: {browser: {ownerGlobal: {gURLBar: fakeUrlBar}}},
-        data: {hiddenFocus: false},
+        data: {},
         meta: {fromTarget: {}},
       });
       assert.calledOnce(fakeUrlBar.focus);
-      assert.notCalled(fakeUrlBar.hiddenFocus);
-      assert.calledOnce(feed.store.dispatch);
-      assert.calledWith(feed.store.dispatch, {
-        meta: {
-          from: "ActivityStream:Main",
-          skipMain: true,
-          to: "ActivityStream:Content",
-          toTarget: {},
-        },
-        type: "SHOW_SEARCH",
-      });
-    });
-    it("should properly handle hiddenFocus=true", () => {
-      feed.handoffSearchToAwesomebar({
-        _target: {browser: {ownerGlobal: {gURLBar: fakeUrlBar}}},
-        data: {hiddenFocus: true},
-        meta: {fromTarget: {}},
-      });
-      assert.calledOnce(fakeUrlBar.hiddenFocus);
-      assert.notCalled(fakeUrlBar.focus);
       assert.notCalled(feed.store.dispatch);
-
-      // Now call keydown listener with "Ctrl".
-      feed.store.dispatch.resetHistory();
-      listeners.keydown({key: "Ctrl"});
-      assert.notCalled(fakeUrlBar.removeHiddenFocus);
-      assert.notCalled(feed.store.dispatch);
-
-      // Now call keydown listener with "Ctrl+f".
-      feed.store.dispatch.resetHistory();
-      listeners.keydown({key: "f", ctrlKey: true});
-      assert.notCalled(fakeUrlBar.removeHiddenFocus);
-      assert.notCalled(feed.store.dispatch);
-
-      // Now call keydown listener with "f".
-      feed.store.dispatch.resetHistory();
-      listeners.keydown({key: "f"});
-      assert.calledOnce(fakeUrlBar.removeHiddenFocus);
-      assert.calledOnce(feed.store.dispatch);
-      assert.calledWith(feed.store.dispatch, {
-        meta: {
-          from: "ActivityStream:Main",
-          skipMain: true,
-          to: "ActivityStream:Content",
-          toTarget: {},
-        },
-        type: "HIDE_SEARCH",
-      });
-
-      // And then call blur listener.
-      fakeUrlBar.removeHiddenFocus.resetHistory();
-      feed.store.dispatch.resetHistory();
-      listeners.blur();
-      assert.calledOnce(fakeUrlBar.removeHiddenFocus);
-      assert.calledOnce(feed.store.dispatch);
-      assert.calledWith(feed.store.dispatch, {
-        meta: {
-          from: "ActivityStream:Main",
-          skipMain: true,
-          to: "ActivityStream:Content",
-          toTarget: {},
-        },
-        type: "SHOW_SEARCH",
-      });
     });
     it("should properly handle text data passed in", () => {
       feed.handoffSearchToAwesomebar({
@@ -416,8 +353,10 @@ describe("PlacesFeed", () => {
       });
       assert.calledOnce(fakeUrlBar.search);
       assert.calledWith(fakeUrlBar.search, "f");
-      assert.notCalled(fakeUrlBar.hiddenFocus);
       assert.notCalled(fakeUrlBar.focus);
+
+      // Now call blur listener.
+      listeners.blur();
       assert.calledOnce(feed.store.dispatch);
       assert.calledWith(feed.store.dispatch, {
         meta: {
@@ -426,7 +365,7 @@ describe("PlacesFeed", () => {
           to: "ActivityStream:Content",
           toTarget: {},
         },
-        type: "HIDE_SEARCH",
+        type: "SHOW_SEARCH",
       });
     });
   });