fix(composer-focus): Fix focus behavior + update InjectedComponent props

Summary:
- Fixes bug with Composer focus caused by injected component. The composer body
  was being focused, but the cursor remained at the beginning of the content
  instead of at the end. This was caused because the focus method was being
  called before the content had actually been rendered to the dom.
  - Adds a callback to check when injected comp was actually rendered, and uses
    that to focus the body at the correct time.
  - Updates specs
- Updates behavior of focusing composer body when selecting threads in split
  mode --  resolves #T3444
  - It will focus the body when a thread is selcted via a click
  - It wont focus the body when a thread is selected via arrow keys
  - It will focus the body when a new inline reply is created
  - Updates specs

Test Plan: - Unit tests

Reviewers: evan, bengotow

Reviewed By: bengotow

Differential Revision: https://phab.nylas.com/D2393

internal_packages/composer/lib/composer-view.cjsx

@@ -12,6 +12,7 @@ React = require 'react'
  FileUploadStore,
  QuotedHTMLTransformer,
  FileDownloadStore,
+ FocusedContentStore,
  ExtensionRegistry} = require 'nylas-exports'
 
 {DropZone,
@@ -89,7 +90,7 @@ class ComposerView extends React.Component
     @_deleteDraftIfEmpty()
     usub() for usub in @_usubs
 
-  componentDidUpdate: =>
+  componentDidUpdate: (prevProps, prevState) =>
     # We want to use a temporary variable instead of putting this into the
     # state. This is because the selection is a transient property that
     # only needs to be applied once. It's not a long-living property of
@@ -97,6 +98,14 @@ class ComposerView extends React.Component
     # re-rendering.
     @_recoveredSelection = null if @_recoveredSelection?
 
+    # If the body changed, let's wait for the editor body to actually get rendered
+    # into the dom before applying focus.
+    # Since the editor is an InjectedComponent, when this function gets called
+    # the editor hasn't actually finished rendering, so we need to wait for that
+    # to happen by using the InjectedComponent's `onComponentDidRender` callback.
+    # See `_renderEditor`
+    bodyChanged = @state.body isnt prevState.body
+    return if bodyChanged
     @_applyFieldFocus()
 
   _keymapHandlers: ->
@@ -112,7 +121,7 @@ class ComposerView extends React.Component
     "composer:undo": @undo
     "composer:redo": @redo
 
-  _applyFieldFocus: ->
+  _applyFieldFocus: =>
     if @state.focusedField and @_lastFocusedField isnt @state.focusedField
       @_lastFocusedField = @state.focusedField
       return unless @refs[@state.focusedField]
@@ -121,8 +130,8 @@ class ComposerView extends React.Component
       else
         React.findDOMNode(@refs[@state.focusedField]).focus()
 
-      if @state.focusedField is Fields.Body and not @_proxy.draftPristineBody()
-        @refs[Fields.Body].focusEditor()
+      if @state.focusedField is Fields.Body
+        @_focusEditor()
 
   componentWillReceiveProps: (newProps) =>
     @_ignoreNextTrigger = false
@@ -324,6 +333,7 @@ class ComposerView extends React.Component
       ref={Fields.Body}
       matching={role: "Composer:Editor"}
       fallback={ComposerEditor}
+      onComponentDidRender={@_onEditorBodyDidRender}
       requiredMethods={[
         'focusEditor'
         'getCurrentSelection'
@@ -332,6 +342,12 @@ class ComposerView extends React.Component
       ]}
       exposedProps={exposedProps} />
 
+  _focusEditor: =>
+    @refs[Fields.Body].focusEditor()
+
+  _onEditorBodyDidRender: =>
+    @_applyFieldFocus()
+
   _onEditorFocus: =>
     @setState(focusedField: Fields.Body)
 
@@ -487,7 +503,7 @@ class ComposerView extends React.Component
 
   _onMouseUpComposerBody: (event) =>
     if event.target is @_mouseDownTarget
-      @refs[Fields.Body].focusEditor()
+      @_focusEditor()
     @_mouseDownTarget = null
 
   # When a user focuses the composer, it's possible that no input is
@@ -496,7 +512,7 @@ class ComposerView extends React.Component
   # erroneously trigger keyboard shortcuts.
   _onFocusIn: (event) =>
     return if DOMUtils.closest(event.target, DOMUtils.inputTypes())
-    @refs[Fields.Body].focusEditor()
+    @_focusEditor()
 
   _onMouseMoveComposeBody: (event) =>
     if @_mouseComposeBody is "down" then @_mouseComposeBody = "move"
@@ -534,7 +550,10 @@ class ComposerView extends React.Component
   _initiallyFocusedField: (draft) ->
     return Fields.To if draft.to.length is 0
     return Fields.Subject if (draft.subject ? "").trim().length is 0
-    return Fields.Body
+
+    shouldFocusBody = @props.mode isnt 'inline' or
+      (FocusedContentStore.didFocusUsingClick('thread') is true)
+    return Fields.Body if shouldFocusBody
 
   _verifyEnabledFields: (draft, state) ->
     enabledFields = @state.enabledFields.concat(state.enabledFields)

internal_packages/composer/spec/composer-view-spec.cjsx

@@ -15,6 +15,7 @@ ReactTestUtils = React.addons.TestUtils
  AccountStore,
  FileUploadStore,
  ContactStore,
+ FocusedContentStore,
  ComponentRegistry} = require "nylas-exports"
 
 {InjectedComponent} = require 'nylas-component-kit'
