Closed Bug 517866 Opened 15 years ago Closed 15 years ago

Find completely broken on xml documents

Categories

(Toolkit :: Find Toolbar, defect, P2)

Product:
Toolkit
Component:
Find Toolbar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- beta2-fixed

People

(Reporter: khuey, Assigned: enndeakin)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(1 file)

restore focus check
2.32 KB, patch
neil
: review+
Details | Diff | Splinter Review 
Spun off from bug 517859.

The find feature no longer works on xml documents.  Type any character and nothing will match whatsoever.  Find has had problems on xml documents for a while (Bug 263049 for instance) but this is definitely a regression between 3.5.3 and trunk.

Steps to reproduce:
1) Load the URL
2) Open the find toolbar
3) Type any character in the toolbar

Observed behavior:
Nothing matches

Expected behavior:
Finds stuff appropriately.
Flags: blocking1.9.2?
Observed on Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090920 Minefield/3.7a1pre
Interestingly if you then proceed to select and deselect part of the document it proceeds fine.
Vlad, any chance this is from bug 512643?
Flags: blocking1.9.2? → blocking1.9.2+
Keywords: qawanted
Priority: -- → P2
(In reply to comment #2)
> Interestingly if you then proceed to select and deselect part of the document
> it proceeds fine.

I'm seeing the problem on both Win XP and OS X 10.5.8.  All I need to do to make find work on the XML document is to simply click somewhere on the document.  Could this just be a focus problem?
OS: Windows 7 → All
Hardware: x86_64 → All
(In reply to comment #4)
> (In reply to comment #2)
> > Interestingly if you then proceed to select and deselect part of the document
> > it proceeds fine.
> I'm seeing the problem on both Win XP and OS X 10.5.8.  All I need to do to
> make find work on the XML document is to simply click somewhere on the
> document.  Could this just be a focus problem?

Possibly.  I will investigate tonight.
Also broken on linux - and seems to be focus related, however, why would the focus have anything to do with searching the data in the page?
Yep this is caused by Bug 178324.
Blocks: 178324
Keywords: qawanted
It doesn't work in 3.5 either.
No longer blocks: 178324
(In reply to comment #9)
> It doesn't work in 3.5 either.

Are you sure you understand the bug correctly?  It definitely works for me on 3.5
(In reply to comment #10)
> (In reply to comment #9)
> > It doesn't work in 3.5 either.
> 
> Are you sure you understand the bug correctly?  It definitely works for me on
> 3.5

It works for me on the revision just before the one for bug 178324.
Well for me (and also for Joe), it is very unreliable on 3.5.
(In reply to comment #12)
> Well for me (and also for Joe), it is very unreliable on 3.5.

Interesting.  I'm basically just launching the test URL from the commandline:

  firefox https://adwords.google.com/api/adwords/v12/CampaignService?wsdl 

then as soon as it loads,

  ctrl-f, then "api"

this does a find in the older builds, but in the newer ones it claims it can't even find "a".

Note that simply clicking inside the page will make it work.

I should have a build for the next revision up (the one with the bug 178324 patch) shortly.
I'm doing almost exactly what Curtis is doing.  (Just not from the commandline)  It works on http://hg.mozilla.org/mozilla-central/rev/771509552a2e.  On http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 it's broken.
(In reply to comment #14)
> I'm doing almost exactly what Curtis is doing.  (Just not from the commandline)
>  It works on http://hg.mozilla.org/mozilla-central/rev/771509552a2e.  On
> http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 it's broken.

It turns out this bug (or a very similar one) can be reproduced on Firefox 3.5.1 using slightly different repro steps.  It will work the first time using my steps from above.  If you then dismiss the find bar with escape and open it again, then it won't work.

That said, there is a definite change in behavior somewhere close to the revisions you mention, after which it becomes much easier to repro the bug.
It's probably superfluous at this point but I can confirm Kyle Huey's findings.  Using the repro steps in comment #13, I get:

http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 (r.29018) doesn't work
http://hg.mozilla.org/mozilla-central/rev/771509552a2e (r.29017) find works

FWIW
(In reply to comment #15)
> (In reply to comment #14)
> > I'm doing almost exactly what Curtis is doing.  (Just not from the commandline)
> >  It works on http://hg.mozilla.org/mozilla-central/rev/771509552a2e.  On
> > http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 it's broken.
> It turns out this bug (or a very similar one) can be reproduced on Firefox
> 3.5.1 using slightly different repro steps.  It will work the first time using
> my steps from above.  If you then dismiss the find bar with escape and open it
> again, then it won't work.
> That said, there is a definite change in behavior somewhere close to the
> revisions you mention, after which it becomes much easier to repro the bug.

Hmm I can't seem to reproduce that on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090925 Shiretoko/3.5.4pre.  Is it intermittent or does it always happen?  I tried following the steps from comment 13 using "+" as the search string (which does not appear in the document).  Then I hit Esc and then Ctrl-F again and try "api" but it finds it like it should.
(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > I'm doing almost exactly what Curtis is doing.  (Just not from the commandline)
> > >  It works on http://hg.mozilla.org/mozilla-central/rev/771509552a2e.  On
> > > http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 it's broken.
> > It turns out this bug (or a very similar one) can be reproduced on Firefox
> > 3.5.1 using slightly different repro steps.  It will work the first time using
> > my steps from above.  If you then dismiss the find bar with escape and open it
> > again, then it won't work.
> > That said, there is a definite change in behavior somewhere close to the
> > revisions you mention, after which it becomes much easier to repro the bug.
> 
> Hmm I can't seem to reproduce that on Mozilla/5.0 (Windows; U; Windows NT 5.1;
> en-US; rv:1.9.1.4pre) Gecko/20090925 Shiretoko/3.5.4pre.  Is it intermittent or
> does it always happen?  I tried following the steps from comment 13 using "+"
> as the search string (which does not appear in the document).  Then I hit Esc
> and then Ctrl-F again and try "api" but it finds it like it should.

It probably didn't help that I screwed up the revised repro steps on comment #15.  Let's try this again:

1. Launch Firefox and open 
    "https://adwords.google.com/api/adwords/v12/CampaignService?wsdl"
2. Type Ctrl/Cmd-F, then type "api".
3. Hit Escape to dismiss the find bar.
4. Hit F5/Cmd-R to reload the page (this is the step I forgot above, d'oh).
5. Type Ctrl/Cmd-F, then type "api".

In 3.5.1 step 2 is successful, but step 5 usually isn't.  It seemed like it would occasionally work however.

I did this test on OS-X, although my regression tests from earlier were actually run on Windows.  I don't think this is platform specific, however.
(In reply to comment #18)
> I did this test on OS-X, although my regression tests from earlier were
> actually run on Windows.  I don't think this is platform specific, however.

I can see this (your observed behavior) on Windows as well.  It is slightly less broken however as clearing out the find box with backspace and then searching works. (I.e. the bug only happens when you're typing over the previous search string that remains.)  This is also fixed if you insert a "focus the content area" step between steps 4 and 5.
Assignee: nobody → ddahl
I tried switching focus to the content window and/or setting the selection/caret point to no avail.

I spent a day on this and couldn't figure it out. Once I realized content iterators might be involved I had Jeff let out a roar of frustration and then I gave up.
Should I start in gdb with some breakpoints then? Did you try switching focus in findbar.xml? I'll see what I can find out.
Don't think it's findbar.xml related. The bug causes the find to iterate only over some empty space nodes, whereas after clicking, it iterates over the right text. I tried, by switching focus and adjusting the selection to be the same, but it didn't affect the outcome. I couldn't determine what was different in each case.
I have been plugging away trying to figure out *what* changes after you click on the XMLDocument. Some additional properties are added to the XMLDocument object:

Here is my console output:

dump(XMLDoc)

[object XPCNativeWrapper [object XMLDocument @ 0xaa5d5500 (native @ 0xaa372800)]]contentType

// enumeration of XMLDoc properties:

getElementsByTagName
title
characterSet
defaultView
location
styleSheets
--DOMWINDOW == 19 (0xaf2c2ff0) [serial = 14] [outer = 0xb6ae9580] [url = about:blank]
--DOMWINDOW == 18 (0xaf2c44f0) [serial = 18] [outer = 0xaf2c4300] [url = about:blank]

// i added a click handler to the window that enumerates the properties again:
*** CLICK ***

[object XPCNativeWrapper [object XMLDocument @ 0xaa5d5500 (native @ 0xaa372800)]]contentType
getElementsByTagName
title
characterSet
defaultView
location
styleSheets

// they have not changed... or have they?
// initiate Find again:

[object XPCNativeWrapper [object XMLDocument @ 0xaa5d5500 (native @ 0xaa372800)]]contentType
getElementsByTagName
title
characterSet
defaultView
location
styleSheets
parentNode
nodeType

// must have been some asynchronous call that adds 2 more properties to the XMLDoc

So, where is this functionality triggered from? Was this state be set for xml docs during a find in the past?
I can do a new regression range on the repro steps in comment #18, if that's of any help.
That would be awesome.
(In reply to comment #25)
> That would be awesome.

This bug has been present since at least July of 2008.  It looks like testing much further back would likely require messing with CVS, which I suspect is more trouble than it's worth.
(In reply to comment #26)
> (In reply to comment #25)
> > That would be awesome.
> 
> This bug has been present since at least July of 2008.  It looks like testing
> much further back would likely require messing with CVS, which I suspect is
> more trouble than it's worth.

ddahl:

I should probably clarify: messing with CVS is more trouble than it's worth for *me* (:-).  If you think a regression window would still be helpful, then Enn suggests: 

"re bug 517866, you can either add the regressionwindow-wanted keyword or ask mw22 or ria to find a regression window".
Is there any reason to believe that it is actually a regression?  It's an unusual enough usage pattern that it may have been broken forever.
(In reply to comment #28)
> Is there any reason to believe that it is actually a regression?  It's an
> unusual enough usage pattern that it may have been broken forever.

This is my suspicion at this point.
I vote for broken forever as well:)
I won't have time to dig through code till the weekend, but given the behavior I would suggest starting looking at what happens when the search box is emptied.  In the comment 18 bug, find works if the search box starts empty or is emptied and only doesn't work when the search box starts with text loaded in it.
(In reply to comment #31)
>  In the comment 18 bug, find works if the search box starts empty or
> is emptied and only doesn't work when the search box starts with text loaded in
> it.

After playing with this extensively, the search only works when you click on the contentDocument. Anything else you do (without clicking) does not matter, and the search fails.
(In reply to comment #32)
> (In reply to comment #31)
> >  In the comment 18 bug, find works if the search box starts empty or
> > is emptied and only doesn't work when the search box starts with text loaded in
> > it.
> 
> After playing with this extensively, the search only works when you click on
> the contentDocument. Anything else you do (without clicking) does not matter,
> and the search fails.

That's true for the filed bug.  For what Curtis found in comment 18 what I said applies.  Maybe we should file a separate bug just so its clear what we're talking about when we talk?
After talking to Jonas, and watching all of this behavior in gdb, I started setting breakpoints in nsXMLPrettyPrinter, nsFind, nsIFindContentIterator. It seems we should be using the ChildIterator in nsChildIterator instead of the nsIFindContentIterator to iterate the nodes in the displayed document. The ChildIterator will iterate the xbl anonymous nodes, where the content that is being searched resides. 

Anyway, I was attempting to just swap out the ChildIterator for the nsIFindContentIterator, and my make-fu is weak:

/home/ddahl/code/moz/mozilla-central/mozilla/embedding/components/find/src/nsFind.cpp:44:29: error: nsChildIterator.h: No such file or directory

nsChildIterator.h is in layout/base/

what to do?
Btw: I don't think that the ChildIterator has all of the methods that nsIFindContentIterator has, so there may be additional hacking here or QueryInterface calls to make.
This bug only affects PrettyPrinted XML docs, for instance: http://www.w3.org/Style/Examples/001/doc.xml is unaffected.

So it seems the regression is from this summer - this worked in 3.5.0. Hunting  the regression now.
I am pretty sure this is a FocusManager regression.

Find in PrettyPrinted docs works in this rev (June 9th):

http://hg.mozilla.org/mozilla-central/rev/39f1a74e500b

But fails in this rev (Jun 11):

http://hg.mozilla.org/mozilla-central/rev/4430cae50dad

I think the changes in nsTypeAheadFind are the culpript:

http://hg.mozilla.org/mozilla-central/diff/cabb8925dcd3/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp
(In reply to comment #37)
> I am pretty sure this is a FocusManager regression.
> 
> Find in PrettyPrinted docs works in this rev (June 9th):
> 
> http://hg.mozilla.org/mozilla-central/rev/39f1a74e500b
> 
> But fails in this rev (Jun 11):
> 
> http://hg.mozilla.org/mozilla-central/rev/4430cae50dad
> 
> I think the changes in nsTypeAheadFind are the culpript:
> 
> http://hg.mozilla.org/mozilla-central/diff/cabb8925dcd3/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp

What exactly is this regression range for?  It appears that you're finding the same range that I found in comment 7 ...
Yeah, it is. Sorry for the spam.
The bug here is caused because the unstyled xml view has the original xml nodes which are hidden, plus the transformed visible nodes which are included via an observer to an xml binding.

The find code is only looking at the original hidden nodes, which never match as they aren't visible. When clicking on the page though, the selection caret is updated to the node that was clicked, which naturally is one of the visible ones, so the find works again.

It stops working again if the selection is cleared again in some way.

The pseudo-fix here is to restore the code which checks for focused content and only clears the first visible flag when a focused element is set. It fixes the regression but it can stop working again if the selection is cleared (for example, by tabbing to the document or clicking after the end of the document) 

I can't seem to get a test working for this though.
Assignee: ddahl → enndeakin
Status: NEW → ASSIGNED
Attachment #407381 - Flags: review?(neil)
so - can't you get the visible node wrapper div element (in the observer) and pass it to the focus manager's setFocus() if it is an unstyled xml doc.

Sounds hacky, I know, and, Its probably not that simple.
That would work also, but only once. Once the selection is cleared (this happens when tabbing for example), it won't work again.

For whatever reason, the find code only checks focus on the first find. A 'find again' assumes the focus and selection are already ok.
Attachment #407381 - Flags: review?(neil) → review+
Checked in this fix: http://hg.mozilla.org/mozilla-central/rev/8e72d0e93cda
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
any regressions from this? if it looks clear, please land on branch asap.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: