969277 - [NFC] NFC Pairing UI is different from UX spec

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Walter Chen[:ypwalter][:wachen]]

please open the UX design document in BUG 894672.
P.6 Step.2
P.7 Step.2
P.8 Step.2
are not as expected.

There is no any kind of notification on bluetooth pairing or bluetooth enabling.

Comment 1

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

UX doc -> https://bug894672.bugzilla.mozilla.org/attachment.cgi?id=811891
Needinfo Arno for he used to provide patch on bug 903305.

Comment 2

[arno]

Interesting. I am not on the CC for bug 894672 and I see the UX design for the first time. During the review process of my implementation no one pointed out the discrepancy. I’ve followed Android’s behavior. The user expresses his/her intent by holding the phone close to the BT device and turning on BT and pairing happens automatically (this is what Android does). If I had seen the UX design proposed in 894672, I would have pointed out that the many UI interactions negates the whole point of NFC-based pairing (it takes just as many steps with the Settings app to pair). Anyways, since everyone on the CC for 894672 seems to agree with the UX design, that is what we’ll do. I am no UI person and I need one of the Moz UI wizards to send me some code that implements the UI as described by the UX design. Then it will be easy for me to wire up the UI with the logic of the static NFC handover.

Comment 3

[Walter Chen[:ypwalter][:wachen]]

Hi, Arno,

I opened up this bug not just for pointing at someone and ask for fixing it. This is for more likely to be a discussion place. Since the behavior and the UX design need to match, I think you should talk with Juwei (UX Designer) to clarify what are the points of her design and what is your concern. After all, you two can come to a conclusion of whether the B2G code should change or UX design document should fix. 

Many Thanks,
Walter

Comment 4

[arno]

Walter: thanks for the clarification. The UX design was already done several months ago and since there were no more discussions, I assumed that is what the implementation should do. Then take my comment as a suggestion to change the UX design. At least I tried to argue in favor of the way Android handles this user story. But I think we should give Juwei a chance to comment.

Comment 5

[Walter Chen[:ypwalter][:wachen]]

I think she is in the USA or flying back to Taiwan now. I will talk with her once she got back.  Thanks.

Comment 6

[Juwei Huang[:juwei]]

Arno, I'm so sorry you didn't receive the spec, hopefully next time we all can make sure the specs deliver to the right person.
As for design, I am ok with skipping step 2 to make the whole flow more easier and quicker, as long as Sandip & everybody also agree with the modification since the user story said that "If BT is turned off, the user will be prompted to turn on BT". I'm not quite sure is everyone all agree with turning on BT without inform users at the moment of pairing BT device. If so, that's do it and I'll deliver spec by one day.

Comment 7

[Sandip Kamat]

Hi Juwei, I need to see some screenshots. Can you pls post?

Comment 8

[Juwei Huang[:juwei]]

Hi Sandip, here's the original wireframe: https://bug894672.bugzilla.mozilla.org/attachment.cgi?id=811891
The new idea is to discard step 2 for p.6~8, which means there's no confirm step whether BT is turned on or not.

Comment 9

[bhavana bajaj [:bajaj]]

Given the descoped 1.4 feature list, this request falls out of scope for 1.4 hence pushing the nomination to 1.5.

Comment 10

[Sandip Kamat]

(In reply to Juwei Huang from comment #8)
> Hi Sandip, here's the original wireframe:
> https://bug894672.bugzilla.mozilla.org/attachment.cgi?id=811891
> The new idea is to discard step 2 for p.6~8, which means there's no confirm
> step whether BT is turned on or not.

Hi Juwei and Arno,

I am okay with pairing without UI notification in case of BT already being on. However, in case it is off, I feel we should let the user explictly know that BT needs to be turned on with a UI prompt. Turning BT on sliently has implications on battery like (among other things), so that prompt is necessary.

Comment 11

[arno]

(In reply to Sandip Kamat from comment #10)
> I am okay with pairing without UI notification in case of BT already being
> on. However, in case it is off, I feel we should let the user explictly know
> that BT needs to be turned on with a UI prompt. Turning BT on sliently has
> implications on battery like (among other things), so that prompt is
> necessary.

In BT there is a difference between pairing and connecting. If you use external speakers you need to do both, pair and connect, if you want to stream music. It makes sense to do both at the same time when you do this via a static NFC handover. I suggest to have one confirmation dialog that asks the user if s/he wants to *connect* to the remote BT device. This dialog should appear irrespective of whether BT is enabled or not. If the user confirms, enabling of BT, pairing and connecting should happen automatically. Juwei: what do you think?

Comment 12

[Juwei Huang[:juwei]]

Hi Arno,
The idea you propose is exactly the same flow as the original spec addressed: We prompt a confirm window at the moment of connecting BT device. And if we are going to carefully differentiate "pair" & "connect", we can modify the string of the confirm window as

1) BT off: "Turn on Bluetooth to connect < BT device name >"
2) BT on: "Are you sure you want to connect <BT device name>?"

