"find previous" feature



18 years ago
14 years ago


(Reporter: doronr, Assigned: doronr)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: need feedback)


(8 attachments)



18 years ago
We need a find previous method where we can via a menuitem find the previous
occurance (needed for example in view source context menu bug).  I have a
working patch attached, which probably needs to be polished and feedback.

Currently, this is how I do it:

I modified nsFindComponent::FindNext to have a additional variable (PRBool)
passed to it called aFindPrevious.  If it is true, I set SearchBackwards true,
and after the search, reset it to the original value.  So if nothing was
searched for and search previous is invoked, the SearchBackwards checkbox will
be checked.

I also added changes to nsBrowserInstance to work with this, even though I
understand that is being killed.

It might make more sense to use a aFindNext variable, so feedback is welcome.  I
originally had a FindPrevious() function, but thought that combining the 2 into
one function was better.

Please keep in mind that the patch attached needs cleanup and has useless
fprintf()'s for some simple debug work commented out...

Comment 1

18 years ago
Created attachment 32292 [details] [diff] [review]
initial patch, needs cleanup and feedback!


18 years ago
Blocks: 39389

Comment 2

18 years ago
Eventually, Simon should be one of the reviewers for this patch.
Severity: normal → enhancement
Keywords: patch
Hardware: PC → All
Whiteboard: need feedback

Comment 3

18 years ago
This would be trivial to do, but that patch probably needs to change now that I 
checked in changes to how Find works.

Comment 4

18 years ago
Created attachment 33645 [details] [diff] [review]

Comment 5

18 years ago
the new patch is only backend, all js calls to findnext() need just to pass a n
additional boolean (findPrevious), like in findutils.js and the rest.

Comment 6

18 years ago
I think this is the wrong approach. I think you just need to make a 
'findPreviousInPage()' in findUtils.js that sets findInst.findBackwards to false, 
and not change any of the C++.

Comment 7

18 years ago
hmm, ok, i had that before but changed it to c++. Going to attach a new patch
that does this via js

Comment 8

18 years ago
Created attachment 34307 [details] [diff] [review]
trivial patch

Comment 9

18 years ago
patch attached, simply always sets findBackwards to true. Trivial

Comment 10

18 years ago
See, wasn't that easy  :)

You might like to factor the common code that you copy/pasted into a new 
function, since there is quite a lot of it.

Comment 11

18 years ago
Created attachment 34527 [details] [diff] [review]
new patch

Comment 12

18 years ago
new patch, created a setFindInPage that returns a findInst.  Any other comments?
Assignee: pchen → doronr

Comment 13

18 years ago
How about 'initFindInPage' rather than 'setFindInPage'. Other than that, r=

Comment 14

18 years ago
So if findBackwards is set to true, won't FindNext do the same thing as 

Should we modify FindNext to always look forwards too?

What about the findAgain keyboard shortcut, what should it do?

Comment 15

18 years ago
if you select the backwards checkbox in the finddialog, then find next should
observe that. That is how we used to do it. However, now that there is a find
previous, I guess we can change this. should I?

I'll also file a bug about giving it a shortcut and menuitem.

Comment 16

18 years ago
The problem here is that having a findPreviousInPage function make the usage of 
these fuctions ambiguous. findNextInPage may either find forwards or backwards, 
depending on the service settings. Your findPreviousInPage will aways find 
backwards. So maybe findNextInPage should always find forwards?

Comment 17

18 years ago
Created attachment 34616 [details] [diff] [review]
another patch

Comment 18

18 years ago
changes implemented.

Comment 19

18 years ago
so I assume r=sfraser, anyone up for a sr=?

Comment 20

18 years ago
What about the associated XUL? If you check this in as is, you'll break Find Next 
when the user was last finding backwards.

Comment 21

18 years ago
ok, i will talk to the UI people about adding "find previous" and giving it a
keycombo and renaming "find again" to "find next". I have it working in my tree
but not sure what keycombo to give them

Comment 22

18 years ago
Created attachment 35402 [details] [diff] [review]
Patch - findutils.js

Comment 23

18 years ago
Created attachment 35403 [details] [diff] [review]
Patch - Navigator

Comment 24

18 years ago
Created attachment 35404 [details] [diff] [review]
Patch - Mailnews

Comment 25

18 years ago
ok, attached 3 patches

as per mpt, changed the accesskeys and keycombos. 

findutils.js: the original find changes, renamed the function to use "next"
instead of "again", also canFindAgain() is canFind() (as it is for both previous
and next).

navigator: ui changes (renaming "again" to "next" and adding "previous")
mailnews: ui changes (renaming "again" to "next" and adding "previous")

I tested both mail and navigator and did  not see any regressions. find next and
find previous worked.  I did not patch composer as it does not seem to use

ccing blake for the nav changes and sspitzer for mailnews changes.
Target Milestone: --- → mozilla0.9.2

Comment 26

18 years ago
This needs running past the UI folks.
Well, the Ctrl+G, Ctrl+Shift+G solution seems logical. 

code wise, do we need BrowserCanFind(); when all it does is call canFindInPage();? 

Comment 28

18 years ago
>do we need BrowserCanFind(); when all it does is call canFindInPage();? 

Yeah, it's consistent with the call levels of the other find-related functions, 
and a bottleneck that could be enhanced later.

Comment 29

18 years ago
0.9.2 freeze is 7 days away. Do you want this to go in?

Comment 30

18 years ago
-> 0.9.3, 0.9.2 is too close
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 31

18 years ago
So other than BrowserCanFind(), any other approval this needs?

Comment 32

18 years ago
Target Milestone: mozilla0.9.3 → mozilla1.0

Comment 33

17 years ago
future I guess
Target Milestone: mozilla1.0 → Future
mass moving open bugs pertaining to find in page/frame to pmac@netscape.com as
qa contact.

to find all bugspam pertaining to this, set your search string to
QA Contact: sairuh → pmac

Comment 35

17 years ago
Why was this futured if there's a patch...?
*** Bug 157772 has been marked as a duplicate of this bug. ***

Comment 37

17 years ago
Doron, what's happened with this? You had a fix and everything.
Keywords: nsbeta1

Comment 38

17 years ago
missing module owner approval :)

If people agree, I can update this probably to work.

Comment 39

17 years ago
I don't think you need menu items, just the accelerator. Menu items might bloat
the menus too much.

However, that's just my opinion. If you decide to go with just the accelerator,
then I give my approval as keyboard component owner.

Comment 40

17 years ago
checked in
Last Resolved: 17 years ago
Resolution: --- → FIXED
aaronl, since the fix for this was part of bug 167921 (which dealt with
typeahead find), will this appear in only mozilla builds, or both moz and

Comment 42

17 years ago
It will appear in commercial too.

Doron, sorry I didn't mean to take this from under you -- it just involved a
bunch of stuff I was doing with typeaheadfind, so it made sense to do it all at
QA Contact: pmac → sairuh
vrfy'd fixed on both commercial and mozilla trunk builds, 2002.09.26.08. Find
Previous is now under the Edit menu; accel-shift-G works as the shortcut (as
does shift-F3).

Comment 44

17 years ago
*** Bug 172612 has been marked as a duplicate of this bug. ***
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.