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.

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

---------------------------------------------------- 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

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

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.

triage: regression

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

Comment on attachment 8514955 [details] 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 on attachment 8514955 [details] 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 on attachment 8514955 [details] 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 on attachment 8514955 [details] 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 on attachment 8514955 [details] 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 on attachment 8514955 [details] 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!

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

Comment on attachment 8514955 [details] First patch (In reply to Julien Wajsberg [:julienw] from comment #12) > Comment on attachment 8514955 [details] > 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 on attachment 8514955 [details] 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 on attachment 8514955 [details] 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.

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.

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.

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.

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?

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

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.

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!

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

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