506275 - Select doesn't behave as manual select on menulist

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Merike Sell (:merike)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; et; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: 

Current version of select appears to be working but it doesn't trigger the same behaviour as manual select does. Even though right option can be seen selected in event dialog in calendar status change to "needs action" should also activate percent complete textbox next to it, but currently it doesn't.


Reproducible: Always

Comment 1

[Merike Sell (:merike)]

Attachment: Patch to fix issue

Comment 2

[cmtalbert]

Comment on attachment 390473 [details]
Patch to fix issue

This seems like a good idea, but when I run this with the eventsdialogv2 test from bug 500469, I still get the popups that never go away.  So, maybe this isn't all we need to do.

Comment 3

[cmtalbert]

Attachment: Events Dialog, edited so that it works a little better

Here is a copy of the eventsdialog version 2 test.  I did a little more hacking on this, and found that putting in the sleeps after the clicks helped a lot. Running this test now on a build of mozmill with the patch from attachment 390473 [details] in place does close most of the menu popups.  The only menupopup I'm not seeing close is the "Reminder" menu popup which occurs just before the attendees dialog is launched.  

Because the attendee dialog causes a hang on my computer, I tried commenting it out and running the test, but even doing that the "reminders" menu popup is still not closed after making the selection.

I'm really stumped as to what is making these menu popups remain open like this.  Mikeal or Adam, do you have any ideas?

Merike's patch above does seem to help.  If I run without the patch, all the menu popups remain displayed.  So, the patch above is a step in the right direction, but it's not all the way there yet.  These sleeps certainly seem to help matters a little bit.

Comment 4

[cmtalbert]

(In reply to comment #3)
> Merike's patch above does seem to help.  If I run without the patch, all the
> menu popups remain displayed.  So, the patch above is a step in the right
> direction, but it's not all the way there yet.  These sleeps certainly seem to
> help matters a little bit.

Hmmm actually it's not quite that easy.

* No patch above, no sleeps --> no menu popups are ever closed
* With patch above, no sleeps --> all popups except reminder list are closed
* No patch above, with sleeps --> all popups except reminder list are closed
* With patch above and sleeps --> all popups except reminder list are closed

Is there just something wrong with the reminder list?
Commenting out reminder list and the attendee dialog (which hangs my test no matter what) I get the following:
* no patch, no sleeps --> all popups displayed
* no patch, with sleeps --> the "privacy" menupopup (the next menu popup following the reminder popup in the test is not closed.

I'm at a loss.  I don't think it is anything specific to do with the reminder list, and I think it must have something to do with the way that we are not handling popups properly, but I have no idea why my sleep idea works just as well as Merike's patch (which makes a whole lot more sense), and why both of them together produce exactly the same behavior as either of them applied separately.

For reference, my "sleeps" that I'm referring to are the event.sleep(xx) calls occurring at lines: 136, 143, 145, and 151 (the one at 151 was trying to get attendee dialog to work, you can ignore that one) in the eventdialogv2 test attached above.

Comment 5

[Merike Sell (:merike)]

Attachment: Testcase

Event dialog test has 0 controller.select calls.

Please try the patch again with this testcase.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Clint, I believe this is related to bug 504468. Looks like we should find a solution with the sleep calls at all. Adding as dependency?

Comment 7

[Merike Sell (:merike)]

Pinging about comment #5.

I'd really like to use controller.select in testcases and not a suspicious click on a menuitem which is essentially the same as the patch but doesn't really reflect that it's selecting from a list and not just clicking a regular element.

Resolving bug 504468 would allow to improve the select call in future so that it also clicks on a menulist before clicking on a menuitem. That would be closer to what user would do as it would first open dropdown and then select an option. But for now I'd take a select call that triggers the same functionality as manual click even though it's not the same visually.

One more note: popup not closing is triggered when there's two clicks, if there's just the one on a menuitem then it's not a problem.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Ok, the whole implementation of the select method was a bit wacky. There are a couple of situations where the function fails. I did mostly a rewrite now. The only thing which doesn't work is the synthesizeMouse call for content select elements. So I use .selected = true at the moment. All other things and also the popup closing should work.

Comment 9

[cmtalbert]

Comment on attachment 394049 [details] [diff] [review]
Patch v1

Sorry it took so long to get this review done.  This looks good, and I tested it with Merike's infamous event dialog patch on Thunderbird/Lightning and all the popups closed properly.

The only comment I have here is that it now appears that we have a special behavior on this function if you pass in -1 for the index.  Could you please document that on MDC in the select section?

Very nicely done, Henrik! r=ctalbert

Comment 10

[cmtalbert]

Oh and I forgot to mention, this might make bug 504468 unnecessary.  We'll have to test around and see if bug 504468 is still needed.

Sorry for the spam.

Comment 11

[cmtalbert]

r541 

Thanks!!!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Docs have been updated on https://developer.mozilla.org/en/Mozmill_Tests/Mozmill_Controller_Object. I have also fixed some other mistakes like using 0 for a not used parameter. That should be null.

Bug 504468 is still needed. The way we are going with this test is just a workaround. The sleep call has also to be done for other mouse/keypress events. I'll check how we can do that.

Further I have filed bug 510519 to get multiple selections work with Mozmill.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks fine for existing tests. Marking as verified.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

An additional fix went in to allow closing the popup after the selection has been made:

http://github.com/mikeal/mozmill/commit/03c31c2c8d56461702f6385939b93ca9fbc3e0c9