230401 - Focus not in address bar after opening new Tab by DoubleClick

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P4)

Description

[Johannes Widmer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

When opening a new Browser Tab using the keys Ctrl+T the adress bar is empty and
the cursor is there. That's cool! But when opening a new Tab by DoubleClick on
an empty part of the TabBar, you first have to click to the adress bar to enter
a location.

Reproducible: Always

Steps to Reproduce:
1. Open new Browser Tab by Double-Click on an empty part of the Tab Bar
Actual Results:  
New Tab opens, but no text cursor in Adress Bar, text "about:blank" in Adress Bar

Expected Results:  
Adress bar should be empty and the text cursor should be there

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Confirming with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)
Gecko/20040107 Firebird/0.7+ (MozJF)

Excepting my adress bar is empty.

Changed summary to find the bug easier.

Comment 2

[Jesse Ruderman]

*** Bug 234795 has been marked as a duplicate of this bug. ***

Comment 3

[Timothy Babytch]

Attachment: fixes tabbrowser.xml in chrome/toolkit.jar#global/bindings/

Comment 4

[Dorando]

Attachment: An implementation independent patch

Comment 5

[Steffen Wilberg]

Comment on attachment 145681 [details] [diff] [review]
fixes tabbrowser.xml in chrome/toolkit.jar#global/bindings/

BrowserOpenTab() is the right thing to do, ctrl-t, the menuitem and the
toolbarbutton all use that function.
But we shouldn't hardcode a function from browser.js in toolkit.

Marking as obsolete and removing review request.

Comment 6

[Steffen Wilberg]

Comment on attachment 150518 [details] [diff] [review]
An implementation independent patch

This calls the function specified by the onnewtab attribute. In case of
Firefox, this is BrowserOpenTab(), see
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.xul#393

Mike, what do you think?
Patch from [e-mail].

Comment 7

[Jesse Ruderman]

I like to avoid using eval() in chrome JS, since it has led to security holes in
the past.  Instead of
  eval(this.parentNode.parentNode.parentNode.getAttribute('onnewtab')
can you do
  this.parentNode.parentNode.parentNode.onnewtab()
or do you hit bug 246720?

Comment 8

[Steffen Wilberg]

Comment on attachment 150518 [details] [diff] [review]
An implementation independent patch

"Eval, besides being slow, provides a good avenue for inserting and running
malicious code":
http://www.mozilla.org/projects/security/components/reviewguide.html

We should consider other solutions here.

Comment 9

[Steffen Wilberg]

Hehe, I read your bug 247606, but not your post here before posting myself.

I just tried this.parentNode.parentNode.parentNode.onnewtab(). Nothing happens
when doubleclicking the tabbar. Instead, the js console shows:

Warning: reference to undefined property
this.parentNode.parentNode.parentNode.onnewtab

Error: this.parentNode.parentNode.parentNode.onnewtab is not a function

Comment 10

[Jesse Ruderman]

Comment on attachment 150518 [details] [diff] [review]
An implementation independent patch

Let's just use eval for now, here and in bug 246719, until bug 246720 is fixed.

Comment 11

[Dorando]

Attachment: An approach without eval

Comment 12

[Steffen Wilberg]

Attachment: implement + use the fireEvent method

How about this? Inspired by Ben's checkin from 2004-06-20 01:29.

Comment 13

[Steffen Wilberg]

Comment on attachment 150518 [details] [diff] [review]
An implementation independent patch

Two better patches available.

Comment 14

[Steffen Wilberg]

Comment on attachment 151282 [details] [diff] [review]
implement + use the fireEvent method

>+      <method name="fireEvent">
This is a copy from
http://lxr.mozilla.org/aviarybranch/source/toolkit/content/widgets/autocomplete
.xml#314

I wonder whether other implemenations are preferable, like
http://lxr.mozilla.org/aviarybranch/source/toolkit/mozapps/downloads/content/do
wnload.xml#57

Comment 15

[Jesse Ruderman]

|new Function()| has the same problems as |eval()|, and it's more verbose, so I
don't see why you would want to use it here.

Comment 16

[Steffen Wilberg]

Comment on attachment 151282 [details] [diff] [review]
implement + use the fireEvent method

I didn't know that.

How about initEvent and dispatchEvent, like used in downloads.xml?

Comment 17

[Jesse Ruderman]

initEvent and dispatchEvent are fine.  You have to be careful with new
Function() and eval() because they treat a string as JavaScript code.  initEvent
and dispatchEvent will probably work here but not in bug 246719.

Comment 18

[Steffen Wilberg]

Attachment: uses init- and dispatchEvent, doesn't work...

This doesn't work at all. When you doubleclick the tabbar, nothing happens.
Really great patch. Do you spot any obvious mistakes?

Comment 19

[Jesse Ruderman]

Remove "on"+.  initEvent, like addEventListener, takes event names without "on".

Comment 20

[Steffen Wilberg]

Doesn't work. I guess this doesn't work without an event listener!?

Jesse, is doCommand() better than eval()?

Comment 21

[Jesse Ruderman]

Yes.  (Where is doCommand implemented?)

Comment 22

[Dorando]

Attachment: Another approach

Just for the fun of it.

Comment 23

[Jesse Ruderman]

Comment on attachment 151975 [details] [diff] [review]
Another approach

You need {} around the body of the "if" since if could be multiple statements. 
Also, setting an event attribute has the same (unlikely) potential security
problems as eval ;)

