891762 - [Gaia] View content from WAP Push message

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Ken Chang[:kenkai]]

Need to let user to view content from WAP Push message

Comment 1

[Kai-Chih Hu [:khu]]

Neo will provide the UI in notification bar. Ivan and Candice, can you help to see it will be under your radar? Thanks.

Comment 2

[zhao_tao59]

Will WAP Push(SI/SL) be supported in FireFox 1.2??

i find that FireFox 1.1 does not support SI/SL,

will WAP Push(SI/SL) be supported in FireFox 1.1??

Comment 3

[Ivan Tsay (:ITsay)]

Candice, could you check if it is on your system front-end team since it is related notification bar?

Comment 4

[Kai-Chih Hu [:khu]]

My understanding is Neo will have WAP PUSH UX ready before 8/2.

Comment 5

[Steve Chung [:steveck]]

Attachment: Wap_Push_Message_0.1.pdf

Hi Gabriele, Neo is responsible for Wap push UX. You could contact him directly if you have any question about the wireframe.

Hi Neo, I have some question about the wireframe:
1) Do we only need to handle the hyper link, or we may also need to take case phone number/email? 
2) If we press the home key instead of close button on the view, should we also close the WAP push app, or just move it into background?

The answer of the questions might affect whether we should create a Wap push app under the system or just reuse current message app.

Comment 6

[Ken Chang[:kenkai]]

(In reply to Steve Chung [:steveck] from comment #5)
Hi Gabriele, do you have any question for this UX? Or it makes sense to you and you can start the implementation in Gaia?

Comment 7

[Enpei]

Hi Neo,

Another question about UX, after read the WAP push, what's the UX behavior?

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ken Chang from comment #6)
> Hi Gabriele, do you have any question for this UX? Or it makes sense to you
> and you can start the implementation in Gaia?

I've read the PDF and it looks like I have everything I might need; if I hit some snags on corner cases I'll contact Neo directly.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Add support for displaying WAP Push messages

This is an incomplete patch adds functional support for WAP Push messages w/o displaying them yet; it still needs some polishing and factoring of duplicated code and I still need to make a new pane to display the message proper.

To do that there's two things that I need to clear up with the UX:

- The first thing is what to display as the message title; the wireframe shows the 'WAP Push message' string, is that correct and should we display a localized version of it?

- The second thing is what happens when closing the WAP push message. Shall we go back to the previous view whatever that was? Since I'm handling this in the message app that's what's going to end up in the foreground so maybe I need to deal with this differently.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

I will be summing up here a few of the discussions we recently had to better narrow the scope of this bug and behavior in the various cases that were pointed out. First of all we decided to create a dedicated app for handling WAP Push messages to keep this code out of the system or SMS app where it does not really belong. Second we clarified the behavior in a number of cases:

- If an incoming message shares the same ID with an already received message already in notification:
 -- If new message has an older time stamp, drop the message
 -- If new message has a newer time stamp, display the message (since we can't replace)

- Expired messages
 -- If message is already expired on receipt, drop the message
 -- If user taps an expired message, show "expired" on the full screen message content.

At the moment we will not support:

- Priority
- action: delete
- Replacing old messages by new messages with the same ID

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

I'm also pasting here the commands used to test this functionality in the emulator that :chucklee sent me:

< SI >
sms pdu 07911326040000F0400B911346610089F6000420806291731408350605040B8423F0010603AEAF8203056A0045C60D036F7265696C6C7900850103436865636B20746869732077656273697465000101
sms pdu 07911326040000F0400B911346610089F6000420806291731408580605040B8423F0010603AEAF8202056A0045C60D0378797A008503656D61696C2F3132332F6162632E776D6C000AC3071999062515231510C304199906300103596F7520686176652034206E657720656D61696C73000101

< SL >
sms pdu 07911326040000F0400B911346610089F60004208062917314081E0605040B8423F0010603B0AF8203066A00850A036F7265696C6C79008501

First you have to connect to emulator command line by telnet localhost 5554

Then just copy and paste the command above.

Note that the sms pdu XXXX must be in same line without any line break.
If it's wrapped into multiple lines by mail software, please unwrap it manually.

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 785677 [details] [diff] [review]
[PATCH WIP] Add support for displaying WAP Push messages

I'm obsoleting this as I've moved development into a dedicated app as previously discussed. I've pushed the basic scaffolding for this app into the following branch:

https://github.com/gabrielesvelto/gaia/tree/bug-891762-wap-push

Comment 13

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Initial scaffolding of the WAP Push app

Here's a very crude outline of the app. I'm trying to get it work but it's still pretty broken (not only the functionality but also the presentation) and I hope to be able to put it in better shape tomorrow.

All in all this is turning into a tutorial on how to write a Gaia app from scratch for me :)

Comment 14

[Neo Hsieh [:neo]]

Hi 
1. As far as I know, there is only URL in WAP push message. 
2. Move into card view (Background). I mentioned that in Specs.
Thanks :)

