focused tab is opened after naming a group and pressing enter

VERIFIED FIXED in Firefox 6

Status

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: AlexLakatos, Assigned: ttaubert)

Tracking

Trunk
Firefox 6
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 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
Posted 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.
Posted 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
Whiteboard: [fixed in cedar]
Pushed:
http://hg.mozilla.org/mozilla-central/rev/1f5876332b4c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in cedar]
Target Milestone: --- → Firefox 6

Comment 14

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

Updated

8 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.