@@ -136,9 +137,9 @@ describe "ComposerView", ->
       body: "Hello <b>World</b><br/> This is a test"
       replyToMessageId: null
 
-  makeComposer = ->
+  makeComposer = (props={}) ->
     @composer = NylasTestUtils.renderIntoDocument(
-      <ComposerView draftClientId={DRAFT_CLIENT_ID} />
+      <ComposerView draftClientId={DRAFT_CLIENT_ID} {...props} />
     )
 
   describe "populated composer", ->
@@ -257,11 +258,23 @@ describe "ComposerView", ->
         makeComposer.call @
         expect(@composer.state.focusedField).toBe Fields.Subject
 
-      it "focuses the body otherwise", ->
+      it "focuses the body if the composer is not inline", ->
         useDraft.call @, to: [u1], subject: "Yo"
-        makeComposer.call @
+        makeComposer.call @, {mode: 'fullWindow'}
         expect(@composer.state.focusedField).toBe Fields.Body
 
+      it "focuses the body if the composer is inline and the thread was focused via a click", ->
+        spyOn(FocusedContentStore, 'didFocusUsingClick').andReturn true
+        useDraft.call @, to: [u1], subject: "Yo"
+        makeComposer.call @, {mode: 'inline'}
+        expect(@composer.state.focusedField).toBe Fields.Body
+
+      it "does not focus any field if the composer is inline and the thread was not focused via a click", ->
+        spyOn(FocusedContentStore, 'didFocusUsingClick').andReturn false
+        useDraft.call @, to: [u1], subject: "Yo"
+        makeComposer.call @, {mode: 'inline'}
+        expect(@composer.state.focusedField).toBeUndefined()
+
     describe "when deciding whether or not to enable the subject", ->
       it "enables the subject when the subject is empty", ->
         useDraft.call @, subject: ""
@@ -355,27 +368,35 @@ describe "ComposerView", ->
         makeComposer.call(@)
         @composer.setState focusedField: Fields.Cc
         @body = @composer.refs[Fields.Body]
-        spyOn(@body, "focus")
         spyOn(React, "findDOMNode").andCallThrough()
+        spyOn(@composer, "_focusEditor")
         spyOn(@composer, "_applyFieldFocus").andCallThrough()
+        spyOn(@composer, "_onEditorBodyDidRender").andCallThrough()
+
+      it "does not apply focus if the focused field hasn't changed", ->
+        @composer._lastFocusedField = Fields.Body
+        @composer.setState focusedField: Fields.Body
+        expect(@composer._focusEditor).not.toHaveBeenCalled()
+        @composer._lastFocusedField = null
 
       it "can focus on the subject", ->
         @composer.setState focusedField: Fields.Subject
-        expect(@composer._applyFieldFocus.calls.length).toBe 1
+        expect(@composer._applyFieldFocus.calls.length).toBe 2
         expect(React.findDOMNode).toHaveBeenCalled()
         calls = _.filter React.findDOMNode.calls, (call) ->
           call.args[0].props.name == "subject"
         expect(calls.length).toBe 1
 
