From c2dc7a8063f08d5eae62723755ba954117fa6e2f Mon Sep 17 00:00:00 2001 From: dxue Date: Tue, 30 Sep 2014 10:24:54 +0800 Subject: [PATCH 1/2] Bug 985853 - [Keyboard UX update][User Story] Hold shift to enter upper case characters --- .../js/keyboard/active_targets_manager.js | 8 +- apps/keyboard/js/keyboard/target_handlers.js | 73 ++++++++++++++++++- .../js/keyboard/target_handlers_manager.js | 11 ++- apps/keyboard/style/keyboard.css | 4 +- .../keyboard/active_targets_manager_test.js | 5 +- .../unit/keyboard/target_handlers_test.js | 50 ++++++++++++- 6 files changed, 136 insertions(+), 15 deletions(-) diff --git a/apps/keyboard/js/keyboard/active_targets_manager.js b/apps/keyboard/js/keyboard/active_targets_manager.js index d33f9ba2ff84..0c6360f35610 100644 --- a/apps/keyboard/js/keyboard/active_targets_manager.js +++ b/apps/keyboard/js/keyboard/active_targets_manager.js @@ -33,6 +33,7 @@ ActiveTargetsManager.prototype.ontargetmovedin = null; ActiveTargetsManager.prototype.ontargetcommitted = null; ActiveTargetsManager.prototype.ontargetcancelled = null; ActiveTargetsManager.prototype.ontargetdoubletapped = null; +ActiveTargetsManager.prototype.onnewtargetwillactivate = null; // Show accent char menu (if there is one) or do other stuff // after LONG_PRESS_TIMEOUT @@ -99,10 +100,11 @@ ActiveTargetsManager.prototype._handlePressStart = function(press, id) { return; } - // All targets before the new touch need to be committed, - // according to UX requirement. + // Notify current targets about the new touch. this.activeTargets.forEach(function(target, id) { - this._handlePressEnd(press, id); + if (typeof this.onnewtargetwillactivate === 'function') { + this.onnewtargetwillactivate(target); + } }, this); var target = press.target; diff --git a/apps/keyboard/js/keyboard/target_handlers.js b/apps/keyboard/js/keyboard/target_handlers.js index ea9149eeda93..3699318d753a 100644 --- a/apps/keyboard/js/keyboard/target_handlers.js +++ b/apps/keyboard/js/keyboard/target_handlers.js @@ -79,6 +79,13 @@ DefaultTargetHandler.prototype.doubleTap = function() { this.app.console.log('DefaultTargetHandler.doubleTap()'); this.commit(); }; +DefaultTargetHandler.prototype.newTargetActivate = function() { + // According to UX requirement, the current target need to be committed when + // there is a new press. We will have to commit ourselves here. + this.commit(); + // Ignore addition calls on commit(). + this.ignoreCommitActions = true; +}; var NullTargetHandler = function(target, app) { DefaultTargetHandler.apply(this, arguments); @@ -109,6 +116,12 @@ var CandidateSelectionTargetHandler = function(target, app) { CandidateSelectionTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); CandidateSelectionTargetHandler.prototype.commit = function() { + if (this.ignoreCommitActions) { + this.app.console.log( + 'CandidateSelectionTargetHandler.commit()::return early'); + return; + } + this.app.candidatePanelManager.hideFullPanel(); // We use the target's data instead of target.text because the @@ -174,6 +187,8 @@ BackspaceTargetHandler.prototype.moveIn = function() { BackspaceTargetHandler.prototype.commit = function() { if (this.ignoreCommitActions) { + this.app.console.log( + 'BackspaceTargetHandler.commit()::return early'); return; } @@ -198,6 +213,12 @@ var CompositeTargetHandler = function(target, app) { CompositeTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); CompositeTargetHandler.prototype.commit = function() { + if (this.ignoreCommitActions) { + this.app.console.log( + 'CompositeTargetHandler.commit()::return early'); + return; + } + // Keys with this attribute set send more than a single character // Like ".com" or "2nd" or (in Catalan) "l·l". var compositeString = this.target.compositeKey; @@ -215,6 +236,12 @@ var PageSwitchingTargetHandler = function(target, app) { PageSwitchingTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); PageSwitchingTargetHandler.prototype.commit = function() { + if (this.ignoreCommitActions) { + this.app.console.log( + 'PageSwitchingTargetHandler.commit()::return early'); + return; + } + var page = this.target.targetPage; this.app.setLayoutPage(page); @@ -229,13 +256,38 @@ PageSwitchingTargetHandler.prototype.commit = function() { var CapsLockTargetHandler = function(target, app) { DefaultTargetHandler.apply(this, arguments); + + this.isPreviouslyUpperCase = undefined; }; CapsLockTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); -CapsLockTargetHandler.prototype.commit = function() { +CapsLockTargetHandler.prototype.isNewTargetActivated = false; +CapsLockTargetHandler.prototype.activate = function() { + this.isPreviouslyUpperCase = this.app.upperCaseStateManager.isUpperCase; + + // Switch to upperCaseLocked state so all combo presses will be upper caps this.app.upperCaseStateManager.switchUpperCaseState({ - isUpperCase: !this.app.upperCaseStateManager.isUpperCase, - isUpperCaseLocked: false + isUpperCaseLocked: true }); + + this.app.feedbackManager.triggerFeedback(this.target); + this.app.visualHighlightManager.show(this.target); +}; +CapsLockTargetHandler.prototype.commit = function() { + if (this.isNewTargetActivated) { + // If the user have ever tap any other keys (i.e. combo keys), + // we should go back to lower case regardless. + this.app.upperCaseStateManager.switchUpperCaseState({ + isUpperCase: false, + isUpperCaseLocked: false + }); + } else { + // Depend on the previous upper case state, single tap should allow user + // switch between upper case and lower case. + this.app.upperCaseStateManager.switchUpperCaseState({ + isUpperCase: !this.isPreviouslyUpperCase, + isUpperCaseLocked: false + }); + } this.app.visualHighlightManager.hide(this.target); }; CapsLockTargetHandler.prototype.doubleTap = function() { @@ -244,6 +296,9 @@ CapsLockTargetHandler.prototype.doubleTap = function() { }); this.app.visualHighlightManager.hide(this.target); }; +CapsLockTargetHandler.prototype.newTargetActivate = function() { + this.isNewTargetActivated = true; +}; var SwitchKeyboardTargetHandler = function(target, app) { DefaultTargetHandler.apply(this, arguments); @@ -273,6 +328,12 @@ var ToggleCandidatePanelTargetHandler = function(target, app) { ToggleCandidatePanelTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); ToggleCandidatePanelTargetHandler.prototype.commit = function() { + if (this.ignoreCommitActions) { + this.app.console.log( + 'ToggleCandidatePanelTargetHandler.commit()::return early'); + return; + } + this.app.candidatePanelManager.toggleFullPanel(); this.app.visualHighlightManager.hide(this.target); @@ -284,6 +345,12 @@ var DismissSuggestionsTargetHandler = function(target, app) { DismissSuggestionsTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); DismissSuggestionsTargetHandler.prototype.commit = function() { + if (this.ignoreCommitActions) { + this.app.console.log( + 'DismissSuggestionsTargetHandler.commit()::return early'); + return; + } + var engine = this.app.inputMethodManager.currentIMEngine; if (typeof engine.dismissSuggestions === 'function') { engine.dismissSuggestions(); diff --git a/apps/keyboard/js/keyboard/target_handlers_manager.js b/apps/keyboard/js/keyboard/target_handlers_manager.js index 9608ef6e51b3..8811e761bd54 100644 --- a/apps/keyboard/js/keyboard/target_handlers_manager.js +++ b/apps/keyboard/js/keyboard/target_handlers_manager.js @@ -39,6 +39,8 @@ TargetHandlersManager.prototype.start = function() { this._callTargetAction.bind(this, 'cancel', false, true); activeTargetsManager.ontargetdoubletapped = this._callTargetAction.bind(this, 'doubleTap', false, true); + activeTargetsManager.onnewtargetwillactivate = + this._callTargetAction.bind(this, 'newTargetActivate', false, false); activeTargetsManager.start(); }; @@ -61,12 +63,15 @@ TargetHandlersManager.prototype.stop = function() { // "longpress" is noticeably an optional step during the life cycle and does // not start or end the handler/active target, so it was not mentioned in the // above list. +// "newTargetActivate" is similar to "longpress", it is an optional step too, +// so it was not mentioned in the above list. newTargetActivate is called on +// the current target(s) when there is a new target is about to be activated. // // Please note that since we are using target (an abstract key object associated // with one DOM element) as the identifier of handlers, we do not assign new -// handler if there are two touches on the same element. Currently that cannot -// happen because of what done in bug 985855, however in the future that will -// change (and these handlers needs to) to adopt bug 985853 (Combo key). +// handler if there are two touches on the same element. If two touches happens +// on one (maybe quite big) button, the same method on the same handler will +// be called again with a console warning. TargetHandlersManager.prototype._callTargetAction = function(action, setHandler, deleteHandler, diff --git a/apps/keyboard/style/keyboard.css b/apps/keyboard/style/keyboard.css index 5d00bc02c8e8..b2b685fa005f 100644 --- a/apps/keyboard/style/keyboard.css +++ b/apps/keyboard/style/keyboard.css @@ -261,12 +261,12 @@ button::-moz-focus-inner { } /* Caps lock case */ -.keyboard-key.special-key.kbr-key-hold > .visual-wrapper > .key-element { +.keyboard-key.special-key.kbr-key-hold:not(.highlighted) > .visual-wrapper > .key-element { color: #00caf2; } /* The underline for caps lock */ -.keyboard-key.kbr-key-hold > .visual-wrapper { +.keyboard-key.kbr-key-hold:not(.highlighted) > .visual-wrapper { box-shadow: 0 0.4rem 0 #00caf2; } diff --git a/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js b/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js index 5c824289c31e..415eb600d32e 100644 --- a/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js +++ b/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js @@ -451,8 +451,9 @@ suite('ActiveTargetsManager', function() { setup(function() { userPressManagerStub.onpressstart(press1, id1); - assert.isTrue(manager.ontargetcommitted.calledWith(press0.target), - 'Commit the first press when the second press starts.'); + assert.isTrue(manager.onnewtargetwillactivate.calledWith(press0.target), + 'Determine commit the first press or not' + + ' when the second press starts.'); assert.isTrue(alternativesCharMenuManagerStub.hide.calledOnce); assert.isTrue( diff --git a/apps/keyboard/test/unit/keyboard/target_handlers_test.js b/apps/keyboard/test/unit/keyboard/target_handlers_test.js index e80194fffcca..add1abf537d4 100644 --- a/apps/keyboard/test/unit/keyboard/target_handlers_test.js +++ b/apps/keyboard/test/unit/keyboard/target_handlers_test.js @@ -727,8 +727,54 @@ suite('target handlers', function() { }); test('activate', function() { - assert.equal(handler.activate, DefaultTargetHandler.prototype.activate, - 'function not overwritten'); + setup(function() { + handler.activate(); + + assert.isTrue(app.feedbackManager.triggerFeedback.calledWith(target)); + assert.isTrue(app.feedbackManager.triggerFeedback.calledOnce); + + assert.isTrue(app.visualHighlightManager.show.calledWith(target)); + assert.isTrue(app.visualHighlightManager.show.calledOnce); + }); + + test('commit', function() { + handler.commit(); + + assert.isTrue(app.upperCaseStateManager.switchUpperCaseState + .calledWith({ + isUpperCase: true, + isUpperCaseLocked: false + })); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + }); + + test('moveOut', function() { + handler.moveOut(); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + }); + + test('cancel', function() { + handler.cancel(); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + }); + + test('doubleTap', function() { + handler.doubleTap(); + + assert.isTrue(app.upperCaseStateManager.switchUpperCaseState + .calledWith({ + isUpperCaseLocked: true + })); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + }); }); test('longPress', function() { From 733f17c2a4a9ac954b89f625f5ec5d154610af8b Mon Sep 17 00:00:00 2001 From: Rudy Lu Date: Wed, 29 Oct 2014 10:54:46 +0800 Subject: [PATCH 2/2] - Amend the test cases. --- .../js/keyboard/active_targets_manager.js | 8 +- .../keyboard/active_targets_manager_test.js | 16 ++-- .../keyboard/target_handlers_manager_test.js | 25 +++++- .../unit/keyboard/target_handlers_test.js | 84 +++++++++---------- 4 files changed, 73 insertions(+), 60 deletions(-) diff --git a/apps/keyboard/js/keyboard/active_targets_manager.js b/apps/keyboard/js/keyboard/active_targets_manager.js index 0c6360f35610..42c2fb25f121 100644 --- a/apps/keyboard/js/keyboard/active_targets_manager.js +++ b/apps/keyboard/js/keyboard/active_targets_manager.js @@ -101,11 +101,11 @@ ActiveTargetsManager.prototype._handlePressStart = function(press, id) { } // Notify current targets about the new touch. - this.activeTargets.forEach(function(target, id) { - if (typeof this.onnewtargetwillactivate === 'function') { + if (typeof this.onnewtargetwillactivate === 'function') { + this.activeTargets.forEach(function(target, id) { this.onnewtargetwillactivate(target); - } - }, this); + }, this); + } var target = press.target; this.activeTargets.set(id, target); diff --git a/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js b/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js index 415eb600d32e..c8b74477e34b 100644 --- a/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js +++ b/apps/keyboard/test/unit/keyboard/active_targets_manager_test.js @@ -38,6 +38,7 @@ suite('ActiveTargetsManager', function() { manager.ontargetcommitted = this.sinon.stub(); manager.ontargetcancelled = this.sinon.stub(); manager.ontargetdoubletapped = this.sinon.stub(); + manager.onnewtargetwillactivate = this.sinon.stub(); manager.start(); assert.isTrue(window.UserPressManager.calledWithNew()); @@ -452,15 +453,12 @@ suite('ActiveTargetsManager', function() { userPressManagerStub.onpressstart(press1, id1); assert.isTrue(manager.onnewtargetwillactivate.calledWith(press0.target), - 'Determine commit the first press or not' + - ' when the second press starts.'); - assert.isTrue(alternativesCharMenuManagerStub.hide.calledOnce); + 'onnewtargetwillactivate called.'); - assert.isTrue( - manager.ontargetactivated.calledWith(press1.target)); + assert.isTrue(manager.ontargetactivated.calledWith(press1.target)); - assert.equal(window.clearTimeout.callCount, 3); - assert.equal(window.setTimeout.callCount, 3); + assert.equal(window.clearTimeout.callCount, 2); + assert.equal(window.setTimeout.callCount, 1); }); test('press end second press, and first press', function() { @@ -471,10 +469,10 @@ suite('ActiveTargetsManager', function() { }; userPressManagerStub.onpressend(pressEnd, id1); - assert.isTrue(alternativesCharMenuManagerStub.hide.calledTwice); + assert.isTrue(alternativesCharMenuManagerStub.hide.calledOnce); assert.isTrue( manager.ontargetcommitted.calledWith(press1.target)); - assert.equal(window.clearTimeout.callCount, 4); + assert.equal(window.clearTimeout.callCount, 3); var pressEnd2 = { target: { diff --git a/apps/keyboard/test/unit/keyboard/target_handlers_manager_test.js b/apps/keyboard/test/unit/keyboard/target_handlers_manager_test.js index 13373cb930bb..1cd20a871b4a 100644 --- a/apps/keyboard/test/unit/keyboard/target_handlers_manager_test.js +++ b/apps/keyboard/test/unit/keyboard/target_handlers_manager_test.js @@ -99,6 +99,12 @@ suite('TargetHandlersManager', function() { assert.isTrue(handlerStub.doubleTap.calledOnce); }); + test('New Target activated', function() { + activeTargetsManagerStub.onnewtargetwillactivate(target); + + assert.isTrue(handlerStub.newTargetActivate.calledOnce); + }); + suite('longPress', function() { setup(function() { activeTargetsManagerStub.ontargetlongpressed(target); @@ -129,6 +135,12 @@ suite('TargetHandlersManager', function() { assert.isTrue(handlerStub.doubleTap.calledOnce); }); + + test('New target activated', function() { + activeTargetsManagerStub.onnewtargetwillactivate(target); + + assert.isTrue(handlerStub.newTargetActivate.calledOnce); + }); }); }); @@ -175,6 +187,12 @@ suite('TargetHandlersManager', function() { assert.isTrue(handlerStub.doubleTap.calledOnce); }); + test('New target activated', function() { + activeTargetsManagerStub.onnewtargetwillactivate(target); + + assert.isTrue(handlerStub.newTargetActivate.calledOnce); + }); + suite('longPress', function() { setup(function() { activeTargetsManagerStub.ontargetlongpressed(target); @@ -205,6 +223,12 @@ suite('TargetHandlersManager', function() { assert.isTrue(handlerStub.doubleTap.calledOnce); }); + + test('New target activated', function() { + activeTargetsManagerStub.onnewtargetwillactivate(target); + + assert.isTrue(handlerStub.newTargetActivate.calledOnce); + }); }); }); @@ -261,7 +285,6 @@ suite('TargetHandlersManager', function() { assert.isTrue(handlerStub.activate.calledOnce); }); - test('SpaceKeyTargetHandler', function() { var target = { keyCode: KeyEvent.DOM_VK_SPACE diff --git a/apps/keyboard/test/unit/keyboard/target_handlers_test.js b/apps/keyboard/test/unit/keyboard/target_handlers_test.js index add1abf537d4..b9672c65465a 100644 --- a/apps/keyboard/test/unit/keyboard/target_handlers_test.js +++ b/apps/keyboard/test/unit/keyboard/target_handlers_test.js @@ -131,6 +131,21 @@ suite('target handlers', function() { assert.isTrue(app.visualHighlightManager.hide.calledOnce); }); + test('new target activated', function() { + handler.newTargetActivate(); + + assert.isTrue( + app.inputMethodManager.currentIMEngine.click.calledWith(99)); + assert.isTrue(app.inputMethodManager.currentIMEngine.click.calledOnce); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + + handler.commit(); + // Won't commit again + assert.isTrue(app.inputMethodManager.currentIMEngine.click.calledOnce); + }); + suite('longPress', function() { setup(function() { handler.longPress(); @@ -722,59 +737,22 @@ suite('target handlers', function() { var target; setup(function() { target = {}; - handler = new CapsLockTargetHandler(target, app); }); test('activate', function() { - setup(function() { - handler.activate(); - - assert.isTrue(app.feedbackManager.triggerFeedback.calledWith(target)); - assert.isTrue(app.feedbackManager.triggerFeedback.calledOnce); - - assert.isTrue(app.visualHighlightManager.show.calledWith(target)); - assert.isTrue(app.visualHighlightManager.show.calledOnce); - }); - - test('commit', function() { - handler.commit(); - - assert.isTrue(app.upperCaseStateManager.switchUpperCaseState - .calledWith({ - isUpperCase: true, - isUpperCaseLocked: false - })); - - assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); - assert.isTrue(app.visualHighlightManager.hide.calledOnce); - }); - - test('moveOut', function() { - handler.moveOut(); - - assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); - assert.isTrue(app.visualHighlightManager.hide.calledOnce); - }); - - test('cancel', function() { - handler.cancel(); - - assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); - assert.isTrue(app.visualHighlightManager.hide.calledOnce); - }); + handler.activate(); - test('doubleTap', function() { - handler.doubleTap(); + assert.isTrue(app.upperCaseStateManager.switchUpperCaseState + .calledWith({ + isUpperCaseLocked: true + })); - assert.isTrue(app.upperCaseStateManager.switchUpperCaseState - .calledWith({ - isUpperCaseLocked: true - })); + assert.isTrue(app.feedbackManager.triggerFeedback.calledWith(target)); + assert.isTrue(app.feedbackManager.triggerFeedback.calledOnce); - assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); - assert.isTrue(app.visualHighlightManager.hide.calledOnce); - }); + assert.isTrue(app.visualHighlightManager.show.calledWith(target)); + assert.isTrue(app.visualHighlightManager.show.calledOnce); }); test('longPress', function() { @@ -804,6 +782,20 @@ suite('target handlers', function() { assert.isTrue(app.visualHighlightManager.hide.calledOnce); }); + test('combo key - activate and then with new arget activated', function() { + handler.newTargetActivate(); + handler.commit(); + + assert.isTrue(app.upperCaseStateManager.switchUpperCaseState + .calledWith({ + isUpperCase: false, + isUpperCaseLocked: false + })); + + assert.isTrue(app.visualHighlightManager.hide.calledWith(target)); + assert.isTrue(app.visualHighlightManager.hide.calledOnce); + }); + test('cancel', function() { assert.equal(handler.cancel, DefaultTargetHandler.prototype.cancel, 'function not overwritten');