fix(focus): Minor changes to composer focus logic to focus new drafts

Summary:
Remove FocusTrackingRegion—all CommandRegions should be focusable, and nesting the two creates varying behavior based on which is the parent

Calling focus() on an injected / unsafe component should always do /something/. Try the inner React method, inner DOM method, or call on ourselves

Rename contentEditable._focusEditor to "focus" since it intends to replace default focus behavior

In ComposerView, always change focus via setState, never by calling focus() directly. Rather than tracking `_lastFocusedField`, just focus whenever the activeElement isnt within the focusedField. Make body initial focus when draft is pristine...

...(ensures new drafts are focused)

Test Plan: Run tests

Reviewers: evan, juan

Reviewed By: evan, juan

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

internal_packages/composer/lib/composer-editor.jsx

@@ -120,7 +120,7 @@ class ComposerEditor extends Component {
     return this.refs.contenteditable.getPreviousSelection();
   }
 
-  focusEditor() {
+  focus() {
     this.refs.contenteditable.selectEnd();
   }
 

internal_packages/composer/lib/composer-view.cjsx

@@ -20,7 +20,6 @@ React = require 'react'
  ScrollRegion,
  InjectedComponent,
  KeyCommandsRegion,
- FocusTrackingRegion,
  InjectedComponentSet} = require 'nylas-component-kit'
 
 FileUpload = require './file-upload'
@@ -108,6 +107,9 @@ class ComposerView extends React.Component
     return if bodyChanged
     @_applyFieldFocus()
 
+  focus: =>
+    @_applyFieldFocus()
+
   _keymapHandlers: ->
     'composer:send-message': => @_sendDraft()
     'composer:delete-empty-draft': => @_deleteDraftIfEmpty()
@@ -122,16 +124,18 @@ class ComposerView extends React.Component
     "composer:redo": @redo
 
   _applyFieldFocus: =>
-    if @state.focusedField and @_lastFocusedField isnt @state.focusedField
-      @_lastFocusedField = @state.focusedField
-      return unless @refs[@state.focusedField]
-      if @refs[@state.focusedField].focus
-        @refs[@state.focusedField].focus()
-      else
-        React.findDOMNode(@refs[@state.focusedField]).focus()
+    @_applyFieldFocusTo(@state.focusedField)
 
-      if @state.focusedField is Fields.Body
-        @_focusEditor()
+  _applyFieldFocusTo: (fieldName) =>
+    return unless @refs[fieldName]
+
+    $el = React.findDOMNode(@refs[fieldName])
+    return if document.activeElement is $el or $el.contains(document.activeElement)
+
+    if @refs[fieldName].focus
+      @refs[fieldName].focus()
+    else
+      $el.focus()
 
   componentWillReceiveProps: (newProps) =>
     @_ignoreNextTrigger = false
@@ -170,26 +174,17 @@ class ComposerView extends React.Component
       @_proxy.changes.commit()
 
   render: ->
-    <KeyCommandsRegion localHandlers={@_keymapHandlers()} >
-      {@_renderComposerWrap()}
+    classes = "message-item-white-wrap composer-outer-wrap #{@props.className ? ""}"
+
+    <KeyCommandsRegion
+      localHandlers={@_keymapHandlers()}
+      className={classes}
+      onFocusIn={@_onFocusIn}
+      tabIndex="-1"
+      ref="composerWrap">
+      {@_renderComposer()}
     </KeyCommandsRegion>
 
-  _renderComposerWrap: =>
-    if @props.mode is "inline"
-      <FocusTrackingRegion className={@_wrapClasses()}
-                           ref="composerWrap"
-                           onFocusIn={@_onFocusIn}
-                           tabIndex="-1">
-        {@_renderComposer()}
-      </FocusTrackingRegion>
-    else
-      <div className={@_wrapClasses()} ref="composerWrap">
-        {@_renderComposer()}
-      </div>
-
-  _wrapClasses: =>
-    "message-item-white-wrap composer-outer-wrap #{@props.className ? ""}"
-
   _renderComposer: =>
     <DropZone className="composer-inner-wrap"
               shouldAcceptDrop={@_shouldAcceptDrop}
