1105857 - [Messages] User doesn't receive any notification about incoming message if native modal dialog is shown

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

Description

[Oleg Zasypkin [:azasypkin]]

Since native (window.alert or window.confirm) modal dialogs block app execution it won't react to any new incoming message until dialog is dismissed. 

This problem seems quite severe especially if user switches to other app or Home Screen when such dialog is shown. Then user will miss all incoming messages until he returns to Messages app and dismisses modal dialog or Messages app is killed by system/user. Obviously messages are stored in DB and will be shown once user dismisses modal dialog.

So the following cases are affected:

1. If user receives class-0 message and doesn't dismiss it (window.alert is used for class-0 messsages);
2. If user tries to attach large file and doesn't dismiss "File too large" dialog (window.alert is used for "File too large" dialog);
3. If user tries to delete any message via long tap context menu, but doesn't confirm deletion (window.confirm is used);
4. If user taps on "error" icon of failed message and doesn't confirm resending (window.confirm is used).

To fix this we'll probably have to switch to CustomDialog everywhere we use native modal dialogs.

[Blocking Requested - why for this release]: User may miss notifications about incoming messages that sounds quite serious

Comment 1

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (v2.0m)

Hey Steve,

I've asked to verify this patch in bug 1105129 comment 8 to check whether it works for class-0 message case, but can you please provide your early feedback on the approach itself?

It's wip patch for v2.0m, for master it should be updated a bit, since l10n lib updated, thus CustomDialog too.

I've left few explanations on GitHub.

Thanks!

Comment 3

[Oleg Zasypkin [:azasypkin]]

Renominating to v2.0m? instead as duped bug 1105129 is v2.0m+

Comment 4

[ruihua.zhang]

Attachment: mtklog_20141128_1955.rar

bug 1105129 test result: 
  Send a class 0 message to the MS. MS have not show the class 0 message, it just open the sms app (See the screenshot in the attachment).

Comment 5

[Oleg Zasypkin [:azasypkin]]

