Closed Bug 649307 Opened 10 years ago Closed 10 years ago

focused tab is opened after naming a group and pressing enter

Categories

(Firefox Graveyard :: Panorama, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 6

People

(Reporter: AlexLakatos, Assigned: ttaubert)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → tim.taubert
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #525918 - Flags: feedback?(raymond)
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.
Attachment #525918 - Flags: review?(ian)
Attachment #525918 - Flags: feedback?(raymond)
Attachment #525918 - Flags: feedback+
(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 :)
(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.
(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 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.
Attachment #525918 - Flags: review?(ian) → review-
(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.
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #525918 - Attachment is obsolete: true
Attachment #526342 - Flags: review?(ian)
Comment on attachment 526342 [details] [diff] [review]
patch v2

Looks great!
Attachment #526342 - Flags: review?(ian) → review+
Attachment #526342 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/1f5876332b4c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6
Verified on Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
Status: RESOLVED → VERIFIED
Duplicate of this bug: 672141
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.