Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Bug 985853 - [Keyboard UX update][User Story] Hold shift to enter upper ... #23456

Closed
wants to merge 1 commit into from
Closed

Bug 985853 - [Keyboard UX update][User Story] Hold shift to enter upper ... #23456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions apps/keyboard/js/keyboard/active_targets_manager.js
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what we can do here is remove this code, and introduce an interface between targetHandlersManager and targetHandler to have each of the target handler to decide whether or not it should be committed when a new target is activated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.

 DefaultTargetHandler.prototype.COMMIT_WITH_ANOTHER_PRESS = true;

and

CapsLockTargetHandler.prototype.COMMIT_WITH_ANOTHER_PRESS = false;

and have TargetHandlersManager at ontargetactivated do things differently against the existing targets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.... ok. After giving another thought on this, we should still remove the code here in active target manager, but I think we shouldn't need a COMMIT_WITH_ANOTHER_PRESS flag on handlers. What we should do is introduce a onanotherpressactivated callback to each of the handlers and have the handlers to commit/ignore on it's own. Default handler, for example, could call it's own this.commit() and then set this.ignoreCommitActions = true to itself.

Does that make sense more? I just try to find a way to generalize *Managers (bad name, I know) as much as possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe onanotherpresswillactivate since this call should happen before onactivate of another handler?

}, this);

var target = press.target;
Expand Down
33 changes: 31 additions & 2 deletions apps/keyboard/js/keyboard/target_handlers.js
View file
Open in desktop
@@ -1,6 +1,6 @@
'use strict';

/* global KeyEvent */
/* global KeyEvent, IMERender */

