567309 - Find does not start with / and does not start "Search for text when I start typing"

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US;
rv:1.9.3a5pre) Gecko/20100520 Minefield/3.7a5pre ID:20100520065946
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US;
rv:1.9.3a5pre) Gecko/20100520 Minefield/3.7a5pre ID:20100520065946

When I use FAYT( "/" ) for the first time after the start of the browser
window, FAYT does not start.
And also "Search for text when I start typing" option fails.

However, If I used FindBar once at least, The both commands function well.

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open HOME( http://www.mozilla.org/projects/minefield/ )
3. Select text on the page
4. keypush "/" and "minefield" ( without quatation marks)

Actual Results:
When I use FAYT( "/" ) for the first time after the start of the browser
window, FAYT does not start.
And also "Search for text when I start typing" option fails.

Expected Results:
FAYT and "Search for text when I start typing" option should be effective.



Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/d5d5ed6d3e1c
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
Gecko/20100518 Minefield/3.7a5pre ID:20100519083331

Fails:
http://hg.mozilla.org/mozilla-central/rev/046c2d2acd3b
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
Gecko/20100518 Minefield/3.7a5pre ID:20100519101258

Push log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d5d5ed6d3e1c&tochange=046c2d2acd3b

Candidate:
Bug 566736  - Lazily initialize the find toolbar

Comment 1

[Alice0775 White]

Sorry, 
please ignore STEP3 of STR in comment#0


Steps to Reproduce:
1. Start Minefield with new profile
2. Open HOME( http://www.mozilla.org/projects/minefield/ )
3. 
4. keypush "/" and "minefield" ( without quotation marks)

Comment 2

[Ilja Sekler]

Linux is affected as well, please change the OS part of "Platform" to "All".

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I just had to downgrade my browser to an older nightly to work around this problem; it was making the browser more or less completely unusable...

Comment 6

[(no longer active)]

The obvious reason is that TAF is implemented inside findbar.xml (see http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#1407 for example), which means that it won't work until the binding is loaded.  I think we need to move the keypress event handler out of findbar.xml and have it initialize the findbar if needed.  We should probably let it live somewhere in toolkit.

Comment 7

[Asaf Romano (gone)]

No. My solution is to use the handler in browser.js, and initialize the findbar there for the first time.

Comment 8

[IU]

Interestingly, if you first press <F3> on a given browser window, "/" works from then on for all tabs in that browser windows.

Comment 9

[Asaf Romano (gone)]

Attachment: patch

Since this is dogfood, I'd like to land the test post-facto.

Comment 10

[Asaf Romano (gone)]

Attachment: patch

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: simple test

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 447601 [details] [diff] [review]
patch

>diff -r 21ca905f5b3e browser/base/content/tabbrowser.xml

>           if (aEvent.altKey)
>             return;

>+          if (!gFindBarInitialized &&
>+              !(aEvent.ctrlKey || aEvent.altKey || aEvent.metaKey) &&

The altKey check is redundant given the earlier one.

I still hate this patch, but I suppose I will begrudgingly r+ it if it lands with a test.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 447706 [details] [diff] [review]
simple test

Many thanks.

Before sending "/", it should be checked that gFindbarInitialized is not set.

Comment 14

[Alice0775 White]

Does this patch solved "Search for text when I start typing"?

Comment 15

[Alice0775 White]

Sorry spam,
please forget comment #14

Comment 16

[Asaf Romano (gone)]

Attachment: as checked in

Comment 17

[:Gavin Sharp [email: [e-mail]]]

It'd be nice to expand the test to also check "'" (I suppose that'd require a new window), and that the find bar actually works.

Comment 18

[Asaf Romano (gone)]

Yes, it would requite a new window.

We already have some tests for the findbar itself.

Comment 19

[Dão Gottwald [:dao]]

I've backed this out as it was a likely cause for the revival of bug 478241.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 447713 [details] [diff] [review]
as checked in

>+          if (!gFindBarInitialized &&
>+              !(aEvent.ctrlKey || aEvent.metaKey) &&
>+              !aEvent.getPreventDefault()) {
>+            var charCode = aEvent.charCode;
>+            if (charCode) {
>+              var char = String.fromCharCode(charCode);

nit: s/var/let/

>+              if (char == "'" || char == "/" ||
>+                  gPrefService.getBoolPref("accessibility.typeaheadfind")) { 

nit: trailing space

... and I think we've consistently switched over to Services.prefs in tabbrowser.xml.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #18)
> We already have some tests for the findbar itself.

Sure, but not the findbar *in Firefox*. We need tests to make sure that our hacky lazy initialization doesn't break something in the findbar widget, e.g.

Comment 23

[Asaf Romano (gone)]

Attachment: patch

I /believe/ that the failures occur due to the new test and not due to the new code in tabbrowsr - likely because of the quick-find timer. So, I'm now calling gFindBar.close manually to avoid any automated findbar stuff.  Hopefully that helps.

Comment 24

[Asaf Romano (gone)]

Attachment: as landed

To be on the safe side, I changed the test to work more like other browser-chrome tests.

http://hg.mozilla.org/mozilla-central/rev/e73cbd31c93e

Watching.

Comment 25

[Alexander L. Slovesnik]

Can Bug 570230 be caused by this bugfix?

Comment 26

[Asaf Romano (gone)]

Commented there.

Comment 27

[Dietrich Ayala (:dietrich)]

posthumous blocking+.