User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 Build ID: 20140911151253 Steps to reproduce: There should be a vibration in addition to the sound when user taps number keys in telephony application MUT HW: FLAME MUT SW: Boot2Gecko2.2.0.0-prerelease Buildidentifier: 20140905040204 (MUT = Mobile Under Test) Actual results: Vibrating touch feedback missing when dialing a number.

What's the UX point of view on that behavior?

Hi, Currently we have two vibrate switches on our system, one is in keyboard settings (vibrate when typing) and the other is in Sound settings (vibrate with ringtone). I think both of them can't be linked to the tactile feedback when touching the screen. On android, other than these two options, they provide a "vibrate on touch" which is related to the touch feedback on pressing buttons and the dialerpad is included in it. I'd suggest to adopt this solution, but we may need to add a switch in Settings (I'd say, in Sound menu). ni? Wilfred for feature proposal and Jenny for Settings UX. Thanks!

Can't we use the keyboard setting? It is already used for the keypad for SIM unlock screen so that would make sense to me.

(In reply to Anthony Ricaud (:rik) from comment #3) > Can't we use the keyboard setting? It is already used for the keypad for SIM > unlock screen so that would make sense to me. I agree. If we create a new setting we'll have to add strings and we're way past string freeze for 2.1

[Blocking Requested - why for this release]: Feature requested by partner for 2.1.

This is considered as a nice to have triage wouldn't block on that. Please ask for approval.

Removing the flag as per comment 6.

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 A few nits on the PR. Also, did you test this on Mulet? I wonder if navigator.vibrate exists there. The keyboard app has a guard against this if it doesn't.

I've addressed the review comments. But before landing this, I've quickly talked with Carrie and it seems we have an issue with re-using the same setting. If the user installs another keyboard, there is a concern that they couldn't disable vibration. I just tried installing another keyboard and of course, the Gaia keyboard settings are still accessible so I'm not sure what the concern is here. Flagging Carrie for clarification.

Hi Anthony, the concern is that user may not know if he wants to have this tactile feedback on his Dialerpad/ Pinpad and actually he is using other 3rd party keyboard, he should go to fxos keyboard settings to switch it on. It's really difficult to connect them together. I've asked some people around and they said they will go "Dialer settings" or "Sound settings". Thanks!

for now I think its fine where it is for 2.1 - perhaps in the future we canrethink where all of the settings for keyboard is before altering some settings.

Hi all, After UX internal discussion, we will add dial pad tactile feedback in Settings > Sound. Tactile/sound feedback for any other type of keypad is already included in Settings > Keyboard > Built-in keyboard settings. Thanks!

Please fix for 2.1 This impacts first time experience Users coming from feature phones are accustomed to tactile feedback. Vibration is the way to give them that on a touch screen. Vibration shall be enabled as default!

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 Carrie: I've added the lockscreen vibration. Tim: I've tried to write new tests for this change but couldn't get anything to work. If you have any hint for me, that would be great!

(In reply to Jenny Lee from comment #13) > Hi all, > After UX internal discussion, we will add dial pad tactile feedback in > Settings > Sound. Tactile/sound feedback for any other type of keypad is > already included in Settings > Keyboard > Built-in keyboard settings. Thanks! Could you open another bug for this? We can't do that for 2.1 because it requires new strings.

Triage group reviewed and decided blocking- because it doesn't meet any of the criteria we use in order to ship releases on time: https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines Also, it's a new feature after feature freeze, with new strings after string freeze, and it also changes behavior existing users are used to. Recommend filing a new bug for doing this without string changes (as rik suggests) and also a bug to make this build-configurable so that partners can decide what the default should be in their releases.

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 It feels good. Great job! Thanks!

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 I was going to r+ the comment add in Keyboard app but I realized that's not what you asked. If you are asking for tests in lockscreen.js let's ask :snowmantw.

I have left my opinions on the GitHub page. I don't know whether them would be fixed or discussed, since I'm not the reviewer.

(In reply to Anthony Ricaud (:rik) from comment #16) > (In reply to Jenny Lee from comment #13) > > Hi all, > > After UX internal discussion, we will add dial pad tactile feedback in > > Settings > Sound. Tactile/sound feedback for any other type of keypad is > > already included in Settings > Keyboard > Built-in keyboard settings. Thanks! > Could you open another bug for this? We can't do that for 2.1 because it > requires new strings. Hi Anthony, I will upload the spec including the change mentioned here to sound meta Bug 1068219 once it's ready. Thanks!

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #20) > I have left my opinions on the GitHub page. I don't know whether them would > be fixed or discussed, since I'm not the reviewer. > If we require every modification in System app should come with an unit test, this rule is not be > satisfied in this patch (add/modify lockscreen related unit tests in System app). I want to provide tests but wasn't able to write any working ones. I was looking for help on this.

I don't know where did you failed. But if there are any information you can provide I may be able to help you, for the LockScreen part.

I can't get the lockscreen to respond to click events :( See the attached patch.

I notice you put dispatchEvent in unit test. We usually directly call 'handleEvent' to handle the specific event, instead of following the dispatching and handling path. Since for the component in unit test, the most important thing is to make sure the event has been handled correctly, rather than to check if dispatching API works. At least this is what I have seen in the unit tests. I'm not saying dispatching failure isn't important, but for this patch and the test I think to test with handleEvent is enough. If we want to fix dispatching issue, we could fire another bug.

Using real events has a value because you're also testing that the registration of event listeners worked. But even with handleEvent, I'm getting this weird error that I can't understand. See the new patch.

Hi, I've tested your patch on my Mac with './bin/gaia-test -d' to test it with B2G-desktop, and the event handling path works well. You can see I add a debug flag '__CLICKED__' in the handleEvent section, and the test would assert it's true after we handle the click event. If you want to verify it, remove the comment of the assert.isFalse line, and this time it would fail to pass the test. I don't know whether this is a platform bug let your test fail or not. But the reason I don't print log is that on B2G desktop there is no such console to output the message, unless I open the jsconsole option manually. And I remember that in some test scenario the console.log won't work, so we need to 'dump' it rather than using console.log. Unfortunately, I don't have the detail now, so maybe this is irrelevant.

Sorry I wasn't clear enough. The problem here is not where you put the __CLICKED__ debug but below where I've put console.log('passcodepad == altCameraButton ???')

Do you mean the line would NOT be executed, since the if statement is failed (since you don't mention what's your expect in Comment 28, could you explain more?). I would try it later, but if it is what I think the issue is caused by the evt.target you passed in is not the same element of the altCameraButton, which may be a new DOM element in the unit test, since we mock the getAllElements: 72 mockGetAllElements = function() { 73 ['area', 'areaCamera', 'areaUnlock', 'altCameraButton', 'iconContainer', 74 'overlay', 'clockTime', 'date'].forEach(function(name) { 75 subject[name] = document.createElement('div'); 76 }); And from the name I believe they're i.rrelevant. I now don't know why we need to expect |this.altCameraButton| to equal to |passcodepade| as your debugging patch, since they're different elements. If you're question is you expect the line should NOT be execute BUT the condition get satisfied, then I think we have a bug should be dig with, and I can help to do more tests later.

And I don't mean it's your failure to encounter such weird things. LockScreen is always tricky especially when you run into the old lockscreen.js file that I always want to eliminate it with new components, but unfortunately never success unless not yet. I just lack some information about your test, so what I can do is guessing like the above comment, sorry about that.

The problem here is that |this.altCameraButton === evt.target| is true and that doesn't make sense.

OK I would dig into it. This may also solve other buggy tests. Thanks.

And since there are some strange issue that :rik shows, I won't block the patch if we fire another bug for fixing and adding test later, if this patch is under the time pressure.

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 I've opened bug 1078351 to track the addition of the related tests. Here's the new version of the patch with the variable name changes.

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 OK. The lockscreen part now is fine, thanks. And I would take a look about the test issue.

Comment on attachment 8496004 [details] https://github.com/mozilla-b2g/gaia/pull/24477 [Approval Request Comment] [Bug caused by] (feature/regressing bug #): New feature [User impact] if declined: From partner request [Testing completed]: Manual testing by developer and some new unit tests. Other unit tests to be added in bug 1069286 [Risk to taking this patch] (and alternatives if risky): Low and easy to back out. [String changes made]: None

(In reply to Olof Wickström from comment #14) > Please fix for 2.1 > This impacts first time experience > Users coming from feature phones are accustomed to tactile feedback. > Vibration is the way to give them that on a touch screen. > > Vibration shall be enabled as default! Olof, can your team please help verify this functionality once it lands in 2.1? Thanks

v2.1: https://github.com/mozilla-b2g/gaia/commit/18509d6c420e1d8b3d7d8ac8ef2958dbe56de883

Yes we can test it when it has landed