Is it okay with you? Let me know :)

Comment 13

[Ken Chang[:kenkai]]

Arno, If you are not the owner, please correct me. Thanks.

Comment 14

[arno]

(In reply to Juwei Huang from comment #12)
> Hi Arno,
> The idea you propose is exactly the same flow as the original spec
> addressed: We prompt a confirm window at the moment of connecting BT device.
> And if we are going to carefully differentiate "pair" & "connect", we can
> modify the string of the confirm window as
> 
> 1) BT off: "Turn on Bluetooth to connect < BT device name >"
> 2) BT on: "Are you sure you want to connect <BT device name>?"
> 
> Is it okay with you? Let me know :)

Sounds good. One minor detail: before we actually pair and connect to a remote device, we may not know its name. I.e., the static handover may only contain the remote MAC address but not a clear-text name of the device (this is optional information). I guess we can do four permutations of the text we show in the dialog. What do you think?

Comment 15

[Sandip Kamat]

(In reply to Juwei Huang from comment #12)
> Hi Arno,
> The idea you propose is exactly the same flow as the original spec
> addressed: We prompt a confirm window at the moment of connecting BT device.
> And if we are going to carefully differentiate "pair" & "connect", we can
> modify the string of the confirm window as
> 
> 1) BT off: "Turn on Bluetooth to connect < BT device name >"
> 2) BT on: "Are you sure you want to connect <BT device name>?"
> 

That sounds good. I am assuming you will only have 1 prompt in both cases and not 2 separate ones for pair and connect.

Comment 16

[arno]

(In reply to Sandip Kamat from comment #15)
> That sounds good. I am assuming you will only have 1 prompt in both cases
> and not 2 separate ones for pair and connect.

Yes. One confirmation dialog, but the message can be of four different variations.

Comment 17

[Juwei Huang[:juwei]]

(In reply to arno from comment #16)
> (In reply to Sandip Kamat from comment #15)
> > That sounds good. I am assuming you will only have 1 prompt in both cases
> > and not 2 separate ones for pair and connect.
> 
> Yes. One confirmation dialog, but the message can be of four different
> variations.

Agree! One confirmation dialog for both pair and connect, four permutations for string. Let's do it :)
1) BT off - Know the device name - "Turn on Bluetooth to connect < BT device name >"
          - Not know the device name - "Turn on Bluetooth to connect the device"
2) BT on - Know the device name - "Are you sure you want to connect <BT device name>?"
         - Not know the device name - "Are you sure you want to connect the device?"

I've updated the spec to bug 894672, which is linked to https://bug894672.bugzilla.mozilla.org/attachment.cgi?id=8392664

Comment 18

[arno]

May I suggest slightly different wordings? Perhaps we should consult a native speaker. :)

(In reply to Juwei Huang from comment #17)
> 1) BT off - Know the device name - "Turn on Bluetooth to connect < BT device
> name >"

"Turn on Bluetooth and connect to <BT_device_name>?"

>           - Not know the device name - "Turn on Bluetooth to connect the
> device"

"Turn on Bluetooth and connect to the device?"

> 2) BT on - Know the device name - "Are you sure you want to connect <BT
> device name>?"

"Are you sure you want to connect to <BT_device_name>?"

>          - Not know the device name - "Are you sure you want to connect the
> device?"

"Are you sure you want to connect to the device?"

Comment 19

[Juwei Huang[:juwei]]

Hi Arno! It looks better! thanks :)

Comment 20

[arno]

Attachment: NFC Pairing UI

This PR implements the UX for NFC pairing. It adds a new system dialog to Gaia. Please let me know if I should also do a ui-review.

Comment 21

[Evelyn Hung]

Comment on attachment 8396052 [details] [review]
NFC Pairing UI

I feel we should use CustomDialog instead of adding more nodes in html. 
Since this bug is not related to bluetooth functionality, I will let Alive review it.

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8396052 [details] [review]
NFC Pairing UI

