Last Comment Bug 649307 - focused tab is opened after naming a group and pressing enter
: focused tab is opened after naming a group and pressing enter
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 6
Assigned To: Tim Taubert [:ttaubert]
:
Mentors:
: 672141 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-12 06:05 PDT by Alex Lakatos[:AlexLakatos]
Modified: 2016-04-12 14:00 PDT (History)
7 users (show)
mounir: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (3.76 KB, patch)
2011-04-13 21:00 PDT, Tim Taubert [:ttaubert]
ian: review-
raymond: feedback+
Details | Diff | Splinter Review
patch v2 (4.05 KB, patch)
2011-04-15 12:29 PDT, Tim Taubert [:ttaubert]
ian: review+
Details | Diff | Splinter Review
patch for checkin (3.11 KB, patch)
2011-04-21 10:55 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review

Description Alex Lakatos[:AlexLakatos] 2011-04-12 06:05:51 PDT
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] 2011-04-13 21:00:40 PDT
Created attachment 525918 [details] [diff] [review]
patch v1
Comment 2 Raymond Lee [:raymondlee] 2011-04-13 21:51:21 PDT
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] 2011-04-13 21:54:39 PDT
(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] 2011-04-13 22:43:36 PDT
(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] 2011-04-13 23:15:28 PDT
(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 7 Ian Gilman [:iangilman] 2011-04-15 09:35:55 PDT
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] 2011-04-15 09:43:49 PDT
(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] 2011-04-15 12:29:36 PDT
Created attachment 526342 [details] [diff] [review]
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 11 Ian Gilman [:iangilman] 2011-04-21 09:56:09 PDT
Comment on attachment 526342 [details] [diff] [review]
patch v2

Looks great!
Comment 12 Tim Taubert [:ttaubert] 2011-04-21 10:55:36 PDT
Created attachment 527586 [details] [diff] [review]
patch for checkin
Comment 13 Mounir Lamouri (:mounir) 2011-04-22 06:30:50 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/1f5876332b4c
Comment 14 George Carstoiu 2011-05-05 00:37:31 PDT
Verified on Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
Comment 15 George Carstoiu 2011-07-19 01:04:00 PDT
*** Bug 672141 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.