In a search input, change Enter key's icon to a search icon.

I am interested in working on this ticket. Where do I start?

Hi bharad, 1. First of all, I think we should look at getBasicInputType() in this file, https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/keyboard_app.js so that we could render a separate keyboard layout page for type=search. 2. And then we could add some logic in _updateModifiedLayout() in, https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/layout_manager.js So, that it would render the [Enter] into "search" icon, by adding CSS class to the key. 3. And then we should add some CSS rule for the CSS class we added in step 2. -- Please note that this may need some understanding of JS code and dive into keyboard code. Let me know if you need more help or want to look at CSS change only. Thanks.

Rudy, Thanks for the pointers. Here is a first stab at the problem. Comments welcome. https://github.com/t20/gaia/commit/4d56665e8a91f668b04ddfb71906a7ab34b6c4ea Can you please point me to directions on adding a test for this feature? Would this be in test/unit/keyboard/layout_manager_test.js

Thanks for showing us your progress. Looks good to me. The layout_manager part is not ready yet, you may wait for my patch in bug 1020779, from that patch, you would get _findKey() function to locate the Enter key and you could add CSS class to it. -- BTW, next time you could try to use github's pull request function, and add an attachment (see bug 1020779 as an example) in a bug which will link to that pull request. This will make your work clear on the bug, and facilitate the review progress. :)

BTW, for the assets of the search icon, you could find them in attachment 8455165 [details].

I will wait for the patch to land before landing bug 1040598.

Comment on attachment 8466585 [details] Patch V1 Hi bharad, Thanks for the update, and sorry that did not notice this patch earlier, please be informed that I'll only get notified if you set review or feedback flag to me. After bug 1020779 is landed, we would also look for [Enter] key to adjust its size, you may try to adapt your changes to base on that patch. Thanks.

BTW, have you ever written unit tests? I would suggest you give it a try to add some test cases to, apps/keyboard/test/unit/keyboard/layout_manager_test.js If you're not familiar with that, don't worry, I'll take care of that. Thank you.

Let me steal the bug as the original assignee hasn't responded for a week and his patch is half-baked (doesn't actually change the Enter key). I will change assignee once I have a patch.

Sorry for the late response. I just updated my pull request with the new commit. I am having trouble running my test cases. I tried getting help using IRC over the weekend on #gaia and #b2g rooms, but did not receive a response.

Can you please provide more info on how I would go about adjusting the size? Thanks.

Rudy ^^^ Hi bharad, if you want us to make clarification and/or provide assistance, please make sure you set the needinfo flag. Thanks!