@@ -335,16 +330,13 @@ class ComposerView extends React.Component
       fallback={ComposerEditor}
       onComponentDidRender={@_onEditorBodyDidRender}
       requiredMethods={[
-        'focusEditor'
+        'focus'
         'getCurrentSelection'
         'getPreviousSelection'
         '_onDOMMutated'
       ]}
       exposedProps={exposedProps} />
 
-  _focusEditor: =>
-    @refs[Fields.Body].focusEditor()
-
   _onEditorBodyDidRender: =>
     @_applyFieldFocus()
 
@@ -503,7 +495,7 @@ class ComposerView extends React.Component
 
   _onMouseUpComposerBody: (event) =>
     if event.target is @_mouseDownTarget
-      @_focusEditor()
+      @setState(focusedField: Fields.Body)
     @_mouseDownTarget = null
 
   # When a user focuses the composer, it's possible that no input is
@@ -512,7 +504,7 @@ class ComposerView extends React.Component
   # erroneously trigger keyboard shortcuts.
   _onFocusIn: (event) =>
     return if DOMUtils.closest(event.target, DOMUtils.inputTypes())
-    @_focusEditor()
+    @setState(focusedField: @_initiallyFocusedField(@_proxy.draft()))
 
   _onMouseMoveComposeBody: (event) =>
     if @_mouseComposeBody is "down" then @_mouseComposeBody = "move"
@@ -551,9 +543,10 @@ class ComposerView extends React.Component
     return Fields.To if draft.to.length is 0
     return Fields.Subject if (draft.subject ? "").trim().length is 0
 
-    shouldFocusBody = @props.mode isnt 'inline' or
+    shouldFocusBody = @props.mode isnt 'inline' or draft.pristine or
       (FocusedContentStore.didFocusUsingClick('thread') is true)
     return Fields.Body if shouldFocusBody
+    return null
 
   _verifyEnabledFields: (draft, state) ->
     enabledFields = @state.enabledFields.concat(state.enabledFields)

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

@@ -273,7 +273,7 @@ describe "ComposerView", ->
         spyOn(FocusedContentStore, 'didFocusUsingClick').andReturn false
         useDraft.call @, to: [u1], subject: "Yo"
         makeComposer.call @, {mode: 'inline'}
-        expect(@composer.state.focusedField).toBeUndefined()
+        expect(@composer.state.focusedField).toBe null
 
     describe "when deciding whether or not to enable the subject", ->
       it "enables the subject when the subject is empty", ->
@@ -369,33 +369,28 @@ describe "ComposerView", ->
         @composer.setState focusedField: Fields.Cc
         @body = @composer.refs[Fields.Body]
         spyOn(React, "findDOMNode").andCallThrough()
-        spyOn(@composer, "_focusEditor")
+        spyOn(@composer, "focus")
         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()
+        expect(@composer.focus).not.toHaveBeenCalled()
         @composer._lastFocusedField = null
 
       it "can focus on the subject", ->
         @composer.setState focusedField: Fields.Subject
         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
+        expect(React.findDOMNode).toHaveBeenCalledWith(@composer.refs[Fields.Subject])
 
       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()
+        @composer._onEditorBodyDidRender()
         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(@composer._focusEditor).not.toHaveBeenCalled()
+        expect(@composer.focus).not.toHaveBeenCalled()
         expect(@composer._applyFieldFocus.calls.length).toBe 2
 
     describe "when participants are added during a draft update", ->

src/components/focus-tracking-region.cjsx

