focused tab is opened after naming a group and pressing enter

VERIFIED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: AlexLakatos, Assigned: ttaubert)

Tracking

Trunk
Firefox 6
Bug Flags:
in-testsuite +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Created attachment 525918 [details] [diff] [review]
patch v1
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

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=f3020a90978f
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.
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.
Attachment #525918 - Attachment is obsolete: true
Attachment #526342 - Flags: review?(ian)
Comment on attachment 526342 [details] [diff] [review]
patch v2

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=24d5db1580b0
Comment on attachment 526342 [details] [diff] [review]
patch v2

Looks great!
Attachment #526342 - Flags: review?(ian) → review+
Created attachment 527586 [details] [diff] [review]
patch for checkin
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
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6

Comment 14

6 years ago
Verified on Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
Status: RESOLVED → VERIFIED

Updated

6 years ago
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.