345526 - make suiterunner FAYT work with toolkit typeaheadfind

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

The recent patch in bug 344243 includes the necessary changes to change suiterunner from building extensions/typeaheadfind to building toolkit/components/typeaheadfind instead.

SeaMonkey does not call FAYT any more with that change though, we need to make sure we are able to enable that feature through toolkit again.

I tried to dig into that, but I'm coming into regions where my knowledge ends: it looks to me that we're relying on nsTypeAheadController <http://lxr.mozilla.org/mozilla/source/extensions/typeaheadfind/src/nsTypeAheadFind.cpp#2821> for doing FAYT, but that does not exist in toolkit's nsTypeAheadFind - but as I don't really know the C/C++ stuff, that's where my investigations come to a halt :(

Comment 1

[Robert Kaiser]

Updating this bug with a bit of changed information, see also bug comment #23 and following.

suiterunner is now building both the old suitetypeaheadfind from extensions and the new typeaheadfind from toolkit, but those are two completely independent sets of code now.
We should probably do a version of that feature that provides everything we need in suite but based on the toolkit one, so we can share some code and don't need to maintain forked versions of the same code.

Comment 2

[Robert Kaiser]

I just filed bug 395688 which could help us in this area.

Comment 3

[Serge Gautherie (:sgautherie)]

Filter "spam" on "guifeatures-nobody-20080610".

Comment 4

[[e-mail]]

Attachment: WIP2

WIP1 was in bug 461938 of course.

Comment 5

[Stefan [:stefanh]]

Comment on attachment 347001 [details] [diff] [review]
WIP2

I tested this in browser and in the mailnews start page - for some reason I cant type any text in msgcompose atm. This looks like it's working quite well. Compared to the previous version the shortcuts works as well ;)

Some notes:
- accessibility.typeaheadfind.autostart works
- There was an oddity when I started to test. When I first tried and set the above pref to false, it looked like accessibility.typeaheadfind.linksonly was ignored, it was like it always was set to false (even if it wasn't) and only the "/" key worked. This doesn't happen now, though. For some reason both keys invokes the right fayt now.

- accessibility.typeaheadfind.soundURL pref doesn't really work, there's something odd going on there, I don't seem to get the default sound when nothing is found.   At the best a get some sound, but nothing that resembles the default sound (works fine in Firefox, though). I don't really think this should stop anything here, though.  Maybe we miss something in the backend?

Comment 6

[[e-mail]]

(In reply to comment #5)
> (From update of attachment 347001 [details] [diff] [review])
> I tested this in browser and in the mailnews start page - for some reason I
> cant type any text in msgcompose atm.
Ah yes, I still need to test for editable regions (including composer).

> - accessibility.typeaheadfind.soundURL pref doesn't really work, there's
> something odd going on there, I don't seem to get the default sound when
> nothing is found.   At the best a get some sound, but nothing that resembles
> the default sound (works fine in Firefox, though).
Note that Firefox defaults to "beep", rather than "default".

Comment 7

[Stefan [:stefanh]]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 347001 [details] [diff] [review] [details])
> > I tested this in browser and in the mailnews start page - for some reason I
> > cant type any text in msgcompose atm.
> Ah yes, I still need to test for editable regions (including composer).
> 
> > - accessibility.typeaheadfind.soundURL pref doesn't really work, there's
> > something odd going on there, I don't seem to get the default sound when
> > nothing is found.   At the best a get some sound, but nothing that resembles
> > the default sound (works fine in Firefox, though).
> Note that Firefox defaults to "beep", rather than "default".

You want the "beep", works fine with that. One oddity is that it seems that I need to restart sm in order to make a change in the pref work. But that could be a system thing.

Comment 8

[Stefan [:stefanh]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (From update of attachment 347001 [details] [diff] [review] [details] [details])
> > > I tested this in browser and in the mailnews start page - for some reason I
> > > cant type any text in msgcompose atm.
> > Ah yes, I still need to test for editable regions (including composer).
> > 
> > > - accessibility.typeaheadfind.soundURL pref doesn't really work, there's
> > > something odd going on there, I don't seem to get the default sound when
> > > nothing is found.   At the best a get some sound, but nothing that resembles
> > > the default sound (works fine in Firefox, though).
> > Note that Firefox defaults to "beep", rather than "default".
> 
> You want the "beep", works fine with that. One oddity is that it seems that I
> need to restart sm in order to make a change in the pref work. But that could
> be a system thing.

Same thing in Firefox (restart), so nothing you should worry about here I guess. Note that the "default" sound do output a sound, but it's kind of bizarre and doesn't follow the system prefs. "beep" follows the system settings, though.

Comment 9

[Robert Kaiser]

Comment on attachment 347001 [details] [diff] [review]
WIP2

I can confirm this patch breaks entering any text anywhere in messengercompose (I tested typing address, subject and content, nothing resulting in any entered character) but entering in webforms or urlbar in browser works.

I did get this in error console a few times from messengercompose:
Error: this.mXULBrowserWindow is null
Source File: chrome://communicator/content/utilityOverlay.js
Line: 1001

Without the patch, everything works. This prevents me from testing the patch in dogfood situations, so r- for this WIP.

Comment 10

[[e-mail]]

(In reply to comment #8)
> Note that the "default" sound do output a sound, but it's kind of
> bizarre and doesn't follow the system prefs.
My bad, bug 193923 changed it for Windows and I didn't realise that it had always been beep on the Mac.

(In reply to comment #9)
> (From update of attachment 347001 [details] [diff] [review])
> I can confirm this patch breaks entering any text anywhere in messengercompose
> (I tested typing address, subject and content, nothing resulting in any entered
> character) but entering in webforms or urlbar in browser works.
I can type in the address and subject, but I realise content is broken.

> I did get this in error console a few times from messengercompose:
> Error: this.mXULBrowserWindow is null
> Source File: chrome://communicator/content/utilityOverlay.js
> Line: 1001
Right, I know what causes this and I need to ignore it.

Comment 11

[[e-mail]]

Attachment: WIP3

This one should be good for most testing, though it might leak!

Comment 12

[Robert Kaiser]

Comment on attachment 347079 [details] [diff] [review]
WIP3

r=me on the functionality - I don't pretend to understand the code, so I didn't really read through it, and I didn't test for leaks or anything like that, but it does what I need it to do for the moment (i.e. FAYT in browser and not killing any functionality I rely on elsewhere).
Please hand to someone who really understands this for actual code review.
I also hope this is a good base on which we can build on for pref-switchable FAYTbar and replacing the find dialog with a findbar (all in further steps/followups).

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 347001 [details] [diff] [review]
WIP2

I've not really got time to dig into this patch to do a review justice.

Comment 15

[[e-mail]]

Attachment: Alternative approach

Comment 16

[Philip Chee]

> diff -r 47a9189abb80 suite/browser/nsTypeAheadFind.js
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/suite/browser/nsTypeAheadFind.js	Sun Nov 16 15:44:23 2008 +0000
> @@ -0,0 +1,316 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

No licence boilerplate?

Comment 17

[jag (Peter Annema)]

I like the alternative approach, but I would use

  findTypeController.prototype = {
    ...
  }

as you did for |typeAheadFind|.

Why did you split the controller off, though?

Do you intend to keep the try/catch in |isCommandEnabled()|?

For readability there might be some value in hoisting some of the interface names up to global (or at least function level) consts, or use |C.i.|.

Comment 18

[Philip Chee]

I d.i.s.a.g.r.e.e.w.i.t.h.t.h.e.u.s.e.o.f. C.i. a.n.d f.r.i.e.n.d.s.

Comment 19

[[e-mail]]

(In reply to comment #17)
> Why did you split the controller off, though?
Each controller needs to know which window it's attached to.

> Do you intend to keep the try/catch in |isCommandEnabled()|?
Oops, I forgot to debug the reason that I put it in there...

> For readability there might be some value in hoisting some of the interface
> names up to global (or at least function level) consts, or use |C.i.|.
Sure, but there were only about 5 that I actually use more than once.

Comment 20

[[e-mail]]

(In reply to comment #19)
> (In reply to comment #17)
> > Do you intend to keep the try/catch in |isCommandEnabled()|?
> Oops, I forgot to debug the reason that I put it in there...
Someone updates the command during a focus event, so there's no focus yet...

Comment 21

[[e-mail]]

Well, there is focus, but if an element is receiving focus then the window hasn't been set yet when the update happens.

Comment 22

[Robert Kaiser]

The latest patch doesn't apply cleanly on current trunk because somebody changed the "DIRS" in the browser/Makefile.in context to "PARALLEL_DIRS" :p

Comment 23

[Philip Chee]

I think the Fairy Dust may be backed out if Thunderbird can't be built with "PARALLEL_DIRS"

Comment 24

[Robert Kaiser]

Comment on attachment 348435 [details] [diff] [review]
Alternative approach

r=me for how it works in the build (no code review), just one nit: shouldn't we reset the position to start the find to the start of the document when we start a new find?

Comment 25

[[e-mail]]

No, the idea is to find from where the selection, caret or focus is, or as near on screen from one of them as possible. Ironically that's the very code that bug 461212 busted which made bug 461938 and thus this bug necessary.

Comment 26

[Robert Kaiser]

Can we get real code review on whatever patch Neil and probable reviewers prefer? We should not ship a2 without any FAYT and the freeze is upcoming (tomorrow)...

Comment 27

[jag (Peter Annema)]

Comment on attachment 348435 [details] [diff] [review]
Alternative approach

sr=jag on this one.

Remove the dump(), and use the .prototype = { ... } style for findTypeController.

Comment 28

[[e-mail]]

Attachment: Final patch

* Fixed findTypeController.isCommandEnabled
* Switched prototype style as requested by jag
* Added lots of comments (including licence block)
* Minor clean up (code reordering to make it easier to comment)

Comment 29

[Karsten Düsterloh]

Comment on attachment 349961 [details] [diff] [review]
Final patch

Just some nits:

>+  switch (pref.getIntPref("browser.backspace_action")) {
>+  case 0:
>+    BrowserBack();
>+    break;
>+  case 1:
>+    goDoCommand("cmd_scrollPageUp");
>+    break;

Cases and their code lack one level of indentation.

>+    if (aEvent.keyCode || aEvent.charCode <= " ".charCodeAt(0))

You do like charCodeAt, do you? ;-)
Better just ...

>+    switch (aEvent.charCode) {
>+      // Start finding text as you type
>+      case "/".charCodeAt(0):
>+        aEvent.preventDefault();
>+        this.startFind(window, false);
>+        break;
>+
>+      // Start finding links as you type
>+      case "'".charCodeAt(0):
>+        aEvent.preventDefault();
>+        this.startFind(window, true);
>+        break;

... make the charCodeAt values const members of typeAheadFind.prototype and use those.

>+  showStatusMatch: function(aResult, aExtra) {
>+    // Set the status text from a find result
>+    // link|text "..." [not] found [next|previous match] [(href)]
>+    if (aExtra)
>+      aExtra = " " + this.mBundle.GetStringFromName(aExtra);
>+    var url = "";
>+    var string = this.mLinks ? "link" : "text";
>+    if (aResult == Components.interfaces.nsITypeAheadFind.FIND_NOTFOUND)
>+      string += "not";
>+    else if (this.mFind.foundLink && this.mFind.foundLink.href)
>+      url = "  " + this.mBundle.GetStringFromName("openparen") +
>+                   this.mFind.foundLink.href +
>+                   this.mBundle.GetStringFromName("closeparen");
>+    string += "found";
>+    this.showStatus(this.mBundle.GetStringFromName(string) +
>+                    this.mSearchString +
>+                    this.mBundle.GetStringFromName("closequote") +
>+                    aExtra + url);
>+  },

This looks rather messy to me from a L10N point of view. Shouldn't this be one L10N string with placeholders to be filled in, because $language could have weird ideas about word order and such? KaiRo?

>+      // Ignore control characters.
>+      if (aEvent.keyCode || aEvent.charCode < " ".charCodeAt(0))

See above.

r=me with that.

Comment 30

[[e-mail]]

(In reply to comment #29)
>(From update of attachment 349961 [details] [diff] [review])
>>+  switch (pref.getIntPref("browser.backspace_action")) {
>>+  case 0:
>>+    BrowserBack();
>>+    break;
>>+  case 1:
>>+    goDoCommand("cmd_scrollPageUp");
>>+    break;
>Cases and their code lack one level of indentation.
Ironically all I did was unindent this because it used to be an inner block ;-)

> ... make the charCodeAt values const members of typeAheadFind.prototype
kSpace, kApos, kSlash?

>This looks rather messy to me from a L10N point of view.
Sure, but the patch itself is L10N neutral because the old code did this.

Comment 31

[[e-mail]]

Attachment: For push

Comment 32

[[e-mail]]

Pushed changeset 7ed6a4b1c9db to comm-central.

Comment 33

[Justin Wood (:Callek)]

(In reply to comment #30)
> (In reply to comment #29)
> >This looks rather messy to me from a L10N point of view.
> Sure, but the patch itself is L10N neutral because the old code did this.

Can we get a followup bug on file for this, and mark it as a dependancy?

Comment 34

[Frank Wein [:mcsmurf]]

Attachment: Package nsTypeAheadFind.js

Adds the new file to the installer/zip file.

Comment 35

[Frank Wein [:mcsmurf]]

Comment on attachment 350206 [details] [diff] [review]
Package nsTypeAheadFind.js

Checked in, changeset dced3f5e4c42.

Comment 36

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081126 SeaMonkey/2.0a2pre - Build ID: 20081126132932

Verified for Linux on this tinderbox-build: the / key triggers FAYT again.
:-) :-) :-) Hip, hip, hip, HURRAH! :-) :-) :-)

Comment 37

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081127 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

V.Fixed

Comment 38

[Robert Kaiser]

(In reply to comment #33)
> (In reply to comment #30)
> > (In reply to comment #29)
> > >This looks rather messy to me from a L10N point of view.
> > Sure, but the patch itself is L10N neutral because the old code did this.
> 
> Can we get a followup bug on file for this, and mark it as a dependancy?

Filed as bug 467004.

Comment 39

[doctor__j]

Bug 470175 was filed to address the timeout problem of "Find as you type".

Comment 40

[Robert Kaiser]

Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query