649307 - focused tab is opened after naming a group and pressing enter

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Alex Lakatos[:AlexLakatos]]

A focused tab is opened after naming a group and pressing enter.

Steps to Reproduce
1.Enter Panorama Mode
2.Create a new Group
3.Name it
4.Press ENTER

Actual Results:
4.Group is named and the focused tab opens

Expected Results:
4.Group is named and the focused tab does not open

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 525918 [details] [diff] [review]
patch v1

Is that other way to avoid the setTimeout 1 sec in the test? It would be good that we don't use setTimeout if possible.

r+ and see what Ian thinks.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #2)
> Is that other way to avoid the setTimeout 1 sec in the test? It would be good
> that we don't use setTimeout if possible.

Oh yeah, I'm sorry for that setTimeout. I've been thinking long about this but checking whether TabView.isVisible() directly after simulating the keypress is too early. So basically we want the test to succeed if nothing happens. Maybe I'm thinking too complex so I'm glad if anyone comes up with a better solution :)

Comment 4

[Raymond Lee [:raymondlee]]

(In reply to comment #3)
> (In reply to comment #2)
> > Is that other way to avoid the setTimeout 1 sec in the test? It would be good
> > that we don't use setTimeout if possible.
> 
> Oh yeah, I'm sorry for that setTimeout. I've been thinking long about this but
> checking whether TabView.isVisible() directly after simulating the keypress is
> too early. So basically we want the test to succeed if nothing happens. Maybe
> I'm thinking too complex so I'm glad if anyone comes up with a better solution
> :)

One suggestion is to set a flag e.g. UI._tabViewHidding at the beginning of hideTabView() to tell that we are hiding the UI and cancel it when the animation finishes.  Therefore, we can see whether the flag changes after the keypress.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #4)
> One suggestion is to set a flag e.g. UI._tabViewHidding at the beginning of
> hideTabView() to tell that we are hiding the UI and cancel it when the
> animation finishes.  Therefore, we can see whether the flag changes after the
> keypress.

Thought about something like this, too. This won't work unfortunately because tabItem.zoomIn() hides the tab view not until it's done with zooming :/ The zoom animation is the reason for the timeout.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 525918 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=f3020a90978f

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 525918 [details] [diff] [review]
patch v1

We have a global flag that turns off animations, right? Maybe set that to true for the test so yo don't need the timeout (or at least not as long of one).

Also, you might want to verify that tabView is still visible every time through the keyPress loop in that case?

One more thing: is it really necessary in this test to create a new window? I don't know; maybe every test should create a new window just to keep things clean, but I feel like it adds extra overhead/running time to the test suite.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #7)
> We have a global flag that turns off animations, right? Maybe set that to true
> for the test so yo don't need the timeout (or at least not as long of one).

Interesting :) I'll try that.

> Also, you might want to verify that tabView is still visible every time through
> the keyPress loop in that case?

Yep, let's do that.

> One more thing: is it really necessary in this test to create a new window? I
> don't know; maybe every test should create a new window just to keep things
> clean, but I feel like it adds extra overhead/running time to the test suite.

Good question. I got quite used to open a new window for most of the tests I wrote in the last time. It's easy to prevent cascading failures because we just close the window and don't need to really clean up. That creates like a small sandbox and we don't affect any following tests. But yeah, of course that creates a little overhead and increases the suite's run time.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #7)
> We have a global flag that turns off animations, right? Maybe set that to true
> for the test so yo don't need the timeout (or at least not as long of one).

Fixed.

> Also, you might want to verify that tabView is still visible every time through
> the keyPress loop in that case?

Fixed.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526342 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=24d5db1580b0

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 526342 [details] [diff] [review]
patch v2

Looks great!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 13

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/1f5876332b4c

Comment 14

[George Carstoiu]

Verified on Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110504 Firefox/6.0a1