@@ -1,73 +0,0 @@
-React = require 'react'
-_ = require 'underscore'
-
-# Public: FocusTrackingRegion is a small wrap component that renders it's children
-# and any props it's provided. Whenever the document's focus is inside the
-# FocusTrackingRegion, it has an additional CSS class: `focused`
-#
-class FocusTrackingRegion extends React.Component
-  @displayName: 'FocusTrackingRegion'
-
-  @propTypes:
-    className: React.PropTypes.string
-    children: React.PropTypes.any
-    onFocusIn: React.PropTypes.func
-    onFocusOut: React.PropTypes.func
-
-  @defaultProps:
-    onFocusIn: ->
-    onFocusOut: ->
-
-  constructor: (@props) ->
-    @state = {focused: false}
-    @_goingout = false
-
-    @_in = (args...) =>
-      @_goingout = false
-      @props.onFocusIn(args...) if @state.focused is false
-      @setState(focused: true)
-
-    @_out = =>
-      @_goingout = true
-      setTimeout =>
-        return unless @_goingout
-
-        # If we're unmounted the `@_goingout` flag will catch the unmount
-        # @_goingout is set to true when we umount
-        #
-        # It's posible for a focusout event to fire from within a region
-        # that we're actually focsued on.
-        #
-        # This happens when component that used to have the focus is
-        # unmounted. An example is the url input field of the
-        # FloatingToolbar in the Composer's Contenteditable
-        el = React.findDOMNode(@)
-        return if el.contains document.activeElement
-
-        # This prevents the strange effect of an input appearing to have focus
-        # when the element receiving focus does not support selection (like a
-        # div with tabIndex=-1)
-        document.getSelection().empty()
-        @props.onFocusOut() if @state.focused is true
-        @setState(focused: false)
-        @_goingout = false
-      , 100
-
-  componentDidMount: ->
-    el = React.findDOMNode(@)
-    el.addEventListener('focusin', @_in)
-    el.addEventListener('focusout', @_out)
-
-  componentWillUnmount: ->
-    el = React.findDOMNode(@)
-    el.removeEventListener('focusin', @_in)
-    el.removeEventListener('focusout', @_out)
-    @_goingout = false
-
-  render: ->
-    className = @props.className
-    className += " focused" if @state.focused
-    otherProps = _.omit(@props, _.keys(@constructor.propTypes))
-    <div className={className} {...otherProps}>{@props.children}</div>
-
-module.exports = FocusTrackingRegion

src/components/injected-component.cjsx

@@ -117,14 +117,26 @@ class InjectedComponent extends React.Component
       </div>
 
   focus: =>
-    # Not forwarding event - just a method call
-    # Note that our inner may not be populated, and it may not have a focus method
-    @refs.inner.focus() if @refs.inner?.focus?
+    @_runInnerDOMMethod('focus')
 
   blur: =>
-    # Not forwarding an event - just a method call
-    # Note that our inner may not be populated, and it may not have a blur method
-    @refs.inner.blur() if @refs.inner?.blur?
+    @_runInnerDOMMethod('blur')
+
+  # Private: Attempts to run the DOM method, ie 'focus', on
+  # 1. Any implementation provided by the inner component
+  # 2. Any native implementation provided by the DOM
+  # 3. Ourselves, so that the method always has /some/ effect.
+  #
+  _runInnerDOMMethod: (method) =>
+    target = null
+    if @refs.inner and @refs.inner[method]
+      target = @refs.inner
+    else if @refs.inner
+      target = React.findDOMNode(@refs.inner)
+    else
+      target = React.findDOMNode(@)
+
+    target[method]?()
 
   _setRequiredMethods: (methods) =>
     methods.forEach (method) =>

src/components/key-commands-region.cjsx

@@ -1,5 +1,6 @@
 _ = require 'underscore'
 React = require 'react'
+classNames = require 'classnames'
 
 ###
 Public: Easily respond to keyboard shortcuts
@@ -85,11 +86,51 @@ class KeyCommandsRegion extends React.Component
     className: React.PropTypes.string
     localHandlers: React.PropTypes.object
     globalHandlers: React.PropTypes.object
