Closed Bug 165665 Opened 22 years ago Closed 22 years ago

Typeaheadfind not active after set new doc

Categories

(SeaMonkey :: Find In Page, defect, P1)

Product:
SeaMonkey
Component:
Find In Page
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.1final

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

Attachments

(1 file, 1 obsolete file)

Even better. Based on discussion with caillon and jkeiser.
9.45 KB, patch
caillon
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review 
Hmm ... dumb mistake. My last performance enhancement for typeaheadfind
regressed it so that it would shut off when a new page was loaded in the same
window. Duh.

Well, the coming patch fixes that, and implements bz's suggestions for improving
typing performance.
Blocks: isearch
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1final
Seeking r=/sr=
>+void nsTypeAheadAtoms::ReleaseAtoms() {
>+
>+  NS_PRECONDITION(gRefCnt != 0, "bad release of xul atoms");

The precondition error string seems wrong.

> nsTypeAheadFind::~nsTypeAheadFind()
> {
>+  nsTypeAheadAtoms::AddRefAtoms();

That should be ReleaseAtoms(), no?

>+      // Don't steal keys from select element - has its own incremental
>+      // find for options

This is in a block of code that applies to isindex and textarea too...

>+      targetContent->GetAttr(0, nsTypeAheadAtoms::type, inputType);

I think it would be better to include nsINameSpaceManager and use
kNameSpaceID_None instead of 0.

>+        presShell->GetPresContext(getter_AddRefs(presContext));
>+        if (!presContext) {
>+          return NS_OK;

Is this code really needed?  I can't think of any cases when you'd have a
presshell with no prescontext (and in this case you never use presContext).
Comment on attachment 97297 [details] [diff] [review]
Fixes new doc regression and enhancs typing performance as well

>Index: src/nsTypeAheadAtomList.h
>===================================================================
>+ *
>+ * Contributor(s):
>+ * Original Author: Aaron Leventhal (aaronl@netscape.com)

IIRC, Gerv just recently went through files looking for things like this and
did a mass change to fix them to something like:

  Contributor(s):
    Foo <foo@bar.com> (original author)

That was highly annoying and I'd like to avoid another mass change which causes
unnecessary rebuilds of files in dep builds.  Please fix your syntax here and
in the following new files.

<...>

>+
>+  It is recommended (but not strictly necessary) to keep all entries
>+  in alphabetical order
<...>
>+TYPEAHEAD_ATOM(select, "select")
>+TYPEAHEAD_ATOM(isindex, "isindex")
>+TYPEAHEAD_ATOM(textarea, "textarea")
>+TYPEAHEAD_ATOM(input, "input")
>+TYPEAHEAD_ATOM(type, "type")

Shouldn't you adhere to your recommendation?  :-)


>Index: src/nsTypeAheadFind.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/typeaheadfind/src/nsTypeAheadFind.cpp,v
>retrieving revision 1.7
>diff -u -r1.7 nsTypeAheadFind.cpp
>--- src/nsTypeAheadFind.cpp	30 Aug 2002 03:31:48 -0000	1.7
>+++ src/nsTypeAheadFind.cpp	30 Aug 2002 08:03:02 -0000
>@@ -88,7 +88,7 @@
> #include "nsContentCID.h"
> #include "nsLayoutCID.h"
> #include "nsWidgetsCID.h"
>-#include "nsXULAtoms.h"
>+#include "nsTypeAheadAtoms.h"
> #include "nsINameSpaceManager.h"
> #include "nsIWindowWatcher.h"
> #include "nsIObserverService.h"
>@@ -147,6 +147,7 @@
> 
> nsTypeAheadFind::~nsTypeAheadFind()
> {
>+  nsTypeAheadAtoms::AddRefAtoms();
>   RemoveCurrentSelectionListener();
>   RemoveCurrentScrollPositionListener();
>   mTimer = nsnull;
>@@ -257,6 +258,7 @@
>       typeAheadFind->CancelFind();
>     }
>     else if (!typeAheadFind->mStringBundle) {
>+      nsTypeAheadAtoms::AddRefAtoms();
>       // Get ready to watch windows
>       nsresult rv;
>       nsCOMPtr<nsIWindowWatcher> windowWatcher =
>@@ -402,7 +404,7 @@
> 
>   // Add scroll position and selection listeners, so we can cancel
>   // current find when user navigates
>-  GetSelection(presShell, nsnull, getter_AddRefs(mFocusedDocSelCon),
>+  GetSelection(presShell, getter_AddRefs(mFocusedDocSelCon),
>                getter_AddRefs(mFocusedDocSelection)); // cache for reuse
>   AttachNewScrollPositionListener(presShell);
>   AttachNewSelectionListener();
>@@ -444,14 +446,41 @@
>   nsCOMPtr<nsIDOMEventTarget> domEventTarget;
>   nsEvent->GetOriginalTarget(getter_AddRefs(domEventTarget));
> 
>-  nsCOMPtr<nsIDOMNode> targetNode(do_QueryInterface(domEventTarget));
>+  nsCOMPtr<nsIContent> targetContent(do_QueryInterface(domEventTarget));
>+
>+  // ---- Exit early if in form controls that can be typed in ---------
>+
>+  if (!targetContent)
>+    return NS_OK;
>+  if (targetContent->IsContentOfType(nsIContent::eHTML_FORM_CONTROL)) {
>+    nsCOMPtr<nsIAtom> targetTag;
>+    targetContent->GetTag(*getter_AddRefs(targetTag));
>+    if (targetTag == nsTypeAheadAtoms::select || 
>+        targetTag == nsTypeAheadAtoms::textarea ||
>+        targetTag == nsTypeAheadAtoms::isindex) {

I'm not sure about isindex (is that possible to be a tag?) but you may want to
consider QI'ing to nsIFormControl, call GetType() and checking for
NS_FORM_SELECT, NS_FORM_TEXTAREA, NS_FORM_INPUT_TEXT, or NS_FORM_INPUT_FILE.

>+      // Don't steal keys from select element - has its own incremental
>+      // find for options

I think this comment would be more appropriate preceding the if statement
rather than within it.

>+
>+      return NS_OK;
>+    }
>+
>+    if (targetTag == nsTypeAheadAtoms::input) {
>+      nsAutoString inputType;
>+      targetContent->GetAttr(0, nsTypeAheadAtoms::type, inputType);
>+      if (inputType.Equals(NS_LITERAL_STRING("text")) || 
>+          inputType.Equals(NS_LITERAL_STRING("file")))

See my above comment.

>+        return NS_OK;
>+    }
>+  }
>+
>+
>   nsCOMPtr<nsIDOMKeyEvent> keyEvent(do_QueryInterface(aEvent));
> 
>-  // ---------- First analyze keystroke, exit early if possible --------------
>+  // ---------- Analyze keystroke, exit early if possible --------------
> 
>   PRUint32 keyCode, charCode;
>   PRBool isShift, isCtrl, isAlt, isMeta;
>-  if (!keyEvent || !targetNode ||
>+  if (!keyEvent || !targetContent ||
>       NS_FAILED(keyEvent->GetKeyCode(&keyCode)) ||
>       NS_FAILED(keyEvent->GetCharCode(&charCode)) ||
>       NS_FAILED(keyEvent->GetShiftKey(&isShift)) ||

>@@ -495,40 +513,17 @@
<...>
>-  // ---------- Get presshell/prescontext/selection/etc. ------------
>+  // ---------- Get presshell . ------------

Nit: please remove the stray dot.


>Index: src/nsTypeAheadFind.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/typeaheadfind/src/nsTypeAheadFind.h,v
>retrieving revision 1.4
>diff -u -r1.4 nsTypeAheadFind.h
>--- src/nsTypeAheadFind.h	29 Aug 2002 06:27:38 -0000	1.4
>+++ src/nsTypeAheadFind.h	30 Aug 2002 08:03:03 -0000
>@@ -129,8 +129,8 @@
>   void RangeStartsInsideLink(nsIDOMRange *aRange, nsIPresShell *aPresShell, 
>                              PRBool *aIsInsideLink, PRBool *aIsStartingLink);
> 
>-  // GetSelection: if aCurrentNode is nsnull, gets selection for document
>-  void GetSelection(nsIPresShell *aPresShell, nsIDOMNode *aCurrentNode, 
>+  // Get selection and selection controller for current pres shell
>+  void GetSelection(nsIPresShell *aPresShell, 
>                     nsISelectionController **aSelCon, nsISelection **aDomSel);

This would look cleaner if you moved the last param to a separate line or
bumped the 2nd param up.

>   PRBool IsRangeVisible(nsIPresShell *aPresShell, nsIPresContext *aPresContext,
>                          nsIDOMRange *aRange, PRBool aMustBeVisible,
Oh, and <isindex> does not advertise itself as a form control (and in particular
is not of type nsIContent::eHTML_FORM_CONTROL
So actually, if we can get at isindex some other way or if we can get by without
it, then we don't really need the new atoms at all... 
Sorry, late night coding with copy and paste. Will get a new patch up soon.
> So actually, if we can get at isindex some other way or if we can get by without
> it, then we don't really need the new atoms at all... 

Wouldn't we still need the other atoms? I'm not sure what you mean here.
> So actually, if we can get at isindex some other way or if we can get by without
> it, then we don't really need the new atoms at all... 

No, I guess I'll re-iterate this since it seems to have gotten lost in my review
(bz read over it too)

>+  if (!targetContent)
>+    return NS_OK;
>+  if (targetContent->IsContentOfType(nsIContent::eHTML_FORM_CONTROL)) {
>+    nsCOMPtr<nsIAtom> targetTag;
>+    targetContent->GetTag(*getter_AddRefs(targetTag));
>+    if (targetTag == nsTypeAheadAtoms::select || 
>+        targetTag == nsTypeAheadAtoms::textarea ||
>+        targetTag == nsTypeAheadAtoms::isindex) {

QI to nsIFormControl, and call the GetType method.  You can then check against
constants NS_FORM_SELECT, NS_FORM_TEXTAREA, NS_FORM_INPUT_TEXT, or
NS_FORM_INPUT_FILE.  This will alleviate the need for 4 atoms, and the fact that
it's much faster this way is an added bonus.  So if we can find some other way
to deal with isindex, we can kill the atoms alltogether.
Chris,

So would I still check IsContentOfType before QI'ing to nsIFormControl?
Otherwise we would have a lot of failing QI situations.

Isn't <isindex> a form control? Isn't the right solution for us to make isindex
report itself as eHTML_FORM_CONTROL, or something? I don't know much about
isindex, maybe some can help me out here.
> So would I still check IsContentOfType before QI'ing to nsIFormControl?
Otherwise we would have a lot of failing QI situations.

Yes, we want to make sure we're QI'ing only after we have ensured we have an
eHTML_FORM_CONTROL.

> Isn't <isindex> a form control? Isn't the right solution for us to make
isindex report itself as eHTML_FORM_CONTROL, or something? I don't know much
about isindex, maybe some can help me out here.

Yeah it is... but I'm not sure why it's not reporting itself as one.  jkeiser?
isindex does not live in a form.  It does not QI to nsIFormControl.  Therefore
it is not a form control.  Even though it looks like one.  And yes,
IsContentOfType() is your friend :)
John, so what is the optimal way to figure out if an element is an <isindex>? 
Is there some comparable way to do this?  I'd rather not have to build an atoms
table just for this one case...
Well, you could check against a string ... but short of an atom, there's no
other excessively quick way I know of (though mind you, comparing to string is
not too shabby because most tags don't start with "ISIN").  And while it hadn't
occurred to you yet anyway, adding eHTML_ISINDEX to IsContentOfType() would be
evil and Not Recommended.  Just heading that off at the pass :)
> And yes, IsContentOfType() is your friend :)

Except for isindex, right? It won't help me there, and you don't want any
changes to the content type of isindex.
Comment on attachment 97578 [details] [diff] [review]
Even better. Based on discussion with caillon and jkeiser.

>Index: src/nsTypeAheadFind.cpp
>===================================================================
>@@ -444,14 +440,49 @@
>   nsCOMPtr<nsIDOMEventTarget> domEventTarget;
>   nsEvent->GetOriginalTarget(getter_AddRefs(domEventTarget));
> 
>-  nsCOMPtr<nsIDOMNode> targetNode(do_QueryInterface(domEventTarget));
>+  nsCOMPtr<nsIContent> targetContent(do_QueryInterface(domEventTarget));
>+
>+  // ---- Exit early if in form controls that can be typed in ---------
>+
>+  if (!targetContent)
>+    return NS_OK;


Nit: could you add a blank line in here?  There's something about the lack of
one that just doesn't look right to me...


>+  if (targetContent->IsContentOfType(nsIContent::eHTML_FORM_CONTROL)) {
>+    nsCOMPtr<nsIFormControl> formControl(do_QueryInterface(targetContent));
>+    PRInt32 controlType;
>+    formControl->GetType(&controlType);
>+    if (controlType == NS_FORM_SELECT || 
>+        controlType == NS_FORM_TEXTAREA ||
>+        controlType == NS_FORM_INPUT_TEXT ||
>+        controlType == NS_FORM_INPUT_FILE) {
>+      // Don't steal keys from these form controls 
>+      // - selects have their own incremental find for options
>+      // - text fields need to allow typing
>+      return NS_OK;
>+    }
>+  }
>+  else {


Assuming typeahead is for all documents (e.g. XML, etc.) it's probably a good
idea to check for IsContentOfType(nsIContent::eHTML) here before playing with
strings...


>+    // Test for isindex, a deprecated kind of text field
>+    // We're using a string compare because <isindex> is not 
>+    // considered a form control, so it does not support nsIFormControl or
>+    // eHTML_FORM_CONTROL, and it's not worth having a table of atoms just for 
>+    // it. Instead, we're paying for 1 extra string compare per keystroke, which 
>+    // isn't too bad.


One more nit:  Could you make the comment align up better?   (This is one nice
ski hill though!)
Chris, I made changes based on your feedback. Do I have to post a new patch for
these small changes? It means I have to detangle the stuff from some other fixes.
Comment on attachment 97578 [details] [diff] [review]
Even better. Based on discussion with caillon and jkeiser.

r=caillon, though I wish you'd change the declaration in nsTypeAheadFind.h of
GetSelection() to have the first two arguments on the first line (as opposed to
one on the first line, two on the second) since you are still under the 80 char
limit for doing that (or alternately have each arg on its own line)
Attachment #97578 - Flags: review+
Attachment #97297 - Attachment is obsolete: true
Comment on attachment 97578 [details] [diff] [review]
Even better. Based on discussion with caillon and jkeiser.

+  if (NS_FAILED(targetContent->GetDocument(*getter_AddRefs(doc)))) {

You need "|| !doc" here just in case this code gets a targetContent that's not
in a document (how, I don't know, but can try to hook up some sort of bizarre
testcase involving manual event dispatch from JS).

With that and the eHTML_CONTENT check on <isindex>, sr=bzbarsky.
Attachment #97578 - Flags: superreview+
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
vrfy'd fixed with 2002.09.24.08 mozilla trunk builds. tested the following new
page loading situations:

* after clicking a link
* clicking a link in the personal toolbar
* accessing a bookmark from the bookmarks toplevel menu
Status: RESOLVED → VERIFIED
Component: Keyboard: Navigation → Keyboard: Find as you Type
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: