Bug 985853 - [Keyboard UX update][User Story] Hold shift to enter upper ... #23456
Conversation
chardis
| this._handlePressEnd(press, id); | ||
| if (!this.app.upperCaseStateManager.isUpperCasePressed) { | ||
| this._handlePressEnd(press, id); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
.... 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.
There was a problem hiding this comment.
Maybe onanotherpresswillactivate since this call should happen before onactivate of another handler?
| @@ -93,7 +94,9 @@ ActiveTargetsManager.prototype._handlePressStart = function(press, id) { | |||
| // All targets before the new touch need to be committed, | |||
There was a problem hiding this comment.
// Notify current targets about the new touch
| @@ -178,6 +185,10 @@ BackspaceTargetHandler.prototype.cancel = function() { | |||
| this.app.visualHighlightManager.hide(this.target); | |||
| }; | |||
|
|
|||
| BackspaceTargetHandler.prototype.anotheractivate = function() { | |||
| this.commit(); | |||
| @@ -65,6 +65,10 @@ DefaultTargetHandler.prototype.cancel = function() { | |||
| DefaultTargetHandler.prototype.doubleTap = function() { | |||
| this.commit(); | |||
| }; | |||
| DefaultTargetHandler.prototype.newTargetActivate = function() { | |||
| // Ignore any action when commit. | |||
| this.ignoreCommitActions = true; | |||
There was a problem hiding this comment.
I think we need this.commit() above this line?
| @@ -244,11 +249,17 @@ var CapsLockTargetHandler = function(target, app) { | |||
| DefaultTargetHandler.apply(this, arguments); | |||
| }; | |||
| CapsLockTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); | |||
| CapsLockTargetHandler.prototype.NEW_TARGET_ACTIVATED = false; | |||
There was a problem hiding this comment.
Since this is not a constant, let's use newTargetActivated instead of all-caps NEW_TARGET_ACTIVATED.
| @@ -244,11 +249,18 @@ var CapsLockTargetHandler = function(target, app) { | |||
| DefaultTargetHandler.apply(this, arguments); | |||
| }; | |||
| CapsLockTargetHandler.prototype = Object.create(DefaultTargetHandler.prototype); | |||
| CapsLockTargetHandler.prototype.newTargetActivated = false; | |||
There was a problem hiding this comment.
Sorry, it turned out newTargetActivated is a bad name.
How about isNewTargetActivated?
|
Vou fixar só de manhã!!! |
|
Oh, ops!!! I will fix them today. Sure in 6:00 am. |
…er case characters
|
This pull request has been closed due to tree stability issues. Please rebase and re-open the pull request if you still need to land this. Ensure the gaia-try run is green before landing. Sorry for any inconvenience. |
Fixed Bug 985853 - [Keyboard UX update][User Story] Hold shift to enter upper case characters