Comment 24

[Steffen Wilberg]

Comment on attachment 151275 [details] [diff] [review]
An approach without eval

> (Where is doCommand implemented?)

Jesse, I can't find the implementation of doCommand(). Dorando proposes it in
this patch. I guess we should either use this or the eval() patch. Both work
fine.

Comment 25

[Steffen Wilberg]

-> Dorando.

Comment 26

[Wolf [:wolf]]

Please don't mark blocking +/- w/o permission, in the future nominate the flag
by choosing ?.
This isn't critical, and 1.0PR is practically out the door. Blake has already
marked it as a blocker for 1.0.

Comment 27

[Ben S]

Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10

I get something very similar, but slightly different.

When I press Ctrl-T, I always get the focus in the empty page rather than the
address bar.

Comment 28

[Asa Dotzler [:asa]]

p4 priority - not a blocker. if a patch fully reviewed materializes, please
nominate for aviary approval. 

Comment 29

[Phil Ringnalda (:philor)]

*** Bug 274534 has been marked as a duplicate of this bug. ***

Comment 30

[sairuh (rarely reading bugmail)]

I'm not sure if this is the same bug, but in recent trunk builds I often get
into a state where focus ends up in the find toolbar rather than the urlbar for
a new tab. however, I open the new (blank) tab by ctrl+T, almost never by
double-clicking.

