make suiterunner FAYT work with toolkit typeaheadfind

VERIFIED FIXED in seamonkey2.0a2

Status

SeaMonkey
UI Design
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: Robert Kaiser, Assigned: neil@parkwaycc.co.uk)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {fixed-seamonkey2.0})

Trunk
seamonkey2.0a2
fixed-seamonkey2.0
Dependency tree / graph
Bug Flags:
wanted-seamonkey2.0 +
blocking-seamonkey2.0a2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

18.43 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
872 bytes, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: approval-seamonkey2.0a2+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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 :(
(Reporter)

Comment 1

11 years ago
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.
No longer blocks: 344243
Depends on: 344243
(Reporter)

Comment 2

10 years ago
I just filed bug 395688 which could help us in this area.
Filter "spam" on "guifeatures-nobody-20080610".
Assignee: guifeatures → nobody
QA Contact: guifeatures

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
Depends on: 461938, 395688
(Assignee)

Comment 4

9 years ago
Created attachment 347001 [details] [diff] [review]
WIP2

WIP1 was in bug 461938 of course.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #347001 - Flags: review?(stefanh)
Attachment #347001 - Flags: review?(mnyromyr)
Attachment #347001 - Flags: review?(kairo)
Attachment #347001 - Flags: review?(jag)
Attachment #347001 - Flags: review?(iann_bugzilla)
Attachment #347001 - Flags: review?(bugzilla)

Comment 5

9 years ago
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?
(Assignee)

Comment 6

9 years ago
(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

9 years ago
(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

9 years ago
(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.
(Reporter)

Updated

9 years ago
Attachment #347001 - Flags: review?(kairo) → review-
(Reporter)

Comment 9

9 years ago
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.
(Assignee)

Comment 10

9 years ago
(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.
(Assignee)

Comment 11

9 years ago
Created attachment 347079 [details] [diff] [review]
WIP3

This one should be good for most testing, though it might leak!
Attachment #347079 - Flags: review?(kairo)
(Reporter)

Comment 12

9 years ago
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).
Attachment #347079 - Flags: review?(kairo) → review+

Updated

9 years ago
Duplicate of this bug: 464465
Comment on attachment 347001 [details] [diff] [review]
WIP2

I've not really got time to dig into this patch to do a review justice.
Attachment #347001 - Flags: review?(bugzilla)
(Assignee)

Comment 15

9 years ago
Created attachment 348435 [details] [diff] [review]
Alternative approach
Attachment #348435 - Flags: review?(stefanh)
Attachment #348435 - Flags: review?(mnyromyr)
Attachment #348435 - Flags: review?(kairo)
Attachment #348435 - Flags: review?(jag)
Attachment #348435 - Flags: review?(iann_bugzilla)

Comment 16

9 years ago
> 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

9 years ago
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

9 years ago
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.
(Assignee)

Comment 19

9 years ago
(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.
(Assignee)

Comment 20

9 years ago
(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...
(Assignee)

Comment 21

9 years ago
Well, there is focus, but if an element is receiving focus then the window hasn't been set yet when the update happens.
(Reporter)

Updated

9 years ago
Flags: wanted-seamonkey2+
Flags: blocking-seamonkey2.0a2?
(Reporter)

Comment 22

9 years ago
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

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

Updated

9 years ago
Flags: blocking-seamonkey2.0a2? → blocking-seamonkey2.0a2+
(Reporter)

Comment 24

9 years ago
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?
Attachment #348435 - Flags: review?(kairo) → review+
(Assignee)

Comment 25

9 years ago
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.
(Reporter)

Comment 26

9 years ago
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)...

Updated

9 years ago
Attachment #348435 - Flags: review?(jag) → superreview+

Comment 27

9 years ago
Comment on attachment 348435 [details] [diff] [review]
Alternative approach

sr=jag on this one.

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

Comment 28

9 years ago
Created attachment 349961 [details] [diff] [review]
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)
Attachment #347001 - Attachment is obsolete: true
Attachment #347079 - Attachment is obsolete: true
Attachment #348435 - Attachment is obsolete: true
Attachment #349961 - Flags: superreview+
Attachment #349961 - Flags: review?(kairo)
Attachment #348435 - Flags: review?(stefanh)
Attachment #348435 - Flags: review?(mnyromyr)
Attachment #348435 - Flags: review?(iann_bugzilla)
Attachment #347001 - Flags: review?(stefanh)
Attachment #347001 - Flags: review?(mnyromyr)
Attachment #347001 - Flags: review?(jag)
Attachment #347001 - Flags: review?(iann_bugzilla)

Comment 29

9 years ago
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.
Attachment #349961 - Flags: review+
(Assignee)

Comment 30

9 years ago
(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.
(Assignee)

Comment 31

9 years ago
Created attachment 350050 [details] [diff] [review]
For push
Attachment #349961 - Attachment is obsolete: true
Attachment #350050 - Flags: superreview+
Attachment #350050 - Flags: review+
Attachment #349961 - Flags: review?(kairo)
(Assignee)

Comment 32

9 years ago
Pushed changeset 7ed6a4b1c9db to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(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?
Created attachment 350206 [details] [diff] [review]
Package nsTypeAheadFind.js

Adds the new file to the installer/zip file.

Updated

9 years ago
Attachment #350206 - Flags: review?(neil)
(Assignee)

Updated

9 years ago
Attachment #350206 - Flags: review?(neil)
Attachment #350206 - Flags: review+
Attachment #350206 - Flags: approval-seamonkey2.0a2+
Comment on attachment 350206 [details] [diff] [review]
Package nsTypeAheadFind.js

Checked in, changeset dced3f5e4c42.
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! :-) :-) :-)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081127 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

V.Fixed
Status: RESOLVED → VERIFIED
Target Milestone: --- → seamonkey2.0a2

Updated

9 years ago
Depends on: 466994
(Reporter)

Updated

9 years ago
Blocks: 467004
(Reporter)

Comment 38

9 years ago
(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

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

Updated

9 years ago
Blocks: 470175
(Reporter)

Comment 40

8 years ago
Adding fixed-seamonkey2.0 to make it go away from "unfixed wanted" query
Keywords: fixed-seamonkey2.0
You need to log in before you can comment on or make changes to this bug.