985853 - [Keyboard UX update][User Story] Hold shift to enter upper case characters

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P2)

Description

[Bruce Huang [:bhuang] (inactive)]

As a user, I want to be able to enter upper case characters by holding shift, but revert back to lowercase when shift is not pressed.  This allows easier entry of single uppercase characters or a short string of uppercase characters.

Acceptance:
Text should be in upper text when holding down the shift key.
Letting go of shift key reverts to lowercase.

Refer to p.9 of UX spec in bug 983043

Comment 1

[Omega Feng [:Omega] [:馮於懋]]

See bug 983043 for the UX spec update.

Comment 2

[William Hsu [:whsu]]

Add the test cases.
- https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2630

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The behavior described in https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8452129#5 ("Combo Key") is already fulfilled with bug 985855 without any new patches, although I talk to Omega and he found some more quirks with the shift/caps key which need to be addressed, for example, press and hold shift and type multiple keys.

Omega, could you update the spec and merge the "Combo Key" description with the state diagram on the left to accompany all behavior? Let's put everything in one state diagram to make sure there isn't conflicting behavior.

Comment 4

[Omega Feng [:Omega] [:馮於懋]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #3)
> Omega, could you update the spec and merge the "Combo Key" description with
> the state diagram on the left to accompany all behavior? Let's put
> everything in one state diagram to make sure there isn't conflicting
> behavior.

Sure, see bug 983043 for the latest UX spec.

Comment 5

[DongShengXue]

Attachment: Patch V1 - pull request 23456

apps/keyboard/js/keyboard/upper_case_state_manager.js:
Add isUpperCasePressed and isUpperCaseCombo.

apps/keyboard/js/keyboard/active_targets_manager.js:
Modified ActiveTargetsManager.prototype._handlePressStart and ActiveTargetsManager.prototype._handlePressEnd to set isUpperCasePressed and isUpperCaseCombo.

apps/keyboard/js/render.js
if state.isUpperCaseCombo != true, then add 'kbr-key-active' class and remove 'kbr-key-hold' class

Hi Rudy, could you help review this? Thanks a lot.

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8481101 [details] [review]
Patch V1 - pull request 23456

Hi DongSheng, thanks for this patch.

Several comments,

 1. We should be change the uppercase state in the target_handlers.js, instead of the active_target_manager.js.
 2. Please also address the unit test/linter failure and add new tests for this feature.

--  
Please help flag me again if you have any updates.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: WIP - for elaborating my suggestions

This is a counter patch to state my points clearly.

It does not take care of the uppercase state transition after the combo key behavior happened, but it basically realize this feature, and most of the changes live in target_handlers.

DongSheng, mind to take a look if see this fits our need?

Comment 8

[DongShengXue]

(In reply to Rudy Lu [:rudyl] from comment #6)
> Comment on attachment 8481101 [details] [review]
> Patch V1 - pull request 23456
> 
> Hi DongSheng, thanks for this patch.
> 
> Several comments,
> 
>  1. We should be change the uppercase state in the target_handlers.js,
> instead of the active_target_manager.js.

Moved code from active_target_manager.js to target_handlers.js

>  2. Please also address the unit test/linter failure and add new tests for
> this feature.
Run gjslint before commit
Add unit test case:
  apps/keyboard/test/unit/keyboard/active_targets_manager_test.js
  apps/keyboard/test/unit/keyboard/target_handlers_test.js
  apps/keyboard/test/unit/keyboard/upper_case_state_manager_test.js

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

SongSheng, please set feedback to me for your next patch.

I don't agree with Rudy on his feedback -- I don't want to introduce any more state (e.g. |isUpperCasePressed|, |isUpperCaseCombo|) into the UpperCaseStateManager _at all_. The proper approach of this bug is to switch the layout to upper case (and maybe re-render) when the shift key is pressed down, and register to switch back if there is another key being pressed during that time. Please don't make ActiveTargetsManager depend on upper case state -- it should be a simple dispatcher w/o considering anything complex in the keyboard elsewhere.

We might want to deal with bug 1044525 first before working on this one, since the two Shift keys might be different DOM elements, if |secondLayout| is true for the layout page.

Comment 11

[DongShengXue]

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #10)
Tim, I I modified the patch as you suggested. Next I also need to modify UI. Thanks for your help.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8481101 [details] [review]
Patch V1 - pull request 23456

I have send a new series of feedback on Github. You didn't not implement completely what I suggested. Please address the feedbacks and set feedback? to me again when it's done. Thanks!

Comment 13

[DongShengXue]

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #12)
> Comment on attachment 8481101 [details] [review]
> Patch V1 - pull request 23456
> 
I fixed the code refer to your feedbacks, except apps/keyboard/js/keyboard/target_handlers.js：
+CapsLockTargetHandler.prototype.newTargetActivate = function() {
+ // Need a new state here, like isUpperCaseCombo,
+ // otherwise the UI is not correct ?
+ this.app.upperCaseStateManager.switchUpperCaseState({
+ isUpperCase: true
+ });
I think isUpperCase is not enough to realize the UI design.
+ this.app.visualHighlightManager.hide(this.target);
+};

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

You mean this comment?

https://github.com/mozilla-b2g/gaia/pull/23456#discussion-diff-18081719

First, please see if my suggestion work in terms of behavior. After that, to fix the UI issue you mentioned, use VisualHighlightManager to tell render if the key is currently pressed or not.

You have not complete all the required changes yet.

Comment 15

[DongShengXue]

(In reply to Tim Guan-tin Chien [:timdream] (OOO) (MoCo-TPE) (please ni?) from comment #14)

Hi Tim, I update the patch. 
Please help review again. Thanks

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

DongSheng,

I amend a patch and submit a pull request to you.

https://github.com/chardis/gaia/pull/1/files

Please squash it on top of your patch and complete the patch with unit tests, after the holiday.

Comment 17

[DongShengXue]

Attachment: Patch V1 - pull request 24966

Comment 18

[DongShengXue]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #16)
Hi Tim,

I have merged with your patch. And I will communicate with UX designer.
Thanks.

Comment 19

[Omega Feng [:Omega] [:馮於懋]]

Comment on attachment 8502200 [details] [review]
Patch V1 - pull request 24966

LGTM, thanks!
However, I found there is a behavior different from the UX spec (v1.6, p.6). In the spec, when Shift is ON, switching to symbol panel and then switching back to default panel should turn OFF the shift. Not sure it should be implemented in this patch or a separated one.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8502200 [details] [review]
Patch V1 - pull request 24966

You need to fix/add some unit tests! Preferably some int test if possible.

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

DongSheng, I realized we didn't address this issue:

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> We might want to deal with bug 1044525 first before working on this one,
> since the two Shift keys might be different DOM elements, if |secondLayout|
> is true for the layout page.

Could you please try your patch on for example, Korean layout, to see if it works with secondLayout=true? You would need to rebase your patch to make sure bug 1075970 is included too.

If there is a problem with the current patch, I would say we unfortunately need to again introduce one-to-many mapping between the target object and DOM elements specifically for the shift key, and have render.js change the rendering of every shift key in every layout altogether.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8502200 [details] [review]
Patch V1 - pull request 24966

Removing the feedback per comment above.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am going to rebase this patch and land it, since bug 1085359 is fixed.

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8502200 [details] [review]
Patch V1 - pull request 24966

Actually --- I can't r+ this because lack of tests.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/timdream/gaia/commit/6e512f009925bd6c526785e90ecf778a6b2a78b4

Talk to Rudy and he will take over and land this before bug 1040603.

Comment 26

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I am starting to look at the existing patch and see what I need to do to land this.
However, I found a UI behavior that I want to confirm with Omega first:
  - If we trigger this shift combo, whether we should not change the state of uppercase.
  That is to say, when the keyboard is in uppercase mode, then trigger shift combo 
  -> the keyboard would change to lowercase mode after you release the shift key.

Comment 27

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Just offline discussed this with Omega, and he said he is fine with the above behavior, see comment 26.
That is we could change the shift state after shift combo.

Comment 28

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1.1

This patch is to add some test cases based on the patch by DongSheng.
Please see the last commit for the changes I made.

Tim, could you please help review this?
Thank you.

--
For moving out AlternativeCharMenu related code out of active_targets_manager.js, I plan to do it in another bug.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8511876 [details] [review]
Patch V1.1

Thank you very much for the help!

Comment 30

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/548b8981f3363fe527b1cb660c0895c6e798fb16

CI result: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=4f14f71bc0ba

--
DongSheng, thanks for working on this!
Tim, thanks for the review.

Comment 31

[Kevin Grandon :kgrandon]

I saw that this commit was landed: https://github.com/mozilla-b2g/gaia/commit/733f17c2a4a9ac954b89f625f5ec5d154610af8b, which came from this pull request:  https://github.com/mozilla-b2g/gaia/pull/25528

It seems that the commit had neither the bug number or reviewer listed. Because of this, it has been backed out. This is necessary information to have in the commit message to track down future issues or regressions.

Backout: https://github.com/mozilla-b2g/gaia/commit/a02558626a2d2248c13427b83fd5247eb72028ef

I've also re-landed this follow-up here with the proper commit format, so you should be good: https://github.com/mozilla-b2g/gaia/commit/9a708bae935a1574c9a504ab1d25bd4098e98446

Comment 32

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Kevin, thanks for your help to handle this.
I don't know that a follow-up commit would need to add bug number info as they are contained in the same merge commit.

Comment 33

[howie [:howie]]

Remove the feature b2g tag to match the new v2.2 scope.