diff --git a/internal_packages/composer/lib/composer-view.cjsx b/internal_packages/composer/lib/composer-view.cjsx index b607a0cb7..130232dfb 100644 --- a/internal_packages/composer/lib/composer-view.cjsx +++ b/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) diff --git a/internal_packages/composer/spec/composer-view-spec.cjsx b/internal_packages/composer/spec/composer-view-spec.cjsx index bc541e6f1..8a26915af 100644 --- a/internal_packages/composer/spec/composer-view-spec.cjsx +++ b/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 World
This is a test" replyToMessageId: null - makeComposer = -> + makeComposer = (props={}) -> @composer = NylasTestUtils.renderIntoDocument( - + ) 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", -> diff --git a/spec/components/injected-component-set-spec.jsx b/spec/components/injected-component-set-spec.jsx new file mode 100644 index 000000000..8309338ca --- /dev/null +++ b/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
; }, + }); +}; + + +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( + + ); + }); + + 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); + }); + }); + }); +}); diff --git a/spec/components/multiselect-split-interaction-handler-spec.coffee b/spec/components/multiselect-split-interaction-handler-spec.coffee index bae6e9b8f..b268aae72 100644 --- a/spec/components/multiselect-split-interaction-handler-spec.coffee +++ b/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", -> diff --git a/src/components/injected-component-set.cjsx b/src/components/injected-component-set.cjsx index 86ee538ab..85fe90a26 100644 --- a/src/components/injected-component-set.cjsx +++ b/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 - + return else - + return ( + + ) if @state.visible @@ -95,10 +110,15 @@ class InjectedComponentSet extends React.Component elements.push() - {elements} - {@props.children ? []} + {elements} + {@props.children ? []} + _onComponentDidRender: (componentName) => + @_renderedComponents.add(componentName) + if @_renderedComponents.size is @state.components.length + @props.onComponentsDidRender() + _getStateFromStores: (props) => props ?= @props diff --git a/src/components/injected-component.cjsx b/src/components/injected-component.cjsx index 84a3e90a5..e1d870ee5 100644 --- a/src/components/injected-component.cjsx +++ b/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
unless @state.component @@ -89,7 +96,14 @@ class InjectedComponent extends React.Component if component.containerRequired is false element = else - element = + element = ( + + ) if @state.visible
diff --git a/src/components/multiselect-split-interaction-handler.coffee b/src/components/multiselect-split-interaction-handler.coffee index bf4a9df74..0b48046f3 100644 --- a/src/components/multiselect-split-interaction-handler.coffee +++ b/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() diff --git a/src/components/unsafe-component.cjsx b/src/components/unsafe-component.cjsx index 15cf63d40..c4a9df36d 100644 --- a/src/components/unsafe-component.cjsx +++ b/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 = - @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 =
-
{@props.component.displayName} could not be displayed.
-
{stack}
-
+ element = ( +
+
{@props.component.displayName} could not be displayed.
+
{stack}
+
+ ) else ## TODO # Add some sort of notification code here that lets us know when @@ -76,7 +82,7 @@ class UnsafeComponent extends React.Component # element =
- @injected = React.render(element, node) + @injected = React.render(element, node) unmountInjected: => try diff --git a/src/flux/stores/focused-content-store.coffee b/src/flux/stores/focused-content-store.coffee index f9614f2e2..de873b7b2 100644 --- a/src/flux/stores/focused-content-store.coffee +++ b/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