175046 - Typeahead dies when IMG tag commented out in TABLE

Status: VERIFIED FIXED

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

Description

[Geoffrey E. Gallaway]

It appears that mozilla's typeahead feature doesn't work when (hold on to your
seats) a IMG tag inside a comment tag inside a table cell that is ABOVE a link.
On the attached URL typing a "p" to select the first link doesn't work, however,
moving the entire <td> line that the image comment is in to below the link fixes
it. Removing the comment tags fixes it. Very strange. It's probably a rare case
but something I thought it should report. Marking as 'minor' because it's the
only place I've been able to find this problem.

Comment 1

[Aaron Leventhal]

This would be fixed if either of the following bugs were fixed:

bug 166419 - nsDOMRange::SetStart() and nsDOMRange::SetEnd() not working with
comment nodes.

bug 166471 - Low level find should not be returning bogus ranges or searching
comment nodes.

Comment 2

[Aaron Leventhal]

Attachment: Check SkipNode() for the node Find starts with, and efficiently check visibility (as little as possible)

Also fixes bug 172991

Seeking r=akkana

Comment 3

[Akkana Peck]

We explicitly didn't check visibility in the find code because
GetPrimaryFrameFor is so slow.  I'd like to see timing comparisons of find with
and without this patch.  Suggestion: load up some huge page like
http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp
and select something at the top of the page, then do a find ((probably
non-typahead) for "asdf" or something like that which isn't in the file, and
time how long it takes.  A stopwatch timing is probably good enough -- if it
isn't measurable with a stopwatch then it's probably close enough to be
worthwhile for the improved accuracy.

If the problem here is comment nodes, that ought to be straightforward to check
for and shouldn't require slowing things down with a GetPrimaryFrameFor.

Kin and Simon, what do you think of this?  And do you know of any
platform-dependant issues with GetPrimaryFrameFor() -- do we have to be
especially careful to test this on any particular platform or with any specific
settings?

Comment 4

[kinmoz]

If I remember correctly, we don't add text frames to the primary frame map hash
table, so the code has to search manually for the frame which might be real slow.

Ick:

+  doc->GetShellAt(0, getter_AddRefs(presShell));

I'm wondering if we should provide a way for find users to register a filter so
that this type of filtering can be controlled at a higher level, where it knows
what presShell context we are using, and allow them to filter other things that
may have frames. (Do <option> elements have frames associated with them?)

Comment 5

[kinmoz]

Btw, there shouldn't be any platform-dependant issues with GetPrimaryFrameFor().

Comment 6

[Aaron Leventhal]

GetPrimaryFrameFor is slow yes, but this will only happen when there is at least
a partial match already. Because of that, I think there is not significant
performance loss.

Comment 7

[Aaron Leventhal]

Akkana, his isn't just for comment nodes, otherwise I wouldn't be checking the
frame -- this patch fixes bug 172991 as well. It fixes find so that it does not
stop on any invisible text.

Personally, I think adding some kind of filter for users of nsIFind is overkill.
All we really need is to avoid finding unrendered text. There are no other bugs
that I know about with <select> or anything else.

Comment 8

[Aaron Leventhal]

I'll do the speed tests today, and post the numbers here.

Comment 9

[Akkana Peck]

> his will only happen when there is at least
> a partial match already. Because of that, I think there is not significant
> performance loss.

For most documents, that's every text node in the document, which can be quite a
large number.  But perhaps the measured performance numbers will show that my
concern is misplaced.

Comment 10

[Aaron Leventhal]

No, not every text node in the document.

It has to have found at least 1 character, and in most cases more, in the
current text node before it to do GetPrimaryFrameFor.

Comment 11

[Akkana Peck]

> No, not every text node in the document.

Sorry, I was misreading the patch and thinking that you were calling the
visibility check from SkipNode.  Calling it from a partial match, as you're
doing, is better -- but I'm concerned that it looks like you'll do the check for
every letter partially matched, as opposed to only once per node.

We just discussed this for a while on #composer, and discussed a couple of ideas
for when to check.  There were two schools of thought:
1. Check only when you get a complete match.  The challenge here is that then
you have to go back and check all the nodes spanned in the found range, not just
the last node.
2. Check before returning a successful match, and also check if we have a
partial match and we're just about to get a new node.  The concern here was that
  if you search for a word that starts with 't', there are still going to be a
lot of nodes in the file that end in 't' so there are still a lot of visibility
checks for a partial match (but a lot fewer than checking on every character
matched).
3. I wondered if you might be able to do something like, only do the check if
matchAnchorNode is null (but I think means that it'll only check at the
beginnings of matches, which wouldn't be enough -- you'd have to do more than
just this).

The one thing we all agreed on was that we definitely need to see performance
numbers.  One good test is to load the lxr page for nsCSSFrameConstructor.cpp
and search for something that will have a lot of partial matches, like nsIASDF.
 Also maybe search for something that will have a lot of partial matches not at
the beginnings of a node, like sIASDF.  And if you can find an equivalent that
matches something commonly found at the ends of text nodes instead of the
beginning, that would also be good.

Comment 12

[Aaron Leventhal]

> I'm concerned that it looks like you'll do the check for
> every letter partially matched, as opposed to only once per node.

-    // Compare
-    if ((c == patc) || (inWhitespace && IsSpace(c)))
+    // Compare, and test for css visibility on both the last char of each node
+    // and the last char of the pattern
+    if (((c == patc) || (inWhitespace && IsSpace(c))) && 
+        ((findex < fragLen - 1 && pindex < patLen) || IsVisibleNode(mIterNode)))

Just to clear things up, this only does the check if we've matched the last
character of a node or if it's the last character of the find string. So, it
only uses at most 1 check per node.

This is the same as:
> 2. Check before returning a successful match, and also check if we have a
> partial match and we're just about to get a new node.  The concern here was 
> that  if you search for a word that starts with 't', there are still going to 
> be a lot of nodes in the file that end in 't' so there are still a lot of 
> visibility checks for a partial match (but a lot fewer than checking on every
> character matched).

I tested this patch in lxr using the NS_TIMELINE macros, and there is a 4%
slowdown when I run in the debugger searching for "asdf" but a 96% slowdown when
I search for "nsIAsdf". 

I think I'll try testing at the very end when we're about to return a match.
I hope people on #composer didn't think I was dumb enough to try testing the
frame after every single character. Yick.

Comment 13

[Aaron Leventhal]

Attachment: Uses IsVisibleNode/GetPrimaryFrameFor only right before returning

I tested with this patch using nsITimelineService, and there is no performance
degradation at all.

This patch also cleans out some of the redundant null checks, and fixes bug
166471 now by making sure we don't return bogus ranges.

Not sure how I should determine which presshell is being used, but I figure if
it's invisible in presshell 0, that indicates we don't want to find it. Anybody
else have an idea on that?

Comment 14

[Akkana Peck]

Comment on attachment 104711 [details] [diff] [review]
Uses IsVisibleNode/GetPrimaryFrameFor only right before returning

>+          ResetAll();
>+          return NS_OK;
>+        }
>+        else {
>+          matchAnchorNode = nsnull;  // This match is no good, continue on in document
>+        }