(In reply to Steve Chung [:steveck] from comment #5)
> Created attachment 784845 [details]
> Wap_Push_Message_0.1.pdf
> 
> Hi Gabriele, Neo is responsible for Wap push UX. You could contact him
> directly if you have any question about the wireframe.
> 
> Hi Neo, I have some question about the wireframe:
> 1) Do we only need to handle the hyper link, or we may also need to take
> case phone number/email? 
> 2) If we press the home key instead of close button on the view, should we
> also close the WAP push app, or just move it into background?
> 
> The answer of the questions might affect whether we should create a Wap push
> app under the system or just reuse current message app.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Initial scaffolding of the WAP Push app

Now I'm making some decent progress: the application has finally the looks described in the UX and intercepts and displays WAP Push messages correctly. A few important things remain to be fixed:

- Notifications do not work yet, I have to figure out why.

- The application is still visible as a regular application but we'll want it to be hidden, to properly make it display messages while hidden I need to add support for popping up attention screens which I haven't done yet.

- The message content and sender are displayed as-is, this might not be what we want as they may contain HTML elements and may need to be sanitized.

Regarding the UX I've got two aspects that need clarification:

- First of all the sender is currently shown as raw text, however it is my understanding that it could be a number, should I have the application look it up in the contacts and display the associated name instead?

- The contents of the message may contain HTML text, for security reasons we usually escape all HTML code contained in incoming SMS messages for example. Should I do the same here and display only sanitized text? The UX spec suggests that we might want to display links in that case this might need some adjustment.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH WIP] Initial scaffolding of the WAP Push app

Most of the functionality is now there, the attention screen is showing up properly but I've still left quite a bit of testing functionality in. I need to do a few more tests to ensure that this is working properly in the emulator, then I'll move the testing bits into proper unit-tests and polish the rest of the code, then we're good to go.

Comment 17

[Gabriele Svelto [:gsvelto] (PTO)]

Updating the ETA for this bug. The functionality is mostly complete as well as the UI but I have to work out a few issues that show up only when I test this in the emulator. Hopefully it should be done by the end of this week.

Comment 18

[Gabriele Svelto [:gsvelto] (PTO)]

Ouch, wrong ETA, corrected.

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Add an application for displaying WAP Push messages

Updated patch, this is the first version that is fully working; compared to the previous patch I did the following changes:

- Fixed a bunch of permissions that prevented the app from working properly
- HTML in message contents is now escaped
- Cleaned and pruned all useless stuff including l10n parts that were copy-pasted

I've still got to move out the test code I've been using to develop this. Once I've done this I'll ask for review. The application is not perfectly polished but contains enough functionality to fully function as per this bug description and I'd be more keen in moving the extra stuff and polish into follow-up bugs.

Comment 20

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Add an application for displaying WAP Push messages

Pointer to Github pull-request