-      it "can focus on the body", ->
-        @composer.setState focusedField: Fields.Body
-        expect(@body.focus).toHaveBeenCalled()
-        expect(@body.focus.calls.length).toBe 1
+      it "focuses the body when the body changes only after it has been rendered", ->
+        @composer.setState focusedField: Fields.Body, body: 'new body'
+        expect(@composer._onEditorBodyDidRender).toHaveBeenCalled()
+        expect(@composer._applyFieldFocus.calls.length).toEqual 1
+        expect(@composer._focusEditor.calls.length).toBe 1
 
       it "ignores focuses to participant fields", ->
         @composer.setState focusedField: Fields.To
-        expect(@body.focus).not.toHaveBeenCalled()
-        expect(@composer._applyFieldFocus.calls.length).toBe 1
+        expect(@composer._focusEditor).not.toHaveBeenCalled()
+        expect(@composer._applyFieldFocus.calls.length).toBe 2
 
     describe "when participants are added during a draft update", ->
       it "shows the cc fields and bcc fields to ensure participants are never hidden", ->

spec/components/injected-component-set-spec.jsx

@@ -0,0 +1,46 @@
+import {React, ComponentRegistry, NylasTestUtils} from 'nylas-exports';
+import {InjectedComponentSet} from 'nylas-component-kit';
+const {renderIntoDocument} = NylasTestUtils;
+
+const reactStub = (displayName)=> {
+  return React.createClass({
+    displayName,
+    render() { return <div className={displayName}></div>; },
+  });
+};
+
+
+describe('InjectedComponentSet', ()=> {
+  describe('render', ()=> {
+    beforeEach(()=> {
+      const components = [reactStub('comp1'), reactStub('comp2')];
+      spyOn(ComponentRegistry, 'findComponentsMatching').andReturn(components);
+    });
+
+    it('calls `onComponentsDidRender` when all child comps have actually been rendered to the dom', ()=> {
+      let rendered;
+      const onComponentsDidRender = ()=> {
+        rendered = true;
+      };
+      runs(()=> {
+        renderIntoDocument(
+          <InjectedComponentSet
+            matching={{}}
+            onComponentsDidRender={onComponentsDidRender} />
+        );
+      });
+
+      waitsFor(
+        ()=> { return rendered; },
+        '`onComponentsDidMount` should be called',
+        100
+      );
+
+      runs(()=> {
+        expect(rendered).toBe(true);
+        expect(document.querySelectorAll('.comp1').length).toEqual(1);
+        expect(document.querySelectorAll('.comp2').length).toEqual(1);
+      });
+    });
+  });
+});

spec/components/multiselect-split-interaction-handler-spec.coffee

@@ -54,9 +54,13 @@ describe "MultiselectSplitInteractionHandler", ->
     expect(@handler.shouldShowKeyboardCursor()).toEqual(true)
 
   describe "onClick", ->
-    it "should focus list items", ->
+    it "should focus the list item and indicate it was focused via click", ->
       @handler.onClick(@item)
-      expect(Actions.setFocus).toHaveBeenCalledWith({collection: @collection, item: @item})
+      expect(Actions.setFocus).toHaveBeenCalledWith({
+        collection: @collection
+        item: @item
+        usingClick: true
+      })
 
   describe "onMetaClick", ->
     describe "when there is currently a focused item", ->

src/components/injected-component-set.cjsx

@@ -43,6 +43,8 @@ class InjectedComponentSet extends React.Component
    - `className` (optional) A {String} class name for the containing element.
    - `children` (optional) Any React elements rendered inside the InjectedComponentSet
       will always be displayed.
+   - `onComponentsDidRender` Callback that will be called when the injected component set
+      is successfully rendered onto the DOM.
    - `exposedProps` (optional) An {Object} with props that will be passed to each
       item rendered into the set.
    - `containersRequired` (optional). Pass false to optionally remove the containers
@@ -57,18 +59,21 @@ class InjectedComponentSet extends React.Component
     className: React.PropTypes.string
     exposedProps: React.PropTypes.object
     containersRequired: React.PropTypes.bool