(function(exports) {

Expand Down Expand Up @@ -79,6 +79,11 @@ DefaultTargetHandler.prototype.doubleTap = function() {
this.app.console.log('DefaultTargetHandler.doubleTap()');
this.commit();
};
DefaultTargetHandler.prototype.newTargetActivate = function() {
this.commit();
// Ignore any action when commit.
this.ignoreCommitActions = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need this.commit() above this line?

};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the target handler doesn't end it's life when this function is called, you need to set

this.ignoreCommitActions = true;

here.


var NullTargetHandler = function(target, app) {
DefaultTargetHandler.apply(this, arguments);
Expand Down Expand Up @@ -231,11 +236,28 @@ var CapsLockTargetHandler = function(target, app) {
DefaultTargetHandler.apply(this, arguments);
};
CapsLockTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype);
CapsLockTargetHandler.prototype.capsLockState = 'none';
CapsLockTargetHandler.prototype.newTargetActivated = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it turned out newTargetActivated is a bad name.

How about isNewTargetActivated?

CapsLockTargetHandler.prototype.activate = function() {
if (this.app.upperCaseStateManager.isUpperCaseLocked) {
this.capsLockState = 'upperCaseLocked';
} else if (this.app.upperCaseStateManager.isUpperCase) {
this.capsLockState = 'upperCase';
} else {
this.capsLockState = 'none';
}
this.app.feedbackManager.triggerFeedback(this.target);
this.app.visualHighlightManager.show(this.target);
};
CapsLockTargetHandler.prototype.commit = function() {
// If hold shift to enter upper case, set isUpperCase false when finished
this.app.upperCaseStateManager.switchUpperCaseState({
isUpperCase: !this.app.upperCaseStateManager.isUpperCase,
isUpperCase: this.newTargetActivated ? false :
!this.app.upperCaseStateManager.isUpperCase,
isUpperCaseLocked: false
});
this.newTargetActivated = false;
this.capsLockState = 'none';
this.app.visualHighlightManager.hide(this.target);
};
CapsLockTargetHandler.prototype.doubleTap = function() {
Expand All @@ -244,6 +266,13 @@ CapsLockTargetHandler.prototype.doubleTap = function() {
});
this.app.visualHighlightManager.hide(this.target);
};
CapsLockTargetHandler.prototype.newTargetActivate = function() {
this.newTargetActivated = true;
this.app.upperCaseStateManager.switchUpperCaseState({
isUpperCaseLocked: true
});
IMERender.highlightKey(this.target, { capsLockState: this.capsLockState });
};

var SwitchKeyboardTargetHandler = function(target, app) {
DefaultTargetHandler.apply(this, arguments);
Expand Down
5 changes: 5 additions & 0 deletions apps/keyboard/js/keyboard/target_handlers_manager.js
View file
Open in desktop
Expand Up @@ -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();
};

Expand All @@ -61,6 +63,9 @@ 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please leave a brief comment on when newTargetActivate is called.

// so it was not mentioned in the above list. When newTargetActivate is called,
// isUpperCase will be set to true, and hold the icon's background.
//
// 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
Expand Down
15 changes: 15 additions & 0 deletions apps/keyboard/js/render.js
View file
Open in desktop
Expand Up @@ -318,11 +318,26 @@ var IMERender = (function() {
if (!options.showUpperCase) {
keyElem.classList.add('lowercase');
}
// Show highlight locked background.
switch (options.capsLockState) {
case 'upperCaseLocked':
keyElem.classList.add('highlight-locked-uppercaselocked');
break;
case 'upperCase':
keyElem.classList.add('highlight-locked-uppercase');
break;
case 'none':
keyElem.classList.add('highlight-locked-none');
break;
}
};

// Unhighlight a key
var unHighlightKey = function kr_unHighlightKey(key) {
var keyElem = targetObjDomMap.get(key);
keyElem.classList.remove('highlight-locked-uppercaselocked');
keyElem.classList.remove('highlight-locked-uppercase');
keyElem.classList.remove('highlight-locked-none');
keyElem.classList.remove('highlighted');
keyElem.classList.remove('lowercase');
};
Expand Down
48 changes: 42 additions & 6 deletions apps/keyboard/style/keyboard.css
View file
Open in desktop
Expand Up @@ -179,19 +179,55 @@ button::-moz-focus-inner {
left: 0;
}

/* Rules for hold shift to enter upper case */
.keyboard-key.highlight-locked-uppercaselocked {
background-color: #4a5255;
}

.keyboard-key.highlight-locked-uppercaselocked > .visual-wrapper {
box-shadow: 0 0.4rem 0 #00caf2;
}

.keyboard-key.highlight-locked-none > .visual-wrapper > .key-element {
color: #00b8d6;
}

.keyboard-key.highlight-locked-uppercase {
background-color: #00caf2;
}

.keyboard-key.highlight-locked-uppercase > .visual-wrapper {
box-shadow: 0 0 0 #00caf2;
}

.keyboard-key.highlight-locked-uppercase > .visual-wrapper > .key-element {
color: #00b8d6;
border-right-color: transparent;
}

.keyboard-key.highlight-locked-none > .visual-wrapper {
box-shadow: 0 0 0 #00caf2;
}

.keyboard-key.highlight-locked-none > .visual-wrapper > .key-element {
color: #00b8d6;
}

/* Special keys */
.keyboard-key.special-key > .visual-wrapper > .key-element {
top: 0;
right: 0;
bottom: 0;
left: 0;
color: #abb0b1;
font: 500 1.5rem/4.3rem 'Keyboard Symbols', sans-serif;
}

.keyboard-key.special-key:not([class*='highlight-locked']) > .visual-wrapper > .key-element {
color: #abb0b1;
}

/* Highlight for special keys */
.keyboard-key.special-key.highlighted > .visual-wrapper > .key-element {
.keyboard-key.special-key.highlighted:not([class*='highlight-locked']) > .visual-wrapper > .key-element {
color: #00b8d6;
}

Expand Down Expand Up @@ -243,22 +279,22 @@ button::-moz-focus-inner {
/* Key states */

/* Active key, used for shift key - uppercase */
.keyboard-key.kbr-key-active {
.keyboard-key.kbr-key-active:not([class*='highlight-locked']) {
background-color: #00caf2;
}

.keyboard-key.kbr-key-active > .visual-wrapper > .key-element {
.keyboard-key.kbr-key-active:not([class*='highlight-locked']) > .visual-wrapper > .key-element {
color: #fff;
border-right-color: transparent;
}

/* Caps lock case */
.keyboard-key.special-key.kbr-key-hold > .visual-wrapper > .key-element {
.keyboard-key.special-key.kbr-key-hold:not([class*='highlight-locked']) > .visual-wrapper > .key-element {
color: #00caf2;
}

/* The underline for caps lock */
.keyboard-key.kbr-key-hold > .visual-wrapper {
.keyboard-key.kbr-key-hold:not([class*='highlight-locked']) > .visual-wrapper {
box-shadow: 0 0.4rem 0 #00caf2;
}

Expand Down