Comment 21

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Add an application for displaying WAP Push messages

Here's the complete patch with the proper amount of polish applied. Tim I'm asking for your feedback because this is a type of component that would have fit within the system app so I think you're the most appropriate person to look at it. I'm not asking for a review right away because this is the first Gaia app I write from scratch and I fear it might not fit within all the current Gaia best practices; so I don't want to waste your review time yet :). This has been tested on the B2G ARM emulator using the method described in comment 11.

BTW this patch implements only the functionality strictly needed to fulfill this bug; I will add further functionality and polish in follow-ups so that they will be easier to develop, test and review.

Comment 22

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

Comment on attachment 793529 [details] [diff] [review]
[PATCH] Add an application for displaying WAP Push messages

Review of attachment 793529 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it is probably for the best to implement this as a new app.

I didn't run the app, but it looks like the app will remain in card view (with a white-ish index.html) when the attention screen is closed. You should fix that.

Thanks!

Comment 23

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #22)
> Yes, it is probably for the best to implement this as a new app.

Thanks for your feedback! I'll add some more polish and then it'll be up for review.

> I didn't run the app, but it looks like the app will remain in card view
> (with a white-ish index.html) when the attention screen is closed. You
> should fix that.

Ah yes, that is the case :( I thought that setting its role to "system" in the manifest would be enough to hide it but apparently it's not; I'll try to get rid of the launch_path and see if it fixes it. Do I have to do anything else to make the app effectively invisible?

Comment 24

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v2] Add an application for displaying WAP Push messages

This patch is a fair bit different than the previous one, the major changes are:

- The application now closes itself after having handled a system message and also after the attention screen has been dismissed, this effectively hides it from the cards-view

- The application now stores locally the incoming messages using IndexedDB; besides being able to persist them between application launches (which is needed for the above functionality) this should allow us to delete and reorder them as needed as required in other bugs

I would be happy with this patch as-is but for some reason I can't seem to be able to retrieve the last message from the database after re-launching the application. The same code-path works fine if I don't close the app window in-between receiving the message and going to the attention screen but stops working otherwise. I've spent a fair bit of time trying to figure out what's wrong with it and its driving me nuts.

Comment 25

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Add an application for displaying WAP Push messages

I figured out the issue I was having was actually quite silly... I was storing the message timestamp as a number but when I tried to retrieve it I passed a string with the same value but a string nevertheless and so the database didn't return the corresponding value. Anyway the final changes I added are the following:

- Dropped the IndexedDB code, used asyncStorage instead

- Properly remove the message from the database after it's been viewed

This is feature-complete, works and has all the nuts and bolts required for quickly implementing the extra features in dependent bugs; hopefully it should be good this time :)

Comment 26

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

Comment on attachment 796078 [details] [diff] [review]
[PATCH v3] Add an application for displaying WAP Push messages

Review of attachment 796078 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Thanks!

Comment 27

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH v3] Add an application for displaying WAP Push messages

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #26)
> Looks great! Thanks!

Thank you for your review but I just realized I made a silly mistake and attached the old patch, sorry for that! Now I've attached the correct patch as described by comment 25 (i.e. dropped the IndexedDB code in favor of asyncStorage and fixed the last issues). If you had looked at the PR when doing the review it already contained the correct patch; I just got the attachment wrong here on the bug. Sorry again for this inconvenience.

Comment 28

[Gabriele Svelto [:gsvelto] (PTO)]

Merged, thanks again!

master: 0c5808de787508b00b13a73e422aad6b97993164

Comment 29

[Ken Chang[:kenkai]]

Does this fix support WAP Push enable and disable functions?

Comment 30

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Ken Chang from comment #29)
> Does this fix support WAP Push enable and disable functions?

Not yet, I'll file follow-ups for that feature as well as others and link them here.

Comment 31

[Gabriele Svelto [:gsvelto] (PTO)]

This has been closed for a while, clearing the needinfo