From ce441a683d293393cee56b4ea0ac9473839323ed Mon Sep 17 00:00:00 2001 From: Nicolas Perriault Date: Thu, 11 Sep 2014 20:54:48 +0100 Subject: [PATCH] Bug 1062126: Loop panel UI shouldn't be fully reset when reopened, r=mikedeboer. --- .../components/loop/content/conversation.html | 1 + browser/components/loop/content/js/panel.js | 95 ++++++------------- browser/components/loop/content/js/panel.jsx | 95 ++++++------------- browser/components/loop/content/panel.html | 3 +- .../loop/content/shared/css/panel.css | 8 +- .../loop/content/shared/js/mixins.js | 95 +++++++++++++++++++ .../loop/content/shared/js/views.js | 32 ++++++- .../loop/content/shared/js/views.jsx | 30 +++++- browser/components/loop/jar.mn | 1 + .../loop/standalone/content/index.html | 1 + .../loop/test/desktop-local/index.html | 1 + .../loop/test/desktop-local/panel_test.js | 65 ++----------- .../components/loop/test/shared/index.html | 2 + .../loop/test/shared/mixins_test.js | 70 ++++++++++++++ .../loop/test/standalone/index.html | 1 + browser/components/loop/ui/index.html | 1 + 16 files changed, 296 insertions(+), 205 deletions(-) create mode 100644 browser/components/loop/content/shared/js/mixins.js create mode 100644 browser/components/loop/test/shared/mixins_test.js diff --git a/browser/components/loop/content/conversation.html b/browser/components/loop/content/conversation.html index dfff4b744a8..f034eba3e0c 100644 --- a/browser/components/loop/content/conversation.html +++ b/browser/components/loop/content/conversation.html @@ -35,6 +35,7 @@ + diff --git a/browser/components/loop/content/js/panel.js b/browser/components/loop/content/js/panel.js index e9aa0ad6115..3e88b40c3ac 100644 --- a/browser/components/loop/content/js/panel.js +++ b/browser/components/loop/content/js/panel.js @@ -11,10 +11,10 @@ var loop = loop || {}; loop.panel = (function(_, mozL10n) { "use strict"; - var sharedViews = loop.shared.views, - sharedModels = loop.shared.models, - // aliasing translation function as __ for concision - __ = mozL10n.get; + var sharedViews = loop.shared.views; + var sharedModels = loop.shared.models; + var sharedMixins = loop.shared.mixins; + var __ = mozL10n.get; // aliasing translation function as __ for concision /** * Panel router. @@ -73,41 +73,11 @@ loop.panel = (function(_, mozL10n) { } }); - /** - * Dropdown menu mixin. - * @type {Object} - */ - var DropdownMenuMixin = { - getInitialState: function() { - return {showMenu: false}; - }, - - _onBodyClick: function() { - this.setState({showMenu: false}); - }, - - componentDidMount: function() { - document.body.addEventListener("click", this._onBodyClick); - }, - - componentWillUnmount: function() { - document.body.removeEventListener("click", this._onBodyClick); - }, - - showDropdownMenu: function() { - this.setState({showMenu: true}); - }, - - hideDropdownMenu: function() { - this.setState({showMenu: false}); - } - }; - /** * Availability drop down menu subview. */ var AvailabilityDropdown = React.createClass({displayName: 'AvailabilityDropdown', - mixins: [DropdownMenuMixin], + mixins: [sharedMixins.DropdownMenuMixin], getInitialState: function() { return { @@ -239,7 +209,7 @@ loop.panel = (function(_, mozL10n) { * Panel settings (gear) menu. */ var SettingsDropdown = React.createClass({displayName: 'SettingsDropdown', - mixins: [DropdownMenuMixin], + mixins: [sharedMixins.DropdownMenuMixin], handleClickSettingsEntry: function() { // XXX to be implemented @@ -308,7 +278,12 @@ loop.panel = (function(_, mozL10n) { } }); + /** + * Call url result view. + */ var CallUrlResult = React.createClass({displayName: 'CallUrlResult', + mixins: [sharedMixins.DocumentVisibilityMixin], + propTypes: { callUrl: React.PropTypes.string, callUrlExpiry: React.PropTypes.number, @@ -325,6 +300,14 @@ loop.panel = (function(_, mozL10n) { }; }, + /** + * Provided by DocumentVisibilityMixin. Schedules retrieval of a new call + * URL everytime the panel is reopened. + */ + onDocumentVisible: function() { + this._fetchCallUrl(); + }, + /** * Returns a random 5 character string used to identify * the conversation. @@ -341,6 +324,13 @@ loop.panel = (function(_, mozL10n) { return; } + this._fetchCallUrl(); + }, + + /** + * Fetches a call URL. + */ + _fetchCallUrl: function() { this.setState({pending: true}); this.props.client.requestCallUrl(this.conversationIdentifier(), this._onCallUrlReceived); @@ -506,7 +496,8 @@ loop.panel = (function(_, mozL10n) { __("display_name_guest"); return ( React.DOM.div(null, - NotificationListView({notifications: this.props.notifications}), + NotificationListView({notifications: this.props.notifications, + clearOnDocumentHidden: true}), TabView({onSelect: this.selectTab}, Tab({name: "call"}, CallUrlResult({client: this.props.client, @@ -547,42 +538,12 @@ loop.panel = (function(_, mozL10n) { if (!options.document) { throw new Error("missing required document"); } - this.document = options.document; - - this._registerVisibilityChangeEvent(); - - this.on("panel:open", this.reset, this); - }, - - /** - * Register the DOM visibility API event for the whole document, and trigger - * appropriate events accordingly: - * - * - `panel:opened` when the panel is open - * - `panel:closed` when the panel is closed - * - * @link http://www.w3.org/TR/page-visibility/ - */ - _registerVisibilityChangeEvent: function() { - // XXX pass in the visibility status to detect when to generate a new - // panel view - this.document.addEventListener("visibilitychange", function(event) { - this.trigger(event.currentTarget.hidden ? "panel:closed" - : "panel:open"); - }.bind(this)); }, /** * Default entry point. */ home: function() { - this.reset(); - }, - - /** - * Resets this router to its initial state. - */ - reset: function() { this._notifications.reset(); var client = new loop.Client({ baseServerUrl: navigator.mozLoop.serverUrl diff --git a/browser/components/loop/content/js/panel.jsx b/browser/components/loop/content/js/panel.jsx index 0e8a3e9b03e..cba930f4231 100644 --- a/browser/components/loop/content/js/panel.jsx +++ b/browser/components/loop/content/js/panel.jsx @@ -11,10 +11,10 @@ var loop = loop || {}; loop.panel = (function(_, mozL10n) { "use strict"; - var sharedViews = loop.shared.views, - sharedModels = loop.shared.models, - // aliasing translation function as __ for concision - __ = mozL10n.get; + var sharedViews = loop.shared.views; + var sharedModels = loop.shared.models; + var sharedMixins = loop.shared.mixins; + var __ = mozL10n.get; // aliasing translation function as __ for concision /** * Panel router. @@ -73,41 +73,11 @@ loop.panel = (function(_, mozL10n) { } }); - /** - * Dropdown menu mixin. - * @type {Object} - */ - var DropdownMenuMixin = { - getInitialState: function() { - return {showMenu: false}; - }, - - _onBodyClick: function() { - this.setState({showMenu: false}); - }, - - componentDidMount: function() { - document.body.addEventListener("click", this._onBodyClick); - }, - - componentWillUnmount: function() { - document.body.removeEventListener("click", this._onBodyClick); - }, - - showDropdownMenu: function() { - this.setState({showMenu: true}); - }, - - hideDropdownMenu: function() { - this.setState({showMenu: false}); - } - }; - /** * Availability drop down menu subview. */ var AvailabilityDropdown = React.createClass({ - mixins: [DropdownMenuMixin], + mixins: [sharedMixins.DropdownMenuMixin], getInitialState: function() { return { @@ -239,7 +209,7 @@ loop.panel = (function(_, mozL10n) { * Panel settings (gear) menu. */ var SettingsDropdown = React.createClass({ - mixins: [DropdownMenuMixin], + mixins: [sharedMixins.DropdownMenuMixin], handleClickSettingsEntry: function() { // XXX to be implemented @@ -308,7 +278,12 @@ loop.panel = (function(_, mozL10n) { } }); + /** + * Call url result view. + */ var CallUrlResult = React.createClass({ + mixins: [sharedMixins.DocumentVisibilityMixin], + propTypes: { callUrl: React.PropTypes.string, callUrlExpiry: React.PropTypes.number, @@ -325,6 +300,14 @@ loop.panel = (function(_, mozL10n) { }; }, + /** + * Provided by DocumentVisibilityMixin. Schedules retrieval of a new call + * URL everytime the panel is reopened. + */ + onDocumentVisible: function() { + this._fetchCallUrl(); + }, + /** * Returns a random 5 character string used to identify * the conversation. @@ -341,6 +324,13 @@ loop.panel = (function(_, mozL10n) { return; } + this._fetchCallUrl(); + }, + + /** + * Fetches a call URL. + */ + _fetchCallUrl: function() { this.setState({pending: true}); this.props.client.requestCallUrl(this.conversationIdentifier(), this._onCallUrlReceived); @@ -506,7 +496,8 @@ loop.panel = (function(_, mozL10n) { __("display_name_guest"); return (
- + -
-
@@ -25,6 +23,7 @@ + diff --git a/browser/components/loop/content/shared/css/panel.css b/browser/components/loop/content/shared/css/panel.css index 436316b5774..69b94eea596 100644 --- a/browser/components/loop/content/shared/css/panel.css +++ b/browser/components/loop/content/shared/css/panel.css @@ -137,8 +137,12 @@ /* Specific cases */ -.panel #messages .alert { - margin-bottom: 0; +.panel .messages { + margin: 0; +} + +.panel .messages .alert { + margin: 0; } /* Dropdown menu (shared styles) */ diff --git a/browser/components/loop/content/shared/js/mixins.js b/browser/components/loop/content/shared/js/mixins.js new file mode 100644 index 00000000000..9a36f620e04 --- /dev/null +++ b/browser/components/loop/content/shared/js/mixins.js @@ -0,0 +1,95 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* global loop:true */ + +var loop = loop || {}; +loop.shared = loop.shared || {}; +loop.shared.mixins = (function() { + "use strict"; + + /** + * Root object, by default set to window. + * @type {DOMWindow|Object} + */ + var rootObject = window; + + /** + * Sets a new root object. This is useful for testing native DOM events so we + * can fake them. + * + * @param {Object} + */ + function setRootObject(obj) { + console.info("loop.shared.mixins: rootObject set to " + obj); + rootObject = obj; + } + + /** + * Dropdown menu mixin. + * @type {Object} + */ + var DropdownMenuMixin = { + getInitialState: function() { + return {showMenu: false}; + }, + + _onBodyClick: function() { + this.setState({showMenu: false}); + }, + + componentDidMount: function() { + rootObject.document.body.addEventListener("click", this._onBodyClick); + }, + + componentWillUnmount: function() { + rootObject.document.body.removeEventListener("click", this._onBodyClick); + }, + + showDropdownMenu: function() { + this.setState({showMenu: true}); + }, + + hideDropdownMenu: function() { + this.setState({showMenu: false}); + } + }; + + /** + * Document visibility mixin. Allows defining the following hooks for when the + * document visibility status changes: + * + * - {Function} onDocumentVisible For when the document becomes visible. + * - {Function} onDocumentHidden For when the document becomes hidden. + * + * @type {Object} + */ + var DocumentVisibilityMixin = { + _onDocumentVisibilityChanged: function(event) { + var hidden = event.target.hidden; + if (hidden && typeof this.onDocumentHidden === "function") { + this.onDocumentHidden(); + } + if (!hidden && typeof this.onDocumentVisible === "function") { + this.onDocumentVisible(); + } + }, + + componentDidMount: function() { + rootObject.document.addEventListener( + "visibilitychange", this._onDocumentVisibilityChanged); + }, + + componentWillUnmount: function() { + rootObject.document.removeEventListener( + "visibilitychange", this._onDocumentVisibilityChanged); + } + }; + + return { + setRootObject: setRootObject, + DropdownMenuMixin: DropdownMenuMixin, + DocumentVisibilityMixin: DocumentVisibilityMixin + }; +})(); diff --git a/browser/components/loop/content/shared/js/views.js b/browser/components/loop/content/shared/js/views.js index d76c37a5871..411ecbe049f 100644 --- a/browser/components/loop/content/shared/js/views.js +++ b/browser/components/loop/content/shared/js/views.js @@ -12,6 +12,8 @@ loop.shared.views = (function(_, OT, l10n) { "use strict"; var sharedModels = loop.shared.models; + var sharedMixins = loop.shared.mixins; + var WINDOW_AUTOCLOSE_TIMEOUT_IN_SECONDS = 5; /** @@ -575,8 +577,7 @@ loop.shared.views = (function(_, OT, l10n) { /** * Notification view. */ - var NotificationView = React.createClass({ - displayName: 'NotificationView', + var NotificationView = React.createClass({displayName: 'NotificationView', mixins: [Backbone.Events], propTypes: { @@ -599,10 +600,15 @@ loop.shared.views = (function(_, OT, l10n) { * Notification list view. */ var NotificationListView = React.createClass({displayName: 'NotificationListView', - mixins: [Backbone.Events], + mixins: [Backbone.Events, sharedMixins.DocumentVisibilityMixin], propTypes: { - notifications: React.PropTypes.object.isRequired + notifications: React.PropTypes.object.isRequired, + clearOnDocumentHidden: React.PropTypes.bool + }, + + getDefaultProps: function() { + return {clearOnDocumentHidden: false}; }, componentDidMount: function() { @@ -615,9 +621,25 @@ loop.shared.views = (function(_, OT, l10n) { this.stopListening(this.props.notifications); }, + /** + * Provided by DocumentVisibilityMixin. Clears notifications stack when the + * current document is hidden if the clearOnDocumentHidden prop is set to + * true and the collection isn't empty. + */ + onDocumentHidden: function() { + if (this.props.clearOnDocumentHidden && + this.props.notifications.length > 0) { + // Note: The `silent` option prevents the `reset` event to be triggered + // here, preventing the UI to "jump" a little because of the event + // callback being processed in another tick (I think). + this.props.notifications.reset([], {silent: true}); + this.forceUpdate(); + } + }, + render: function() { return ( - React.DOM.div({id: "messages"}, + React.DOM.div({className: "messages"}, this.props.notifications.map(function(notification, key) { return NotificationView({key: key, notification: notification}); }) diff --git a/browser/components/loop/content/shared/js/views.jsx b/browser/components/loop/content/shared/js/views.jsx index a25062cb95d..307e40016e1 100644 --- a/browser/components/loop/content/shared/js/views.jsx +++ b/browser/components/loop/content/shared/js/views.jsx @@ -12,6 +12,8 @@ loop.shared.views = (function(_, OT, l10n) { "use strict"; var sharedModels = loop.shared.models; + var sharedMixins = loop.shared.mixins; + var WINDOW_AUTOCLOSE_TIMEOUT_IN_SECONDS = 5; /** @@ -576,7 +578,6 @@ loop.shared.views = (function(_, OT, l10n) { * Notification view. */ var NotificationView = React.createClass({ - displayName: 'NotificationView', mixins: [Backbone.Events], propTypes: { @@ -599,10 +600,15 @@ loop.shared.views = (function(_, OT, l10n) { * Notification list view. */ var NotificationListView = React.createClass({ - mixins: [Backbone.Events], + mixins: [Backbone.Events, sharedMixins.DocumentVisibilityMixin], propTypes: { - notifications: React.PropTypes.object.isRequired + notifications: React.PropTypes.object.isRequired, + clearOnDocumentHidden: React.PropTypes.bool + }, + + getDefaultProps: function() { + return {clearOnDocumentHidden: false}; }, componentDidMount: function() { @@ -615,9 +621,25 @@ loop.shared.views = (function(_, OT, l10n) { this.stopListening(this.props.notifications); }, + /** + * Provided by DocumentVisibilityMixin. Clears notifications stack when the + * current document is hidden if the clearOnDocumentHidden prop is set to + * true and the collection isn't empty. + */ + onDocumentHidden: function() { + if (this.props.clearOnDocumentHidden && + this.props.notifications.length > 0) { + // Note: The `silent` option prevents the `reset` event to be triggered + // here, preventing the UI to "jump" a little because of the event + // callback being processed in another tick (I think). + this.props.notifications.reset([], {silent: true}); + this.forceUpdate(); + } + }, + render: function() { return ( -
{ +
{ this.props.notifications.map(function(notification, key) { return ; }) diff --git a/browser/components/loop/jar.mn b/browser/components/loop/jar.mn index 20d36890bf9..c28286f5035 100644 --- a/browser/components/loop/jar.mn +++ b/browser/components/loop/jar.mn @@ -51,6 +51,7 @@ browser.jar: content/browser/loop/shared/js/feedbackApiClient.js (content/shared/js/feedbackApiClient.js) content/browser/loop/shared/js/models.js (content/shared/js/models.js) content/browser/loop/shared/js/router.js (content/shared/js/router.js) + content/browser/loop/shared/js/mixins.js (content/shared/js/mixins.js) content/browser/loop/shared/js/views.js (content/shared/js/views.js) content/browser/loop/shared/js/utils.js (content/shared/js/utils.js) content/browser/loop/shared/js/websocket.js (content/shared/js/websocket.js) diff --git a/browser/components/loop/standalone/content/index.html b/browser/components/loop/standalone/content/index.html index 611453d7b38..1b9dde3fc7a 100644 --- a/browser/components/loop/standalone/content/index.html +++ b/browser/components/loop/standalone/content/index.html @@ -36,6 +36,7 @@ + diff --git a/browser/components/loop/test/desktop-local/index.html b/browser/components/loop/test/desktop-local/index.html index d69ef907746..8f620012647 100644 --- a/browser/components/loop/test/desktop-local/index.html +++ b/browser/components/loop/test/desktop-local/index.html @@ -36,6 +36,7 @@ + diff --git a/browser/components/loop/test/desktop-local/panel_test.js b/browser/components/loop/test/desktop-local/panel_test.js index f6e431cfd1a..32a078513ee 100644 --- a/browser/components/loop/test/desktop-local/panel_test.js +++ b/browser/components/loop/test/desktop-local/panel_test.js @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at http://mozilla.org/MPL/2.0/. */ +/*jshint newcap:false*/ /*global loop, sinon */ var expect = chai.expect; @@ -82,25 +83,18 @@ describe("loop.panel", function() { }); describe("#home", function() { - it("should reset the PanelView", function() { - sandbox.stub(router, "reset"); - - router.home(); - - sinon.assert.calledOnce(router.reset); - }); - }); - - describe("#reset", function() { - it("should clear all pending notifications", function() { + beforeEach(function() { sandbox.stub(notifications, "reset"); - router.reset(); + }); + + it("should clear all pending notifications", function() { + router.home(); sinon.assert.calledOnce(notifications.reset); }); it("should load the home view", function() { - router.reset(); + router.home(); sinon.assert.calledOnce(router.loadReactComponent); sinon.assert.calledWithExactly(router.loadReactComponent, @@ -111,51 +105,6 @@ describe("loop.panel", function() { }); }); }); - - describe("Events", function() { - beforeEach(function() { - sandbox.stub(loop.panel.PanelRouter.prototype, "trigger"); - }); - - it("should listen to document visibility changes", function() { - var fakeDocument = { - hidden: true, - addEventListener: sandbox.spy() - }; - - var router = createTestRouter(fakeDocument); - - sinon.assert.calledOnce(fakeDocument.addEventListener); - sinon.assert.calledWith(fakeDocument.addEventListener, - "visibilitychange"); - }); - - it("should trigger panel:open when the panel document is visible", - function() { - var router = createTestRouter({ - hidden: false, - addEventListener: function(name, cb) { - cb({currentTarget: {hidden: false}}); - } - }); - - sinon.assert.calledOnce(router.trigger); - sinon.assert.calledWith(router.trigger, "panel:open"); - }); - - it("should trigger panel:closed when the panel document is hidden", - function() { - var router = createTestRouter({ - hidden: true, - addEventListener: function(name, cb) { - cb({currentTarget: {hidden: true}}); - } - }); - - sinon.assert.calledOnce(router.trigger); - sinon.assert.calledWith(router.trigger, "panel:closed"); - }); - }); }); describe("loop.panel.AvailabilityDropdown", function() { diff --git a/browser/components/loop/test/shared/index.html b/browser/components/loop/test/shared/index.html index bb5898e4866..2291b467d64 100644 --- a/browser/components/loop/test/shared/index.html +++ b/browser/components/loop/test/shared/index.html @@ -34,6 +34,7 @@ + @@ -41,6 +42,7 @@ + diff --git a/browser/components/loop/test/shared/mixins_test.js b/browser/components/loop/test/shared/mixins_test.js new file mode 100644 index 00000000000..eafee57a396 --- /dev/null +++ b/browser/components/loop/test/shared/mixins_test.js @@ -0,0 +1,70 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* global loop, sinon */ +/* jshint newcap:false */ + +var expect = chai.expect; + +describe("loop.shared.mixins", function() { + "use strict"; + + var sandbox; + var sharedMixins = loop.shared.mixins; + + beforeEach(function() { + sandbox = sinon.sandbox.create(); + }); + + afterEach(function() { + sandbox.restore(); + }); + + describe("loop.panel.DocumentVisibilityMixin", function() { + var comp, TestComp, onDocumentVisibleStub, onDocumentHiddenStub; + + beforeEach(function() { + onDocumentVisibleStub = sandbox.stub(); + onDocumentHiddenStub = sandbox.stub(); + + TestComp = React.createClass({ + mixins: [loop.shared.mixins.DocumentVisibilityMixin], + onDocumentHidden: onDocumentHiddenStub, + onDocumentVisible: onDocumentVisibleStub, + render: function() { + return React.DOM.div(); + } + }); + }); + + function setupFakeVisibilityEventDispatcher(event) { + loop.shared.mixins.setRootObject({ + document: { + addEventListener: function(_, fn) { + fn(event); + }, + removeEventListener: sandbox.stub() + } + }); + } + + it("should call onDocumentVisible when document visibility changes to visible", + function() { + setupFakeVisibilityEventDispatcher({target: {hidden: false}}); + + comp = TestUtils.renderIntoDocument(TestComp()); + + sinon.assert.calledOnce(onDocumentVisibleStub); + }); + + it("should call onDocumentVisible when document visibility changes to hidden", + function() { + setupFakeVisibilityEventDispatcher({target: {hidden: true}}); + + comp = TestUtils.renderIntoDocument(TestComp()); + + sinon.assert.calledOnce(onDocumentHiddenStub); + }); + }); +}); diff --git a/browser/components/loop/test/standalone/index.html b/browser/components/loop/test/standalone/index.html index 8070262d2cc..8f2d2f8ab6c 100644 --- a/browser/components/loop/test/standalone/index.html +++ b/browser/components/loop/test/standalone/index.html @@ -33,6 +33,7 @@ + diff --git a/browser/components/loop/ui/index.html b/browser/components/loop/ui/index.html index d44740243f4..06a74801b47 100644 --- a/browser/components/loop/ui/index.html +++ b/browser/components/loop/ui/index.html @@ -34,6 +34,7 @@ +