Closed Bug 230401 Opened 16 years ago Closed 15 years ago

Focus not in address bar after opening new Tab by DoubleClick

Categories

(Firefox :: Tabbed Browser, defect, P4, minor)

defect

Tracking

()

RESOLVED FIXED
Firefox1.5

People

(Reporter: mail, Assigned: martijn.martijn)

References

Details

Attachments

(2 files, 7 obsolete files)

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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Contact: bugzilla
Summary: Cursor should be in Address bar after Opening new BrowserTab by DoubleClick → Focus not in address bar after opening new Tab by DoubleClick
*** Bug 234795 has been marked as a duplicate of this bug. ***
Flags: blocking0.9?
Attachment #145681 - Flags: review?(mconnor)
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.
Attachment #145681 - Attachment is obsolete: true
Attachment #145681 - Flags: review?(mconnor)
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 bugzilla-mozilla@dorando.at.
Attachment #150518 - Flags: review?(mconnor)
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?
Flags: blocking0.9? → blocking1.0?
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.
Attachment #150518 - Flags: review?(mconnor)
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 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.
Attachment #150518 - Flags: review?(mconnor)
Flags: blocking1.0? → blocking1.0+
Attached patch An approach without eval (obsolete) — Splinter Review
How about this? Inspired by Ben's checkin from 2004-06-20 01:29.
Assignee: firefox → steffen.wilberg
Attachment #150518 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 150518 [details] [diff] [review]
An implementation independent patch

Two better patches available.
Attachment #150518 - Flags: review?(mconnor)
Attachment #151282 - Flags: review?(mconnor)
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
|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 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?
Attachment #151282 - Attachment is obsolete: true
Attachment #151282 - Flags: review?(mconnor)
Priority: -- → P4
Target Milestone: --- → Firefox1.0
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.
Component: General → Tabbed Browser
This doesn't work at all. When you doubleclick the tabbar, nothing happens.
Really great patch. Do you spot any obvious mistakes?
Remove "on"+.  initEvent, like addEventListener, takes event names without "on".
Doesn't work. I guess this doesn't work without an event listener!?

Jesse, is doCommand() better than eval()?
Yes.  (Where is doCommand implemented?)
Attached patch Another approach (obsolete) — Splinter Review
Just for the fun of it.
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 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.
-> Dorando.
Assignee: steffen.wilberg → bugzilla-mozilla
Status: ASSIGNED → NEW
Flags: blocking-aviary1.0PR+
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.
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0PR-
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.
p4 priority - not a blocker. if a patch fully reviewed materializes, please
nominate for aviary approval. 
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
*** Bug 274534 has been marked as a duplicate of this bug. ***
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.
Flags: blocking-aviary1.1?
*** Bug 284777 has been marked as a duplicate of this bug. ***
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+
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
Attached patch use eval, unbitrotted (obsolete) — Splinter Review
(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.
Assignee: bugzilla-mozilla → steffen.wilberg
Attachment #151275 - Attachment is obsolete: true
Attachment #151952 - Attachment is obsolete: true
Attachment #151975 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #177225 - Flags: review?(bryner)
OS: Windows XP → All
Hardware: PC → All
Target Milestone: Firefox1.0 → Firefox1.1
Any chance of this getting a review and landed before it bitrots again? :)
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
*** Bug 268885 has been marked as a duplicate of this bug. ***
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 ?
Attachment #177225 - Flags: review?(bryner) → review?(neil.parkwaycc.co.uk)
Blocks: majorbugs
No longer blocks: majorbugs
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!
copying from ctrl+T command
Attachment #185279 - Flags: review?
Attachment #185279 - Flags: review? → review?(steffen.wilberg)
Attachment #185279 - Flags: review?(steffen.wilberg) → review?(neil.parkwaycc.co.uk)
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 on attachment 185279 [details] [diff] [review]
Patch without eval()

tabbrowser shouldn't know anything about gURLBar
Attachment #185279 - Flags: review?(neil.parkwaycc.co.uk) → review-
Possible hack:
<children>
  ...
  <data anonid="newtab" xbl:inherits="oncommand=onnewtab"/>
  ...
</children>

document.getAnonymousElementByAttribute(this, "anonid", "newtab").doCommand();
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Attached patch patchv9Splinter Review
Well, this works. But it's probably not good, I guess (or maybe it is).
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?
Attachment #187200 - Flags: review?(benjamin)
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.
Attachment #187200 - Flags: superreview?(mconnor)
Attachment #187200 - Flags: review?(benjamin)
Attachment #187200 - Flags: review+
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
Attachment #187200 - Flags: superreview?(mconnor) → superreview+
Attachment #177225 - Attachment is obsolete: true
Attachment #177225 - Flags: review?(neil.parkwaycc.co.uk)
Assignee: steffen.wilberg → martijn.martijn
Status: ASSIGNED → NEW
Attachment #187200 - Flags: approval-aviary1.1a2?
Comment on attachment 187200 [details] [diff] [review]
patchv9

a=bsmedberg for landing today (6/29)
Attachment #187200 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
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
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: blocking-aviary1.0PR-
Flags: blocking-aviary1.0-
Resolution: --- → FIXED
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? 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I also get those errors with or without this patch.
I suspect that is a different issue.
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. 
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. 
Henrik, can you try with a 20050630 build, with all extensions disabled?
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.
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. 
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
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.
The recent regression on trunk is bug 305872.
QA Contact: bugzilla → tabbed.browser
*** Bug 307433 has been marked as a duplicate of this bug. ***
*** Bug 308739 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.