Typeahead dies when IMG tag commented out in TABLE

VERIFIED FIXED in mozilla1.3alpha

Status

defect
P3
minor
VERIFIED FIXED
17 years ago
11 years ago

People

(Reporter: geoffeg, Assigned: aaronlev)

Tracking

Trunk
mozilla1.3alpha
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

17 years ago
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.
Assignee

Comment 1

17 years ago
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.
Blocks: isearch
Status: UNCONFIRMED → NEW
Depends on: 166419, 166471
Ever confirmed: true
Assignee

Updated

17 years ago
Blocks: 172991

Comment 3

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

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

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

Comment 6

17 years ago
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.
Assignee

Comment 7

17 years ago
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.
Assignee

Comment 8

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

Comment 9

17 years ago
> 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.
Assignee

Comment 10

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

17 years ago
> 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.
Assignee

Comment 12

17 years ago
> 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.
Assignee

Comment 13

17 years ago
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?
Attachment #104193 - Attachment is obsolete: true

Comment 14

17 years ago
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.
Attachment #104711 - Flags: review+
Assignee

Comment 15

17 years ago
Okay, I've removed the extra else.

Seeking sr=.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.3alpha

Comment 16

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

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

Comment 18

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

sr=kin@netscape.com

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?
Attachment #104711 - Flags: superreview+
Assignee

Comment 19

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

Comment 20

17 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
vrfy'd fixed with the test url, using 2002.11.19 comm trunk builds.
Status: RESOLVED → VERIFIED

Updated

17 years ago
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.