Closed
Bug 985853
Opened 10 years ago
Closed 10 years ago
[Keyboard UX update][User Story] Hold shift to enter upper case characters
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect, P2)
Tracking
(b2g-v2.2 fixed)
RESOLVED
FIXED
2.1 S8 (7Nov)
Tracking | Status | |
---|---|---|
b2g-v2.2 | --- | fixed |
People
(Reporter: bhuang, Assigned: rudyl)
References
Details
(Whiteboard: [ucid:SystemPlatform55, ft:system-platform])
Attachments
(3 files, 1 obsolete file)
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
Reporter | ||
Updated•10 years ago
|
Whiteboard: [ucid:SystemPlatform55, 1.5, ft:system-platform]
Comment 1•10 years ago
|
||
See bug 983043 for the UX spec update.
Comment 2•10 years ago
|
||
Add the test cases. - https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2630
Updated•10 years ago
|
Flags: in-moztrap+
Updated•10 years ago
|
Whiteboard: [ucid:SystemPlatform55, 1.5, ft:system-platform] → [ucid:SystemPlatform55, 2.0, ft:system-platform]
Updated•10 years ago
|
Whiteboard: [ucid:SystemPlatform55, 2.0, ft:system-platform] → [ucid:SystemPlatform55, ft:system-platform]
Comment 3•10 years ago
|
||
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.
Flags: needinfo?(ofeng)
Comment 4•10 years ago
|
||
(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.
Flags: needinfo?(ofeng)
Assignee | ||
Updated•10 years ago
|
Priority: -- → P2
Updated•10 years ago
|
Assignee: nobody → dxue
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
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.
Attachment #8481101 -
Flags: review?(rlu)
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8481101 -
Flags: review?(rlu) → feedback-
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.
Flags: needinfo?(dxue)
Comment 11•10 years ago
|
||
(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.
Flags: needinfo?(dxue)
Comment 12•10 years ago
|
||
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!
Attachment #8481101 -
Flags: feedback-
Flags: needinfo?(dxue)
Comment 13•10 years ago
|
||
(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); +};
Flags: needinfo?(dxue)
Comment 14•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.
Flags: needinfo?(dxue)
Comment 17•10 years ago
|
||
Attachment #8481101 -
Attachment is obsolete: true
Attachment #8502200 -
Flags: review?(timdream)
Flags: needinfo?(dxue)
Comment 18•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8502200 -
Flags: ui-review?(ofeng)
Comment 19•10 years ago
|
||
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.
Attachment #8502200 -
Flags: ui-review?(ofeng) → ui-review+
Comment 20•10 years ago
|
||
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.
Attachment #8502200 -
Flags: review?(timdream) → feedback+
Comment 21•10 years ago
|
||
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.
Flags: needinfo?(dxue)
Comment 22•10 years ago
|
||
Comment on attachment 8502200 [details] [review] Patch V1 - pull request 24966 Removing the feedback per comment above.
Attachment #8502200 -
Flags: feedback+
Comment 23•10 years ago
|
||
I am going to rebase this patch and land it, since bug 1085359 is fixed.
Flags: needinfo?(dxue)
Updated•10 years ago
|
Attachment #8502200 -
Flags: review+
Comment 24•10 years ago
|
||
Comment on attachment 8502200 [details] [review] Patch V1 - pull request 24966 Actually --- I can't r+ this because lack of tests.
Attachment #8502200 -
Flags: review+
Comment 25•10 years ago
|
||
https://github.com/timdream/gaia/commit/6e512f009925bd6c526785e90ecf778a6b2a78b4 Talk to Rudy and he will take over and land this before bug 1040603.
Assignee | ||
Comment 26•10 years ago
|
||
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.
Flags: needinfo?(rlu)
Assignee | ||
Comment 27•10 years ago
|
||
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.
Assignee | ||
Comment 28•10 years ago
|
||
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.
Attachment #8511876 -
Flags: review?(timdream)
Comment 29•10 years ago
|
||
Comment on attachment 8511876 [details] [review] Patch V1.1 Thank you very much for the help!
Attachment #8511876 -
Flags: review?(timdream) → review+
Assignee | ||
Comment 30•10 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.2:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
Comment 31•10 years ago
|
||
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
Assignee | ||
Comment 32•10 years ago
|
||
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.
Updated•10 years ago
|
feature-b2g: --- → 2.2+
Comment 33•10 years ago
|
||
Remove the feature b2g tag to match the new v2.2 scope.
feature-b2g: 2.2+ → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•