1. Use custom dialog or modal dialog instead. Or, wait until bug 964653 is relanded and use new system dialog.
2. Please follow system 2 convention:
https://wiki.mozilla.org/Gaia/System/Refactoring_Plan
3. Please add unit tests.

Comment 23

[arno]

(In reply to Alive Kuo [:alive] from comment #22)
> 1. Use custom dialog or modal dialog instead. Or, wait until bug 964653 is
> relanded and use new system dialog.
> 2. Please follow system 2 convention:
> https://wiki.mozilla.org/Gaia/System/Refactoring_Plan
> 3. Please add unit tests.

Alive: thanks for your feedback and guidance. I've opted for the new system dialog since bug 964653 has landed by now. Without doing a formal review, would you mind taking a quick look at the rewritten UI:
https://github.com/svic/gaia/commit/64508b6819b93d69bce40c68225fe79763d1e690

If you approve with the general approach, I will add unit tests.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

f+ if your NfcConnectDialog is instantiable.

Comment 25

[Walter Chen[:ypwalter][:wachen]]

I am asking to nominate this to 2.0+ since UX design should be put in front of everyone if there is any issue. In that case, we can make UX design clear and we can implement it when coding. If it is not nominated, it will never be done.

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I believe we can + it as Arno already took it.

Comment 27

[arno]

The PR implements the UX. The only reason I didn't request a review yet is because Alive wants to have test cases. This can only be done once the NFC emulation is capable of doing tag reading. I will followup as as soon as the prerequisites are in place.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to arno from comment #27)
> The PR implements the UX. The only reason I didn't request a review yet is
> because Alive wants to have test cases. This can only be done once the NFC
> emulation is capable of doing tag reading.

Alive says "3. Please add unit tests."
That doesn't need emulator.

You just need a Firefox browser.

https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests

Comment 29

[arno]

(In reply to Yoshi Huang[:allstars.chh] from comment #28)
> Alive says "3. Please add unit tests."
> That doesn't need emulator.
> 
> You just need a Firefox browser.

Please propose a meaningful unit test for this particular PR that ties in NFC, BT and a system dialog.

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to arno from comment #29)
> Please propose a meaningful unit test for this particular PR that ties in
> NFC, BT and a system dialog.

By 'meaningful' I think you are talking about more high level integration test, like gaia-ui-test, or marionette js or Gaia integration test. But that's not Alive asked in the first place.

Also I wouldn't call the test (With NFC, BT and a system dialog) a 'unit test' because it involves some many componenets at once.

For Gaia devs, they mostly write Gaia Unit tests, which can generate some native event or some test data, (like nfc-tech-discovered, as Garner did in Bug 963556, without a real NFC phone or emulator), and use this to see if the output or some data structure is as expected. 

And most of the time they use some Mock objects (some fake objects, for example, a fake navigator.mozNfc), so you could verify your code on your desktop browser, without really need a NFC phone and a BT-headset to test this.

Comment 31

[arno]

(In reply to Yoshi Huang[:allstars.chh] from comment #30)
> By 'meaningful' I think you are talking about more high level integration
> test, like gaia-ui-test, or marionette js or Gaia integration test. But
> that's not Alive asked in the first place.

Yes. This PR adds a new system dialog and IMHO the proper way to test this is via a Marionette test case. If you really want to do a unit test case, then do you propose I mockup the system dialog similar to this:
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/mock_confirm_dialog.js

This would not test the code in this PR but the code paths that make use of the system dialog. Just want to confirm that this is what you want.
> 
> Also I wouldn't call the test (With NFC, BT and a system dialog) a 'unit
> test' because it involves some many componenets at once.
> 
> For Gaia devs, they mostly write Gaia Unit tests, which can generate some
> native event or some test data, (like nfc-tech-discovered, as Garner did in
> Bug 963556, without a real NFC phone or emulator), and use this to see if
> the output or some data structure is as expected. 
> 
> And most of the time they use some Mock objects (some fake objects, for
> example, a fake navigator.mozNfc), so you could verify your code on your
> desktop browser, without really need a NFC phone and a BT-headset to test
> this.

Comment 32

[arno]

Attachment: NFC Pairing UI

This PR implements the pairing UI according to UX specs and adds a unit test for the NFC Handover Manager.

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8411423 [details] [review]
NFC Pairing UI

cancel r? for it has lint error.

Comment 34

[arno]

Attachment: NFC Pairing UI

Make jshint happy.

Comment 35

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8412065 [details] [review]
NFC Pairing UI

Hi Arno,
I am sorry if I didn't make it clear in my previous comment.
1. Please have nfcConnectDialog in nfcConnectSystemDialog. It's superfluous.
2. Instantiate nfcConnectSystemDialog in NfcHandoverManager
3. I strongly expect nfcConnectSystemDialog to have unit tests.

Why unit test?
1. No integration tests could be made for NFC right now.
2. We have a very low integration test coverage. I want to improve it.
3. You are not the only one to maintain it. Anyone could break your functionality if you don't have tests.

Comment 36

[arno]

Attachment: NFC Pairing UI

Alive: thanks a lot for the feedback. I based my original implementation on SimPinDialog/SimPinSystemDialog in bug 964653 you mentioned in comment 22. I merged NfcConnectDialog into NfcConnectSystemDialog as you've requested and also fixed the other nits you mentioned in your last review.

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8414211 [details] [review]
NFC Pairing UI

r+ thanks

Comment 38

[arno]

Attachment: NFC Pairing UI. r=alive

Fixed Alive's nits and rebased to latest master.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/4e5ce8487ddbcfc831f0cfab67877a3529500cd3

Comment 40

[Axel Hecht [:Pike]]

Arno, I'm looking at the strings we expose to localizers, and I'm wondering what {{n}} stands for.

Also, is there any reason why the messages are numbered?

Localizers have an easier time contributing if both the string IDs and the placable names are semantically meaningful names.

Comment 41

[arno]

(In reply to Axel Hecht [:Pike] from comment #40)
> Arno, I'm looking at the strings we expose to localizers, and I'm wondering
> what {{n}} stands for.

'n' stands for the name of the Bluetooth device that you try to connect to.

> Also, is there any reason why the messages are numbered?

I admit that the numbers are not very expressive. Let me explain the four different variations and please propose appropriate names and I will create a PR.

confirmNFCConnectMsg1: this text will be shown to the user when the name of the Bluetooth device to connect to is not known and Bluetooth is currently enabled on the FFOS device.

confirmNFCConnectMsg2: this text will be shown to the user when the name of the Bluetooth device to connect to is not known and Bluetooth is currently disabled on the FFOS device.

confirmNFCConnectMsg3: this text will be shown to the user when the name of the Bluetooth device to connect to is known and Bluetooth is currently enabled on the FFOS device.

confirmNFCConnectMsg4: this text will be shown to the user when the name of the Bluetooth device to connect to is known and Bluetooth is currently disabled on the FFOS device.

Comment 42

[Francesco Lodolo [:flod]]

Using {{device-name}} (or similar) instead of {{n}} would definitely help.
Same for string IDs (confirmNFCConnectMsgUnknown, confirmNFCConnectMsgDevice, etc.).

Comment 43

[arno]

(In reply to Francesco Lodolo [:flod] from comment #42)
> Using {{device-name}} (or similar) instead of {{n}} would definitely help.
> Same for string IDs (confirmNFCConnectMsgUnknown,
> confirmNFCConnectMsgDevice, etc.).

I'll change the 'n' to 'device-name'. The other identifiers need to encode two variables: whether Bluetooth is enabled or disabled and whether the device name is known or not. Here one suggestion:

confirmNFCConnectBTenabledNameKnown
confirmNFCConnectBTenabledNameUnknown
confirmNFCConnectBTdisabledNameKnown
confirmNFCConnectBTdisabledNameUnknown

Not sure if this is more descriptive. Please suggest something better. Perhaps it is better to add a comment that explains the four variants.

Comment 44

[Francesco Lodolo [:flod]]

(In reply to arno from comment #43)
> I'll change the 'n' to 'device-name'. The other identifiers need to encode
> two variables: whether Bluetooth is enabled or disabled and whether the
> device name is known or not. Here one suggestion:
> 
> confirmNFCConnectBTenabledNameKnown
> confirmNFCConnectBTenabledNameUnknown
> confirmNFCConnectBTdisabledNameKnown
> confirmNFCConnectBTdisabledNameUnknown
> 
> Not sure if this is more descriptive. Please suggest something better.
> Perhaps it is better to add a comment that explains the four variants.

Both string and variable names look good to me. After the update, I don't think an additional comment is necessary.

Comment 45

[Kamil Leszczuk [:kamituel]]

I've issued a bug to track work on pairing UI unit tests as suggested by @Alive: https://bugzilla.mozilla.org/show_bug.cgi?id=1005896.

Comment 46

[arno]

Created bug 1006032 for changes described in comment 44.