bharad, 1. For Running Gaia unit test, you could take this as an guideline, https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests 2. And you may need to rebase your work onto bug 1020779, that has covered the resizing of Enter key, so you should need to cover the CSS styling here. (But please make sure you don't do findKey twice in order to accomplish this).

I got the test cases running, but they are failing now. I am working on it. I ll keep you posted.

Comment on attachment 8466585 [details] Patch V1 I rebased the code, added test cases and got them running. Can you help me fix this error? Thanks a lot, B [keyboard-test/unit/keyboard/layout_manager_test.js] type=search, without IME switching ‣ Error: expected { keys: [ [ [Object] ], [ [Object], [Object], [Object] ] ], layoutName: 'spaceLayout', alternativeLayoutName: '' } to equal { layoutName: 'spaceLayout', alternativeLayoutName: '', keys: [ [ [Object] ], [ [Object], [Object], [Object], [Object], [Object] ] ] } at chaiAssert (app://keyboard.gaiamobile.org/common/test/helper.js:31:1) at eql (app://keyboard.gaiamobile.org/common/vendor/chai/chai.js:526:1) at deepEqual (app://keyboard.gaiamobile.org/common/vendor/chai/chai.js:1331:5) at (anonymous) (app://keyboard.gaiamobile.org/test/unit/keyboard/layout_manager_test.js:965:9)

Comment on attachment 8466585 [details] Patch V1 It seems your code was not based on the latest master, so I could not check the failed test cases. Would you mind rebasing your patch again? You could skip the unit tests first if you have trouble with that. BTW, the search icon was not placed correctly per my test, it should replace the [Enter] mark instead of being put under the [Enter] mark. Could you please take a look or need me to take over from here?

I have rebased and updated the pull request. Please take over this pull request from here. Thanks a lot for your help. My first attempt at a patch for a mozilla project.

Comment on attachment 8466585 [details] Patch V1 (In reply to bharad from comment #18) > I have rebased and updated the pull request. Please take over this pull > request from here. > Thanks a lot for your help. My first attempt at a patch for a mozilla > project. bharad - Thank you so much for the patch here! If we can get your name as the author on this commit and landed we will definitely try to. Rudy - Can you run through the latest patch and see if anything is still missing and what we need to do?

Hi bharad, Again, thanks for your awesome work! Rudy and I discussed and we found only minor hiccups that needed tweaking. I made some fine tunes based upon your patch and it is ready for testing & reviewing in preparation to land. It will definitely mark your first contribution to the Gaia project once tests pass and Rudy grants review+ on the patch. Rudy: please feel free to ask me for a demo. However, I'm having troubles with Nightly (even system app doesn't launch and test-agent can't do any unit tests) atm, so I'm not 100% sure that unit tests are good -- but since bharad's original patch looks good on TBPL, I don't think we have much to worry about this.

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) Hi John, Thanks for your help to take over the remaining work to finish up this issue. This patch looks quite good to me, but I have to confirm one behavior with our UX that if we need to show 'search icon' when switching to the symbol panel.

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) @John, Please also use search icon in both symbol panels, thanks! @Carol, please review the visual parts, thanks

(In reply to Omega Feng [:Omega] [:馮於懋] (PTO until 8/22) from comment #22) > @John, Please also use search icon in both symbol panels, thanks! okay, set NI to myself for reminding.

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) Hi rudy, As discussed earlier offline, I modified the unit tests to account for prototyped enter_key object. Please take a moment to see if you're okay with the concept of such modification: https://github.com/mnjul/gaia/commit/f84f976613392c396890ec1b96f7f31b3e6e69ae

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) I agree with this modification, but see my comment here, https://github.com/mozilla-b2g/gaia/pull/23196/files#r16644244

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) Hi John, As our offline discussion, the search icon should shift left for 0.1 rem and it looks fine visually. Thanks!

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) (In reply to Omega Feng [:Omega] [:馮於懋] (PTO until 8/22) from comment #22) > Comment on attachment 8477347 [details] > Fine-Tuned Patch (PR @ GitHub) > > @John, Please also use search icon in both symbol panels, thanks! Hi Omega, please confirm that the icon now displays with symbol panels and set ui-review+ accordingly, thanks! Rudy: I got rid of the constant index. Please also check my logic to display search icon in symbol panels -- I think I'm adding complex logic to the inference of modifyType but there doesn't seem a (much) better way to accomplish what we want. After this round I'll be squashing & rebasing and we'll be ready to get bharad's contribution into Gaia ;)

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) Looks good. Thank you.

Comment on attachment 8477347 [details] Fine-Tuned Patch (PR @ GitHub) Confirmed. It looks good.

At TBPL for final landing preparation: https://tbpl.mozilla.org/?rev=630cf3d7117d018e8b1b02197d1474e4163dfe58&tree=Gaia-Try

bharad, super awesome! Your contribution has now landed into the Gaia repository, in commit 964b32c7cd208683bea91aedcbc2a37c61b94073. -- Master: https://github.com/mozilla-b2g/gaia/commit/c61e6d2d5318ffbadf0fad0d6f56611a54cb737f ( FYI there was another TBPL try after comment 30 https://tbpl.mozilla.org/?rev=06371245718b77f912b005f9d3ff70aedb3573eb&tree=Gaia-Try )