-    requiredMethods: React.PropTypes.arrayOf(React.PropTypes.string)
+    onComponentsDidRender: React.PropTypes.func
 
   @defaultProps:
     direction: 'row'
     containersRequired: true
+    onComponentsDidRender: ->
 
   constructor: (@props) ->
     @state = @_getStateFromStores()
+    @_renderedComponents = new Set()
 
   componentDidMount: =>
     @_componentUnlistener = ComponentRegistry.listen =>
       @setState(@_getStateFromStores())
+    @props.onComponentsDidRender() if @props.containersRequired is false
 
   componentWillUnmount: =>
     @_componentUnlistener() if @_componentUnlistener
@@ -77,16 +82,26 @@ class InjectedComponentSet extends React.Component
     if newProps.location isnt @props?.location
       @setState(@_getStateFromStores(newProps))
 
+  componentDidUpdate: =>
+    @props.onComponentsDidRender() if @props.containersRequired is false
+
   render: =>
+    @_renderedComponents = new Set()
     flexboxProps = _.omit(@props, _.keys(@constructor.propTypes))
     flexboxClassName = @props.className ? ""
     exposedProps = @props.exposedProps ? {}
 
     elements = @state.components.map (component) =>
       if @props.containersRequired is false or component.containerRequired is false
-        <component key={component.displayName} {...exposedProps} />
+        return <component key={component.displayName} {...exposedProps} />
       else
-        <UnsafeComponent component={component} key={component.displayName} {...exposedProps} />
+        return (
+          <UnsafeComponent
+            key={component.displayName}
+            component={component}
+            onComponentDidRender={@_onComponentDidRender.bind(@, component.displayName)}
+            {...exposedProps} />
+        )
 
 
     if @state.visible
@@ -95,10 +110,15 @@ class InjectedComponentSet extends React.Component
       elements.push(<span key="_clear" style={clear:'both'}/>)
 
     <Flexbox className={flexboxClassName} {...flexboxProps}>
-        {elements}
-        {@props.children ? []}
+      {elements}
+      {@props.children ? []}
     </Flexbox>
 
+  _onComponentDidRender: (componentName) =>
+    @_renderedComponents.add(componentName)
+    if @_renderedComponents.size is @state.components.length
+      @props.onComponentsDidRender()
+
   _getStateFromStores: (props) =>
     props ?= @props
 

src/components/injected-component.cjsx

@@ -36,28 +36,33 @@ class InjectedComponent extends React.Component
       This set of descriptors is provided to {ComponentRegistry::findComponentsForDescriptor}
       to retrieve the component that will be displayed.
 
+   - `onComponentDidRender` (optional) Callback that will be called when the injected component
+      is successfully rendered onto the DOM.
+
    - `className` (optional) A {String} class name for the containing element.
 
    - `exposedProps` (optional) An {Object} with props that will be passed to each
       item rendered into the set.
 
+   - `fallback` (optional) A {Component} to default to in case there are no matching
+     components in the ComponentRegistry
+
    - `requiredMethods` (options) An {Array} with a list of methods that should be
      implemented by the registered component instance. If these are not implemented,
      an error will be thrown.
 
-   - `fallback` (optional) A {Component} to default to in case there are no matching
-     components in the ComponentRegistry
-
   ###
   @propTypes:
     matching: React.PropTypes.object.isRequired
     className: React.PropTypes.string
     exposedProps: React.PropTypes.object
     fallback: React.PropTypes.func
+    onComponentDidRender: React.PropTypes.func
     requiredMethods: React.PropTypes.arrayOf(React.PropTypes.string)
 
   @defaultProps:
     requiredMethods: []
+    onComponentDidRender: ->
 
   constructor: (@props) ->
     @state = @_getStateFromStores()
@@ -67,6 +72,7 @@ class InjectedComponent extends React.Component
   componentDidMount: =>
     @_componentUnlistener = ComponentRegistry.listen =>
       @setState(@_getStateFromStores())
+    @props.onComponentDidRender() if @state.component?.containerRequired is false
 
   componentWillUnmount: =>
     @_componentUnlistener() if @_componentUnlistener
@@ -77,6 +83,7 @@ class InjectedComponent extends React.Component
 
   componentDidUpdate: =>
     @_setRequiredMethods(@props.requiredMethods)
+    @props.onComponentDidRender() if @state.component?.containerRequired is false
 
   render: =>
     return <div></div> unless @state.component
@@ -89,7 +96,14 @@ class InjectedComponent extends React.Component
     if component.containerRequired is false
       element = <component ref="inner" key={component.displayName} {...exposedProps} />
     else
-      element = <UnsafeComponent ref="inner" component={component} key={component.displayName} {...exposedProps} />
+      element = (
+        <UnsafeComponent
+          ref="inner"
+          key={component.displayName}
+          component={component}
+          onComponentDidRender={@props.onComponentDidRender}
+          {...exposedProps} />
+      )
 
     if @state.visible
       <div className={className}>

src/components/multiselect-split-interaction-handler.coffee

@@ -17,7 +17,7 @@ class MultiselectSplitInteractionHandler
     @dataView.selection.count() > 1
 
   onClick: (item) ->
-    Actions.setFocus({collection: @collection, item: item})
+    Actions.setFocus({collection: @collection, item: item, usingClick: true})
     @dataView.selection.clear()
     @_checkSelectionAndFocusConsistency()
 

src/components/unsafe-component.cjsx

@@ -34,6 +34,10 @@ class UnsafeComponent extends React.Component
   ###
   @propTypes:
     component: React.PropTypes.func.isRequired
+    onComponentDidRender: React.PropTypes.func
+
+  @defaultProps:
+    onComponentDidRender: ->
 
   componentDidMount: =>
     @renderInjected()
@@ -54,7 +58,7 @@ class UnsafeComponent extends React.Component
       props = _.omit(@props, _.keys(@constructor.propTypes))
       component = @props.component
       element = <component key={name} {...props} />
-      @injected = React.render(element, node)
+      @injected = React.render(element, node, @props.onComponentDidRender)
     catch err
       if NylasEnv.inDevMode()
         console.error err
@@ -65,10 +69,12 @@ class UnsafeComponent extends React.Component
           stackEnd = stack.lastIndexOf('\n', stackEnd)
           stack = stack.substr(0,stackEnd)
 
-        element = <div className="unsafe-component-exception">
-          <div className="message">{@props.component.displayName} could not be displayed.</div>
-          <div className="trace">{stack}</div>
-        </div>
+        element = (
+          <div className="unsafe-component-exception">
+            <div className="message">{@props.component.displayName} could not be displayed.</div>
+            <div className="trace">{stack}</div>
+          </div>
+        )
       else
         ## TODO
         # Add some sort of notification code here that lets us know when
@@ -76,7 +82,7 @@ class UnsafeComponent extends React.Component
         #
         element = <div></div>
 
-    @injected = React.render(element, node)
+        @injected = React.render(element, node)
 
   unmountInjected: =>
     try

src/flux/stores/focused-content-store.coffee

@@ -70,6 +70,7 @@ class FocusedContentStore
 
   _resetInstanceVars: =>
     @_focused = {}
+    @_focusedUsingClick = {}
     @_keyboardCursor = {}
     @_keyboardCursorEnabled = WorkspaceStore.layoutMode() is 'list'
 
@@ -87,11 +88,12 @@ class FocusedContentStore
     @_keyboardCursor[collection] = item
     @triggerAfterAnimationFrame({ impactsCollection: (c) -> c is collection })
 
-  _onFocus: ({collection, item}) =>
+  _onFocus: ({collection, item, usingClick}) =>
     throw new Error("focus() requires a collection") unless collection
     return if @_focused[collection]?.id is item?.id
 
     @_focused[collection] = item
+    @_focusedUsingClick[collection] = usingClick
     @_keyboardCursor[collection] = item if item
     @triggerAfterAnimationFrame({ impactsCollection: (c) -> c is collection })
 
@@ -155,6 +157,16 @@ class FocusedContentStore
   focusedId: (collection) =>
     @_focused[collection]?.id
 
+  ###
+  Public: Returns true if the item for the collection was focused via a click or
+  false otherwise.
+
+  - `collection` The {String} name of a collection. Standard collections are
+    listed above.
+  ###
+  didFocusUsingClick: (collection) =>
+    @_focusedUsingClick[collection] ? false
+
   ###
   Public: Returns the {Model} the keyboard is currently focused on
   in the collection specified. Keyboard focus is not always separate from