1091299 - It's possible to open the SMS activity menu multiple times

Status: VERIFIED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

STR:

Double tap the ... icon in the SMS app

Expected behavior:

The "activity" menu opens once.

Observed behavior:

The "activity" menu opens multiple times (i.e. once you cancel it there will be another copy of the menu underneath).

qawanted for branch checks.

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

Issue occurs on Flame 2.2. Double-tapping on the ellipsis icon in Messages app causes the menu to open twice.

Device: Flame 2.2 Master (shallow flash)
BuildID: 20141029135013
Gaia: 0dfc1996eb583c8b507a82bf6b8319624bba23ea
Gecko: 80e18ff7c7b2
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

---------

Issue does NOT occur on Flame 2.1 and Flame 2.0. Double-tapping on the ellipsis icon in Messages app opens only one menu.

Device: Flame 2.1 (shallow flash)
BuildID: 20141029082611
Gaia: 2099fb0df60548cf7d4afc367f5048622cc29b3e
Gecko: f02f3fbd0bb0
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.0 (shallow flash)
BuildID: 20141029060208
Gaia: 9f5b6f025e528fabfcc068782cb9b492cb51a7f9
Gecko: e652d8489ee8
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[Adam Alldredge [:AdamA][Inactive]]

----------------------------------------------------
B2G-Inbound Regression Window (Shallow Flash)
----------------------------------------------------

Last Working:
Device: Flame 2.2 Master
BuildID: 20141015070318
Gaia: a18559e844b6aec946309a3bee6a32ca06ab4649
Gecko: fe4916e0e9df
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken:
Device: Flame 2.2 Master
BuildID: 20141015102818
Gaia: 93fb5070782a549977cf8c485c5d93de54ec657e
Gecko: 069db53e0f68
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Last Working Gaia First Broken Gecko: Issue does NOT reproduce
Gaia: a18559e844b6aec946309a3bee6a32ca06ab4649
Gecko: 069db53e0f68

First Broken Gaia Last Working Gecko: Issue DOES reproduce
Gaia: 93fb5070782a549977cf8c485c5d93de54ec657e
Gecko: fe4916e0e9df

Pushlog:
https://github.com/mozilla-b2g/gaia/compare/a18559e844b6aec946309a3bee6a32ca06ab4649...93fb5070782a549977cf8c485c5d93de54ec657e

Caused by Bug 1078295

Comment 3

[Joshua Mitchell (Inactive)]

possibly broken by Bug 1078295 - can you take a look Chris?

Comment 4

[Julien Wajsberg [:julienw]]

Ah yeah, because of the transition, the button is not hidden anymore.

I don't think we'll ask Chris to do this (unless you're willing to !). The easiest is likely to disable the buttons while a dialog is displayed.

Comment 5

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

triage:  regression

Comment 6

[Manuel Casas Barrado [:mancas]]

Attachment: First patch

Please take a look at this first approach and let me know if we need to change something

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8514955 [details] [review]
First patch

The problem with this approach is that you'll need to change all events for all buttons/links. The problem will appear for other interface pieces.

The generic method is rather to disable the whole interface except the menu itself. This should be reasonnably easy using "pointer-events: none" on the right elements.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8514955 [details] [review]
First patch

My preference here for a quick fix would be to simple store a reference to OptionMenu and check on the state before calling show(). Feel free to flag me for review if needed.

Comment 9

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8514955 [details] [review]
First patch

Julien, I've implemented an approach using |pointer-events|. Please take a look. The main idea is to set a class to the whole document to prevent tap events and then, when the option menu transition has ended, we must to remove the class.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8514955 [details] [review]
First patch

Adding a class seems to trigger a restyle which prevents the nice animation from running. I tried setting pointerEvents directly on body.style and it seems to work nicer, can you try this too?

Please also look at sms/js/attachment_menu.js and sms/js/dialog.js.

* attachment_menu is used when you tap on an attachment in the composer (not in the thread)
* dialog is used in several places but a good way to check for the issue is in clicking on a known contact recipient in the "new message" panel. (I could definitely reproduce the issue by double clicking a recipient).

Obviously, please modify the existing unit tests too !

Thanks a lot !

Comment 11

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8514955 [details] [review]
First patch

Hey Julien, I've tried what you said. It works on a flame with 319mb. The patch fixed all the files you've listed in the previous comments.

However, I want to ask you how can we develop an unit test for this patch. I've seen that the only test that checks the |optionMenu| is |thread_ui_list_test| but it use a Mock of the menu. Check the approach in github and tell me if it is enough or if we need to make something more complex

Thanks for your patience!

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8514955 [details] [review]
First patch

I checked the cases I was discussing in comment 10, and while attachment_menu.js does not have the issue (we're not inserting a new element, but reusing an existing one, so opening it twice just has no effect), dialog.js has the issue. 

Please try this STR:

* go to "new message" panel
* insert a known contact
* dbl click on the recipient
=> you get twice the dialog

So you need to fix this in dialog.js too.

About unit tests: shared/js/option_menu.js has unit tests in apps/sharedtest/test/unit/option_menu_test.js. I know this is not obvious but here it is :)

No unit test change is necessary in SMS for option_menu.js. You'll have to add some for dialog.js though.

Thanks!

Comment 13

[Julien Wajsberg [:julienw]]

Oh, and you'll need a review from kgrandon for the option_menu file.

Comment 14

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8514955 [details] [review]
First patch

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Comment on attachment 8514955 [details] [review]
> First patch
> 
> I checked the cases I was discussing in comment 10, and while
> attachment_menu.js does not have the issue (we're not inserting a new
> element, but reusing an existing one, so opening it twice just has no
> effect), dialog.js has the issue. 
> 
> Please try this STR:
> 
> * go to "new message" panel
> * insert a known contact
> * dbl click on the recipient
> => you get twice the dialog
> 
> So you need to fix this in dialog.js too.
> 
> About unit tests: shared/js/option_menu.js has unit tests in
> apps/sharedtest/test/unit/option_menu_test.js. I know this is not obvious
> but here it is :)
> 
> No unit test change is necessary in SMS for option_menu.js. You'll have to
> add some for dialog.js though.
> 
> Thanks!

Julien, I've fixed the issue in |dialog.js| too. Besides, I've added an unit test to check if the style is applied to the document.body when we call |show| function.

Please take a look and let's see if there is another case where we can reproduce this issue. I've checked the whole application but I haven't found more dialogs.

Kevin, could you check the |option_menu| changes, please?

Thanks!

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 8514955 [details] [review]
First patch

r=me for the SMS part, I just added one nit.

I'm a bit sad that we can't use a css class but I'll try to find out

Comment 16

[Kevin Grandon :kgrandon]

Comment on attachment 8514955 [details] [review]
First patch

The code seems fine to me, but I don't really like it. Hooking into a global style like that seems like it might cause problems with other apps that use this - even though I do think this should be ported to the gaia-menu web component in the future. I still think that the app should control this with a reference to the object, and possibly check on some value of that object, like I mentioned in comment 8.

You do have my approval to land with Juliens review though, but you can always flag me again if needed.

Comment 17

[Manuel Casas Barrado [:mancas]]

Julien, what do you think about the approach Kevin proposed in comment 16?

With the patch that we have, all the same issues around the system should be fixed. However, I've seen that |dialog.js| and |option_menu.js| handle this case: "The user taps twice to open an instance of one of the objects".

https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/dialog.js#L150-L156

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/option_menu.js#L181-L187

As you can see in this lines, if the instance has been already included in the DOM, it will not be included again, so the problem is solved but keep in mind that we have to create an instance of the object on init, then we just need to call to the methods |show| and |hide| without creating a new object.

I don't mind if I have to change the approach so don't hesitate to tell it to me.

Comment 18

[Julien Wajsberg [:julienw]]

Right we could do this in some cases (But not in init (because I want to reduce what we do at init). We can do this lazily at first use, either in the lib or in the caller.)

But see also that the dialogs' constructors take some parameters. The parameters usually change depending on what you click on... Not always true but quite common.

Also this would fix the issue that the alert is displayed twice, but a determined user could tap on another part of the window.

So I'm still in favor of using pointer events. Another way would be to display (without an animation) an empty transparent div on top of the window, and remove it when the transition is finished. But I think pointer events is better :/

Ideally I'd like to file a bug about the css restyle that happens when using a class, and have a reference to that bug in the code. But I couldn't find the time to see if that's what's really happening.

Can you still file a bug about this? Something like "animations don't run if we set a class on document.body before", add a reference to this in a comment to the code where you add the pointerEvents style, and I can try to find time to investigate on a smaller testcase... I just want that a reader can have a reference to the issue when he reads the code.

Comment 19

[Julien Wajsberg [:julienw]]

Note that we could definitely change how dialog/option_menu work, and change the displayed text at show()ing time. And note also that's why I prefer the approach we took for attachment_menu: the DOM is in the html file at startup, we don't need to do anything to add it.

But I think this is out of scope for this bug.

Comment 20

[Manuel Casas Barrado [:mancas]]

I've open the related bug you suggested. I think we should keep the current approach and if you want, open a follow up to refactor the dialog and option_menu as you suggested in comment 19. What do think about it?

Comment 21

[Julien Wajsberg [:julienw]]

Yep, sounds good !

don't forget to add the bug number as a comment near your line "pointerEvents = none" :)

Comment 22

[Julien Wajsberg [:julienw]]

Manuel, can you land this patch yourself or do you need help?
If you added the comment then I can land it if you want.

Comment 23

[Manuel Casas Barrado [:mancas]]

Julien, I haven't permissions to land the patch. Besides, I was checking the integration test that fails in travis, but it seems not to be related to our changes. The comment has been added so if you have a moment, I think it will be more quick, if you land the patch instead of requesting a checkin.

Thanks!

Comment 24

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/9b4585e14863917943c02f18d003a77c5ae8aaac

Comment 25

[Brogan Zumwalt [Inactive]]

Issue verified fixed on Flame 2.2

The activity menu opens once even if pressed multiple times.

Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141121040204
Gaia: 25388c6bce932657ebf93adedf31881bfaf88c15
Gecko: 3366c0fcf9c2
Version: 36.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0