(In reply to ruihua.zhang from comment #4)
> Created attachment 8530251 [details]
> mtklog_20141128_1955.rar
> 
> bug 1105129 test result: 
>   Send a class 0 message to the MS. MS have not show the class 0 message, it
> just open the sms app (See the screenshot in the attachment).

Okay, thanks, will flash v2.0 + v2.0m gaia to see what is going on.

Comment 6

[Oleg Zasypkin [:azasypkin]]

(In reply to ruihua.zhang from comment #4)
> Created attachment 8530251 [details]
> mtklog_20141128_1955.rar
> 
> bug 1105129 test result: 
>   Send a class 0 message to the MS. MS have not show the class 0 message, it
> just open the sms app (See the screenshot in the attachment).

Okay I see the problem: looks like patch ("class0.diff") from archive in attachment 8530251 [details] is different from what we asked to test in bug 1105129 comment 11.

Could you please try with the correct one? You can also use attachment 8530119 [details] [review]

Thanks!

Comment 7

[ruihua.zhang]

Attachment: mtklog_20141201_1010.rar

(In reply to Oleg Zasypkin [:azasypkin] from comment #6)

My mistake. I had try the correct one and the result is the same:
The Class1 message can be received and display on the status bar,when you click it or enter SMS app to read the Class1 message, MS will display the Class0 message firstly, after you press OK key, then you can read the Class1 message.

Comment 8

[ruihua.zhang]

(In reply to ruihua.zhang from comment #7)
> Created attachment 8530732 [details]
> mtklog_20141201_1010.rar
> 
> (In reply to Oleg Zasypkin [:azasypkin] from comment #6)
> 
> My mistake. I had try the correct one and the result is the same:
> The Class1 message can be received and display on the status bar,when you
> click it or enter SMS app to read the Class1 message, MS will display the
> Class0 message firstly, after you press OK key, then you can read the Class1
> message.

Test result is OK.

Comment 9

[Oleg Zasypkin [:azasypkin]]

(In reply to ruihua.zhang from comment #8)
> (In reply to ruihua.zhang from comment #7)
> > Created attachment 8530732 [details]
> > mtklog_20141201_1010.rar
> > 
> > (In reply to Oleg Zasypkin [:azasypkin] from comment #6)
> > 
> > My mistake. I had try the correct one and the result is the same:
> > The Class1 message can be received and display on the status bar,when you
> > click it or enter SMS app to read the Class1 message, MS will display the
> > Class0 message firstly, after you press OK key, then you can read the Class1
> > message.
> 
> Test result is OK.

Great, thanks for checking!

Comment 10

[ruihua.zhang]

Attachment: screenshot_20141202.png

I just found a problem. Yesterday we had merge P2 patch from MTK, which include the patch of bug 1087116(make alert scrollable). Base on this, I use this patch and do a test again. I send a class 0 message to MS like the bug 1087116, and the result like the attachment(all the newline is replaced by blank).

Could you make some optimization?

Comment 11

[Oleg Zasypkin [:azasypkin]]

(In reply to ruihua.zhang from comment #10)
> Created attachment 8531011 [details]
> screenshot_20141202.png
> 
> I just found a problem. Yesterday we had merge P2 patch from MTK, which
> include the patch of bug 1087116(make alert scrollable). Base on this, I use
> this patch and do a test again. I send a class 0 message to MS like the bug
> 1087116, and the result like the attachment(all the newline is replaced by
> blank).
> 
> Could you make some optimization?

Thanks for reporting, will see what we can do here.

Comment 12

[Oleg Zasypkin [:azasypkin]]

It'll be nice when Fx implements html5 <dialog> element so that we can base CustomDialog on it some day. Not now though.

Comment 13

[Josh Cheng [:josh]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #12)
> It'll be nice when Fx implements html5 <dialog> element so that we can base
> CustomDialog on it some day. Not now though.

Hi Oleg,
Thanks for the help. do you mean we will fix this "newline is replaced by blank" after bug 840640 is fixed?
Since partner is requesting this in 2.0M and device release date is soon. Can we have temp fix for 2.0M now?

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Josh Cheng [:josh] from comment #13)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #12)
> > It'll be nice when Fx implements html5 <dialog> element so that we can base
> > CustomDialog on it some day. Not now though.
> 
> Hi Oleg,
> Thanks for the help. do you mean we will fix this "newline is replaced by
> blank" after bug 840640 is fixed?
> Since partner is requesting this in 2.0M and device release date is soon.
> Can we have temp fix for 2.0M now?

Hey Josh,

Sorry for the confusion, comment 12 is just for the record, once our bright "<dialog>" future comes :) 

For now, yeah, we'll fix both issues (comment 0 and comment 10) with what we have available already. So once I get feedback from Steve, I'll proceed with the full v2.0m and master patches.

Comment 15

[ruihua.zhang]

(In reply to Oleg Zasypkin [:azasypkin] from comment #14)
> (In reply to Josh Cheng [:josh] from comment #13)
> > (In reply to Oleg Zasypkin [:azasypkin] from comment #12)
> > > It'll be nice when Fx implements html5 <dialog> element so that we can base
> > > CustomDialog on it some day. Not now though.
> > 
> > Hi Oleg,
> > Thanks for the help. do you mean we will fix this "newline is replaced by
> > blank" after bug 840640 is fixed?
> > Since partner is requesting this in 2.0M and device release date is soon.
> > Can we have temp fix for 2.0M now?
> 
> Hey Josh,
> 
> Sorry for the confusion, comment 12 is just for the record, once our bright
> "<dialog>" future comes :) 
> 
> For now, yeah, we'll fix both issues (comment 0 and comment 10) with what we
> have available already. So once I get feedback from Steve, I'll proceed with
> the full v2.0m and master patches.

Hi Oleg,

Is there any update?

Comment 16

[Julien Wajsberg [:julienw]]

Oleg is in holiday for 2 days to recover from his travel time. He'll be back tomorrow!

Comment 17

[Steve Chung [:steveck]]

Comment on attachment 8530119 [details] [review]
GitHub pull request URL (v2.0m)

The overall implementation looks good, but Jenny would prefer to display the latest message on the top, so we might need to adjust the way we handle the queue or stack.

Comment 18

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #17)
> Comment on attachment 8530119 [details] [review]
> GitHub pull request URL (v2.0m, wip)
> 
> The overall implementation looks good, but Jenny would prefer to display the
> latest message on the top, so we might need to adjust the way we handle the
> queue or stack.

Thanks for feedback! Will update patch with Jenny's request then.

Hey Josh, is too late to introduce new l10n strings in v2.0m patch? It's not really critical for this patch, but we need to know anyway.

Thanks!

Comment 19

[Josh Cheng [:josh]]

Hi Oleg,
I am afraid it's too late for l10n strings work for 2.0M. Can we avoid it? 
If an l10n work for this patch is mandatory. I can try to negotiate with partner to do it by themselves after we provide patch. 
Please let me know your preference. Thank you!

Comment 20

[Oleg Zasypkin [:azasypkin]]

(In reply to Josh Cheng [:josh] from comment #19)
> Hi Oleg,
> I am afraid it's too late for l10n strings work for 2.0M. Can we avoid it? 
> If an l10n work for this patch is mandatory. I can try to negotiate with
> partner to do it by themselves after we provide patch. 
> Please let me know your preference. Thank you!

Okay, got it. So I'll just use existing l10n strings from other places, but with the content I need :) I think it should be fine as v2.0m only solution.

Thanks!

Comment 21

[Josh Cheng [:josh]]

Dear Oleg,
It’s closing to Woodduck device launch and partner is requesting patch for this bug asap. 
Could you provide a rough ETA date so I can come back to partner?
Thank you very very much!

Comment 22

[Oleg Zasypkin [:azasypkin]]

(In reply to Josh Cheng [:josh] from comment #21)
> Dear Oleg,
> It’s closing to Woodduck device launch and partner is requesting patch for
> this bug asap. 
> Could you provide a rough ETA date so I can come back to partner?
> Thank you very very much!

v2.0m patch will be ready for review in hour or so. I think we'll tightly collaborate with Steve to finish review and land it tomorrow.

Comment 23

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8530119 [details] [review]
GitHub pull request URL (v2.0m)

Hey Steve,

I've updated this patch with the following changes:

* I've found out that if we use our own Dialog instead of shared/custom_dialog we'll fulfil Jenny's requirement for free without any additional code, so I switched to it;

* Added/updated a bunch of unit tests + fix I've mentioned in bug 1069135 comment 69 to run utils test in full, we can get rid of this fix if we re-land patch for bug 1069135;

* Utils.alert/confirm have Promise return results to comply with use cases where we use modal dialogs;

* Using "sms-hacky-2.0.properties" to define required l10n strings (confirmed with :flod);

* Added CSS change to preserve line breaks inside long class-0 messages.


Ruihua Zhang could you please test the latest patch in the meantime and confirm that it works as you expect? 

Thanks!

Comment 24

[ruihua.zhang]

(In reply to Oleg Zasypkin [:azasypkin] from comment #23)
> Comment on attachment 8530119 [details] [review]
> GitHub pull request URL (v2.0m)
> 
> Hey Steve,
> 
> I've updated this patch with the following changes:
> 
> * I've found out that if we use our own Dialog instead of
> shared/custom_dialog we'll fulfil Jenny's requirement for free without any
> additional code, so I switched to it;
> 
> * Added/updated a bunch of unit tests + fix I've mentioned in bug 1069135
> comment 69 to run utils test in full, we can get rid of this fix if we
> re-land patch for bug 1069135;
> 
> * Utils.alert/confirm have Promise return results to comply with use cases
> where we use modal dialogs;
> 
> * Using "sms-hacky-2.0.properties" to define required l10n strings
> (confirmed with :flod);
> 
> * Added CSS change to preserve line breaks inside long class-0 messages.
> 
> 
> Ruihua Zhang could you please test the latest patch in the meantime and
> confirm that it works as you expect? 
> 
> Thanks!

The newline in class0 message display normal. The class0 message scrollable if the message is longer than the screen. The class1 message can be received if the MS receive the class0 message and press home key.

Test result is ok. Thanks!

Comment 25

[Oleg Zasypkin [:azasypkin]]

(In reply to ruihua.zhang from comment #24)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #23)
> > Comment on attachment 8530119 [details] [review]
> > GitHub pull request URL (v2.0m)
> > 
> > Hey Steve,
> > 
> > I've updated this patch with the following changes:
> > 
> > * I've found out that if we use our own Dialog instead of
> > shared/custom_dialog we'll fulfil Jenny's requirement for free without any
> > additional code, so I switched to it;
> > 
> > * Added/updated a bunch of unit tests + fix I've mentioned in bug 1069135
> > comment 69 to run utils test in full, we can get rid of this fix if we
> > re-land patch for bug 1069135;
> > 
> > * Utils.alert/confirm have Promise return results to comply with use cases
> > where we use modal dialogs;
> > 
> > * Using "sms-hacky-2.0.properties" to define required l10n strings
> > (confirmed with :flod);
> > 
> > * Added CSS change to preserve line breaks inside long class-0 messages.
> > 
> > 
> > Ruihua Zhang could you please test the latest patch in the meantime and
> > confirm that it works as you expect? 
> > 
> > Thanks!
> 
> The newline in class0 message display normal. The class0 message scrollable
> if the message is longer than the screen. The class1 message can be received
> if the MS receive the class0 message and press home key.
> 
> Test result is ok. Thanks!

Thanks!

Comment 26

[Oleg Zasypkin [:azasypkin]]

Quick update: since new patch for bug 1069135 has been just landed - I've rebased my PR on it to remove redundant changes.

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 8530119 [details] [review]
GitHub pull request URL (v2.0m)

The patch looks fine, although I'm wondering that the new alert's styling would be sightly different then the original window alert, ruihua.zhang already gave positive feedback per his/her testing. So I think the styling is not a serious concern here. Thanks for the great efforts!

Comment 28

[Julien Wajsberg [:julienw]]

Maybe do a quick ui-review with Fang?

Comment 29

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #27)
> Comment on attachment 8530119 [details] [review]
> GitHub pull request URL (v2.0m)
> 
> The patch looks fine, although I'm wondering that the new alert's styling
> would be sightly different then the original window alert, ruihua.zhang
> already gave positive feedback per his/her testing. So I think the styling
> is not a serious concern here. Thanks for the great efforts!

Thanks for review!

(In reply to Julien Wajsberg [:julienw] from comment #28)
> Maybe do a quick ui-review with Fang?

Yeah, let's try - I'll prepare some screenshots shortly. + anyway it would be better to wait until bug 1110682 is resolved to be sure that all tests are green.

Comment 30

[Oleg Zasypkin [:azasypkin]]

Attachment: Dialogs (top - before the patch, bottom - after the patch)

Hey Fang,

We need your expert advice here :)

Some history behind the change: 

In Messages we use two types of dialogs: our custom dialog (#1, #2 and #3 on the screenshot) and native modal dialogs (#4, #5, #6). Since native modal dialogs have a bunch of technical limitations we decided to use the same non-native dialogs everywhere in Messages. That is the reason why dialogs #4, #5 and #6 changed their appearance. Top row is what we had in Messages before the patch, bottom row - with the patch applied.

As engineer I tend to have the same style for all dialogs (as in bottom row), but what are your thoughts on this?

Thanks!

Comment 31

[Josh Cheng [:josh]]

Dear Fang,
Could you help to provide review feedback as this is partner blocker bug. Thanks!

Comment 32

[Fang Shih [:fang]]

Comment on attachment 8535617 [details]
Dialogs (top - before the patch, bottom - after the patch)

Hi Oleg, 
I agree with you that we use the same style for all the dialogs. But is there any reason that we have custom dialog on #1, #2, #3 at the beginning? We should have the same style design for all of our dialogs like other apps. The custom dialogs would conflict our common UI design. Can we change it back? 
Thanks!

Comment 33

[Oleg Zasypkin [:azasypkin]]

(In reply to Fang Shih [:grasspizza] from comment #32)
> Comment on attachment 8535617 [details]
> Dialogs (top - before the patch, bottom - after the patch)
> 
> Hi Oleg, 
> I agree with you that we use the same style for all the dialogs. But is
> there any reason that we have custom dialog on #1, #2, #3 at the beginning?
> We should have the same style design for all of our dialogs like other apps.
> The custom dialogs would conflict our common UI design. Can we change it
> back? 
> Thanks!

Hey Fang,

Mmm, as far as I remember we always (at least last several months) used our custom dialogs for cases #1, #2 and #3 (this patch doesn't change it).

I agree that we should use the same dialogs for all apps, to give you current state of things, I've investigated through all dialogs we have in gaia a bit, here some observations: 

There are 3 "types" of dialogs that are used across the apps and every type has somewhat _different_ styles

* Gecko "native" dialogs (window.alert/window.confirm) - unfortunately we should avoid such dialogs due to technical limitations (dialogs #4, #5 and #6 used these styles previously);

* Custom gaia dialogs that are located in shared folder and some apps use it (SMS never used it);

* App-specific custom dialogs that are located in specific app folder (SMS and some other apps).

So, what styles should we use as common ground? Do we have any spec or something on this?

Also probably we can handle this in a separate bug.

Comment 34

[Fang Shih [:fang]]

After an offline discussion with Oleg. We will change all the dialogs to the same style with the current common UI: http://buildingfirefoxos.com/building-blocks/confirm.html
And All the "OK" button should be blue. 

Thanks!

Comment 35

[Oleg Zasypkin [:azasypkin]]

(In reply to Fang Shih [:grasspizza] from comment #34)
> After an offline discussion with Oleg. We will change all the dialogs to the
> same style with the current common UI:
> http://buildingfirefoxos.com/building-blocks/confirm.html
> And All the "OK" button should be blue. 
> 
> Thanks!

Thanks Fang!

Hey Josh, we're going to land this patch on v2.0m without style updates as it seems quite urgent. Could you please confirm that you're fine with this and whether you need separate v2.0m patch for style changes mentioned in comment 34?

We'll definitely do style changes for the master patch :)

Thanks!

Comment 36

[Josh Cheng [:josh]]

Dear Oleg,
Partner has tried the patch and they are fine with the current solution. I think we do not need separate v2.0m patch for style changes mentioned in comment 34. I'll help to communicate with partner later.
Thank you very much!

@Fang,
Thank you too!

Comment 37

[Oleg Zasypkin [:azasypkin]]

(In reply to Josh Cheng [:josh] from comment #36)
> Dear Oleg,
> Partner has tried the patch and they are fine with the current solution. I
> think we do not need separate v2.0m patch for style changes mentioned in
> comment 34. I'll help to communicate with partner later.
> Thank you very much!
> 
> @Fang,
> Thank you too!

Okay, thanks!

v2.0m: https://github.com/mozilla-b2g/gaia/commit/09097552f236b1482ce469b4c7d92d80d0706f83

Working on master patch at the moment. Will be ready soon.

Comment 38

[Oleg Zasypkin [:azasypkin]]

Attachment: Updated dialogs (master).zip

Hey Fang,

I've updated styles for dialogs inside Messages app, it has affected main dialogs inside apps, so asking for careful ui-review here :) 

Archive contains several screenshots for main dialogs that have changed, on the left side is what we had before patch, on the right side - with this patch. Especially I'm wondering if you're ok with "Remove recipient" and "Delete single message" dialog changes.

Please, ping me on IRC if you have any questions/suggestions/doubts.

Thanks a lot!

Comment 39

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (master)

Steve,

Here is the master patch that contains the same changes as in v2.0m one + style updates + some unexpected refactoring (left some clarifying comments at GitHub) :) Would you mind to review it?

Arnau, I'm proposing small style change in shared confirm.css to make confirm buttons _more_ centered and remove some js code in Messages to support single-button-confirm dialog. Can you please check if it looks fine for you?

Thanks!

Comment 40

[Fang Shih [:fang]]

Comment on attachment 8538336 [details]
Updated dialogs (master).zip

Thanks for the screenshots! Looks good!
The changed you've made looks fine to me as well! Thanks A lot!

Comment 41

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8538338 [details] [review]
GitHub pull request URL (master)

The BB part looks good but needs minor tweaks commented in github.
Thanks Oleg!

Comment 42

[Steve Chung [:steveck]]

Hi Oleg, overall loks good, but I'll need more time to test on device. Since Bug 1018111 landed with some alert option changes, could you please rebase your patch based on this? Thanks!

Comment 43

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #42)
> Hi Oleg, overall loks good, but I'll need more time to test on device. Since
> Bug 1018111 landed with some alert option changes, could you please rebase
> your patch based on this? Thanks!

Thanks! No worries, take your time. I've rebased PR on the latest master :)

Comment 44

[Steve Chung [:steveck]]

Comment on attachment 8538338 [details] [review]
GitHub pull request URL (master)

The patch is really great that the styling of hte dialog could be unified. About your question for the deletion dialog, I think we could fire another bug for it. Thanks for the awsome work!

Comment 45

[Steve Chung [:steveck]]

(In reply to Steve Chung [:steveck] from comment #44)
> 
> The patch is really great that the styling of hte dialog could be unified.
> About your question for the deletion dialog, I think we could fire another
> bug for it. Thanks for the awsome work!

Create bug 1115243 for it.

Comment 46

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #44)
> Comment on attachment 8538338 [details] [review]
> GitHub pull request URL (master)
> 
> The patch is really great that the styling of hte dialog could be unified.
> About your question for the deletion dialog, I think we could fire another
> bug for it. Thanks for the awsome work!

Thanks for review! Nits fixed, Treeherder is green.

Master: https://github.com/mozilla-b2g/gaia/commit/322ef5116a5827a30c9a3cd9b842449a9c66a5b3

Comment 47

[Julien Wajsberg [:julienw]]

Did this land in v2.0m ?

Comment 48

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #47)
> Did this land in v2.0m ?

Yes, sorry forgot to paste the link.

v2.0m: https://github.com/mozilla-b2g/gaia/commit/09097552f236b1482ce469b4c7d92d80d0706f83