Since the preceeding clause ends with a return, I'd remove that else { }. 
Otherwise, looks fine, shouldn't hurt performance, and should fix a number of
niggling problems related to returning bad ranges.  r=akkana.

Comment 15

[Aaron Leventhal]

Okay, I've removed the extra else.

Seeking sr=.

Comment 16

[kinmoz]

So I'm curious, if I had something like this in my content:

   <b>foo </b>  <i> bar</i>

Can we still find "foo bar" with these changes?

Comment 17

[kinmoz]

Hmmmm nevermind, I see that you are only checking the end points of the range
for visibility.

Comment 18

[kinmoz]

Comment on attachment 104711 [details] [diff] [review]
Uses IsVisibleNode/GetPrimaryFrameFor only right before returning

sr=[e-mail]

The GetShellAt() call still bugs me, but I don't have an alternative to suggest
other than the filter approach I mentioned above which would push it up to the
level that would know which shell to use and what it would like to skip.

I wonder if using the CSS DOM would be viable alternative?

Comment 19

[Aaron Leventhal]

GlobalWindowImpl::GetComputedStyle also gets the presshell from the docshell. We
could rewrite this using similar code as found in
SimpleDOMNode::GetComputedStyleDeclaration.

Comment 20

[Aaron Leventhal]

checked in

Comment 21

[sairuh (rarely reading bugmail)]

vrfy'd fixed with the test url, using 2002.11.19 comm trunk builds.