+    onFocusIn: React.PropTypes.func
+    onFocusOut: React.PropTypes.func
 
   @defaultProps:
     className: ""
     localHandlers: {}
     globalHandlers: {}
+    onFocusIn: ->
+    onFocusOut: ->
+
+  constructor: ->
+    @state = {focused: false}
+    @_goingout = false
+
+    @_in = (args...) =>
+      @_goingout = false
+      @props.onFocusIn(args...) if @state.focused is false
+      @setState(focused: true)
+
+    @_out = =>
+      @_goingout = true
+      setTimeout =>
+        return unless @_goingout
+
+        # If we're unmounted the `@_goingout` flag will catch the unmount
+        # @_goingout is set to true when we umount
+        #
+        # It's posible for a focusout event to fire from within a region
+        # that we're actually focsued on.
+        #
+        # This happens when component that used to have the focus is
+        # unmounted. An example is the url input field of the
+        # FloatingToolbar in the Composer's Contenteditable
+        el = React.findDOMNode(@)
+        return if el.contains document.activeElement
+
+        # This prevents the strange effect of an input appearing to have focus
+        # when the element receiving focus does not support selection (like a
+        # div with tabIndex=-1)
+        document.getSelection().empty()
+        @props.onFocusOut() if @state.focused is true
+        @setState(focused: false)
+        @_goingout = false
+      , 100
+
 
   componentWillReceiveProps: (newProps) ->
     @_unmountListeners()
@@ -117,16 +158,26 @@ class KeyCommandsRegion extends React.Component
     return unless @_mounted
     $el = React.findDOMNode(@)
     @_localDisposable = NylasEnv.commands.add($el, props.localHandlers)
+    $el.addEventListener('focusin', @_in)
+    $el.addEventListener('focusout', @_out)
 
   _unmountListeners: ->
     @_globalDisposable?.dispose()
     @_globalDisposable = null
     @_localDisposable?.dispose()
     @_localDisposable = null
+    $el = React.findDOMNode(@)
+    $el.removeEventListener('focusin', @_in)
+    $el.removeEventListener('focusout', @_out)
+    @_goingout = false
 
   render: ->
+    classname = classNames
+      'key-commands-region': true
+      'focused': @state.focused
     otherProps = _.omit(@props, _.keys(@constructor.propTypes))
-    <div className="key-commands-region #{@props.className}" {...otherProps}>
+
+    <div className="#{classname} #{@props.className}" {...otherProps}>
       {@props.children}
     </div>
 

src/components/unsafe-component.cjsx

@@ -91,12 +91,25 @@ class UnsafeComponent extends React.Component
     catch err
 
   focus: =>
-    # Not forwarding event - just a method call
-    @injected.focus() if @injected.focus?
+    @_runInjectedDOMMethod('focus')
 
   blur: =>
-    # Not forwarding an event - just a method call
-    @injected.blur() if @injected.blur?
+    @_runInjectedDOMMethod('blur')
 
+  # Private: Attempts to run the DOM method, ie 'focus', on
+  # 1. Any implementation provided by the inner component
+  # 2. Any native implementation provided by the DOM
+  # 3. Ourselves, so that the method always has /some/ effect.
+  #
+  _runInjectedDOMMethod: (method) =>
+    target = null
+    if @injected and @injected[method]
+      target = @injected
+    else if @injected
+      target = React.findDOMNode(@injected)
+    else
+      target = React.findDOMNode(@)
+
+    target[method]?()
 
 module.exports = UnsafeComponent

src/global/nylas-component-kit.coffee

@@ -35,7 +35,6 @@ class NylasComponentKit
 
   @load "ScrollRegion", 'scroll-region'
   @load "ResizableRegion", 'resizable-region'
-  @load "FocusTrackingRegion", 'focus-tracking-region'
 
   @loadFrom "MailLabel", "mail-label"
   @loadFrom "LabelColorizer", "mail-label"