1. open a new tab via keyboard, ctrl+T (I'm primarily on Linux fc2).
2. start typing url, presuming that focus goes to Location field.

actual results: the Find toolbar appears and whatever I type ends up there
rather than in the Location bar.

fwiw, I've got accessibility.typeaheadfind set to true.

Comment 31

[Phil Ringnalda (:philor)]

*** Bug 284777 has been marked as a duplicate of this bug. ***

Comment 32

[Anish Patel]

No longer seems to be an issue using the a current nightly.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050305
Firefox/1.0+

Comment 33

[Thomas Andersen]

It has not been fixed. 

Tested with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050306
Firefox/1.0+

Tried with both old 1.01 profile and a new clean profile

Comment 34

[Steffen Wilberg]

Attachment: use eval, unbitrotted

(In reply to comment #10)
> (From update of attachment 150518 [details] [diff] [review] [edit])
> Let's just use eval for now, here and in bug 246719, until bug 246720 is
fixed.

So let's do that for real. This is an updated patch since attachment 150518 [details] [diff] [review] and
every other patch in this bug are bitrotten.

Comment 35

[Steve England [:stevee]]

Any chance of this getting a review and landed before it bitrots again? :)

Comment 36

[Thomas K. (:tom)]

CONFIRMED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317
Firefox/1.0.2

it goes to address bar when i ctrl+t, not doubleclick in tabbar though

Comment 37

[Nickolay_Ponomarev]

*** Bug 268885 has been marked as a duplicate of this bug. ***

Comment 38

[Caleb]

There are 43 votes to this bug, the patch (which is very simple by the looks of
it) has been posted on the 12th of March and still no one can review this simple
yet very annoying bug ?

Comment 39

[Ben Shadwick]

This is similar to bug 200664 I think. Mozilla 1.7.8 focuses on the address bar,
but Firefox 1.0.4 doesn't. Very annoying!

Comment 40

[Mathieu Parent]

Attachment: Patch without eval()

copying from ctrl+T command

Comment 41

[Benjamin Smedberg]

Comment on attachment 177225 [details] [diff] [review]
use eval, unbitrotted

This approach is better than "gURLBar", but I think we should be using a
synthetic DOM event rather than hardcoding the "onnewtab" attribute.

Comment 42

[Benjamin Smedberg]

Comment on attachment 185279 [details] [diff] [review]
Patch without eval()

tabbrowser shouldn't know anything about gURLBar

Comment 43

[[e-mail]]

Possible hack:
<children>
  ...
  <data anonid="newtab" xbl:inherits="oncommand=onnewtab"/>
  ...
</children>

document.getAnonymousElementByAttribute(this, "anonid", "newtab").doCommand();

Comment 44

[Martijn Wargers (dead)]

Attachment: patchv9

Well, this works. But it's probably not good, I guess (or maybe it is).

Comment 45

[Martijn Wargers (dead)]

Comment on attachment 187200 [details] [diff] [review]
patchv9

So maybe this is the right approach?
This is basically what you suggested in comment 41, not?

Comment 46

[Benjamin Smedberg]

Comment on attachment 187200 [details] [diff] [review]
patchv9

Yes, this is what I wanted exactly. I would like mconnor to review this also in
case I'm missing something.

Comment 47

[Mike Connor [:mconnor]]

Comment on attachment 187200 [details] [diff] [review]
patchv9


>-            if (aEvent.originalTarget.localName != 'tab' && aEvent.button == 0)
>-              this.selectedTab = this.addTab();
>+            if (aEvent.originalTarget.localName != 'tab' && aEvent.button == 0) {
>+              var e = document.createEvent("Events");
>+              e.initEvent("NewTab", false, true);
>+             this.dispatchEvent(e);
>+            }

r=me, with the indentation fixed here

Comment 48

[Benjamin Smedberg]

Comment on attachment 187200 [details] [diff] [review]
patchv9

a=bsmedberg for landing today (6/29)

Comment 49

[Asaf Romano (gone)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.445; previous revision: 1.444
done
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.88; previous revision: 1.87
done

Comment 50

[Henrik Skupin [:whimboo][⌚️UTC+1]]

No, this is definitely not fixed for my current Linux cvs debug build
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050629 Firefox/1.0+

Both methods, Ctrl+T and double click don't set the cursor into the address bar.
I've to press Ctrl+L manually.

I get a js error and warning when doing the above actions:

Error: this._fastFind.setDocShell is not a function
Source File: chrome://global/content/bindings/tabbrowser.xml
Line: 653

Warning: reference to undefined property this.fastFind.searchString
Source File: chrome://global/content/bindings/browser.xml
Line: 270

I don't know if there is the fault? 

Comment 51

[Martijn Wargers (dead)]

I also get those errors with or without this patch.
I suspect that is a different issue.

Comment 52

[Tim Meader]

I would vote to close this and open a seperate bug for Linux only (or reset it
to Linux specific) as the latest build "Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.8b2) Gecko/20050630 Firefox/1.0+ ID:2005063006" is working fine. The
cursor is focused in the location bar as it should be when "double-click
opening" a new tab. And I'm not seeing the reported JS errors either.

Also tested with today's build on my Mac. So, for Mac and Win anyhow...
confirmed fixed. 

Comment 53

[Magnus Melin [:mkmelin]]

FWIW, this is working fine for me, Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.8b2) Gecko/20050630 Firefox/1.0+. No JS errors either. 

Comment 54

[Tim Meader]

Henrik, can you try with a 20050630 build, with all extensions disabled?

Comment 55

[Szymon Konarski]

Tim: WFM with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050630 Firefox/1.0+ ID:2005063006 with new, no extension profile.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, I've to confirm that this is fixed. My CVS build seems to be broken. The
latest trunk build Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2)
Gecko/20050630 Firefox/1.0+ from ftp.mozilla.org works fine. 

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Just for additional information: I hardly checked why it doesn't work with my
CVS build. I found the solution. I always build with --enable-extensions=all.
This will cause the mentioned errors because the extension typeaheadfind uses
the same interface like FastFind and is not compatible with Firefox. When I
build without it the errors aren't shown anymore. Also Ctrl-T and double-click
work like expected. More info about the fastfind bug you can find in bug 292904.

Comment 58

[Mike Pesce (:By-Tor)]

This bug seems to have resurfaced in Mozilla/5.0 (Windows; U; Windows NT 5.1;
en-US; rv:1.9a1) Gecko/20050826 Firefox/1.6a1 ID:2005082604.  The location bar
no longer gets focus when double clicking to open a new tab.

Comment 59

[Jesse Ruderman]

The recent regression on trunk is bug 305872.

Comment 60

[Phil Ringnalda (:philor)]

*** Bug 307433 has been marked as a duplicate of this bug. ***

Comment 61

[Jesse Ruderman]

*** Bug 308739 has been marked as a duplicate of this bug. ***