Closed Bug 499115 Opened 16 years ago Closed 15 years ago

nsFind fails to find more than 1 match in anonymous content if parent form control is last node in search range

Categories

(Core :: Find Backend, defect)

Product:
Core
Component:
Find Backend
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: graememcc, Assigned: neil)

References

Details

Attachments

(4 files, 2 obsolete files)

New testcase (uses enhanced privileges)
1.73 KB, text/html
Details
Proposed patch
5.79 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Restored null-check
5.79 KB, patch
Details | Diff | Splinter Review
Mochitest to automate the testcase
4.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached file Testcase (uses enhanced privileges) (obsolete) —
See attached testcase (same results when substituting the <input> with a <textarea>. (However, in the textarea case, the problem is only exhibited with the initial value given in the value attribute - <textarea>mozilla mozilla</textarea> finds the correct number of matches.) Uncommenting the #define DEBUG_FIND in embedding/components/nsFind.cpp, the second time round, it doesn't seem to be recognising that it's starting in the anonymous content of a form control.
Forgot to add - same in 3.0, so not a recent regression, if it is even a regression - might never have worked at all)
Revised testcase. You know what the data URI thing has to do with this? Not a jot. What is crucial is that the input/textarea should be the very last node in the search range.
Attachment #383902 - Attachment is obsolete: true
Summary: nsFind fails to find second match in form control when page source is data URI → nsFind fails to find more than 1 match in anonymous content if parent form control is last node in search range
At first I thought something is likely going awry with nsFind's outer/inner iterators, but now I'm beginning to wonder if this is in fact a bug in nsContentIterator? I've been doing some playing around, using another test file, which has two inputs both containing more than 1 instance of the search term, to see why resuming with a start point in the middle of the first input's anonymous text node works, but the second input's doesn't work. The only difference seems to be IsDone() reports true in the second case. To try and make it clearer with some ASCII art <BODY> <INPUT> <INPUT> (anonymous text nodes) match1 match2 match3 match4 <--------R1---------------> <---R2--> So, having found match1 in the first input, calling nsFind::Find again with the start point immediately after the first match, and end point at the end of the content, causes nsFindContentIterator to create a nsPreContentIterator with range R1. This correctly reports false to an IsDone() call. However, having found match3 in the second input, following the same process, nsFindContentIterator creates an nsPreContentIterator with range R2. This immediatly reports true for IsDone() despite the fact there's still some content not yet iterated over. bz: any thoughts?
Not offhand; I'm not really all that familiar with this code... Neil, do you know anything about it?
No, but I agree that the content iterator is getting confused. What happens is that it's creating the range from the native anonymous text content of the <input> element to the end of the <body>. Now the content iterator tries to calculate the last node to be iterated. Unfortunately since it doesn't take anonymous text into account it considers this to be the <input> element itself which, being before the start of the range, is not in the range. It therefore decides that the range is empty. Note that making nsFind ignore the bogus outer iterator still fails, because it tries to set up the inner iterator on a text node instead of a text control.
So this is a regression from bug 328412, which landed on 1.9.1 :-(
Blocks: 328412
Flags: wanted1.9.1.x?
Attached patch Proposed patch — — Splinter Review
So, the correct fix for bug 328412 is to protect the outer iterator at each call (there are four calls to consider). However as I mentioned that isn't actually quite enough to fix this bug; the inner iterator doesn't quite get set up correctly because we pass in the wrong element. I had to change the variables from nsIDOMNode to nsIContent so that we could pass the correct ones in. Some of the find code looks like it assumes that the logical end point of the search is never in anonymous content but I decided not to optimise for that.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #384592 - Flags: superreview?(jst)
Attachment #384592 - Flags: review?(bzbarsky)
So why is it ok to ditch the null-checks on startContent and endContent? Also, after this patch outerNode ends up native anonymous whereas before it used to be the input node, right? Is that correct?
(In reply to comment #8) > So why is it ok to ditch the null-checks on startContent and endContent? I thought they were real content because they come out of a range, but I can easily put the checks back in. > Also, after this patch outerNode ends up native anonymous whereas before it > used to be the input node, right? Is that correct? The other way around, I hope! We used to pass in the anonymous text node.
Although we never reached that point because we failed to find the editor.
> I thought they were real content because they come out of a range But they're the range start and end parents, no? If so, they could be Document nodes, say. > The other way around, I hope! We used to pass in the anonymous text node. Ah, I see. The key is changing what goes into the inner iter init. OK.
Comment on attachment 384592 [details] [diff] [review] Proposed patch If you put those null-checks back or prove they're not needed, looks good.
Attachment #384592 - Flags: review?(bzbarsky) → review+
Attachment #384592 - Flags: superreview?(jst) → superreview+
Actually I'd removed the null-checks in an intermediate version that tried to optimise the anonymous node detection and there was no reason not to keep them.
Attachment #384623 - Attachment is patch: true
Attachment #384623 - Attachment mime type: application/octet-stream → text/plain
Pushed changeset 4244c0215308 to mozilla-central.
Flags: in-testsuite?
Attachment #385099 - Flags: review?(bzbarsky)
Comment on attachment 385099 [details] [diff] [review] Mochitest to automate the testcase Some thoughts: >+ var noMatches = 0; Confusing name? >+ var doc = document; Not sure how useful this is... >+ this._searchRange.setStart(body, 0); >+ this._searchRange.setEnd(body, count); selectNodeContents? >+ this._startPt.setStart(body, 0); >+ this._startPt.setEnd(body, 0); >+ this._endPt.setStart(body, count); >+ this._endPt.setEnd(body, count); cloneRange/collapse? >+ this._startPt = retRange.endContainer.ownerDocument.createRange(); >+ this._startPt.setStart(retRange.endContainer, retRange.endOffset); >+ this._startPt.setEnd(retRange.endContainer, retRange.endOffset); cloneRange/collapse?
Er, quite, guilty as charged. Note to self: blind faith in the quality of your earlier quickly thrown together testcase code generally not a good idea.
Attachment #385116 - Flags: review?(neil)
Attachment #385099 - Attachment is obsolete: true
Attachment #385099 - Flags: review?(bzbarsky)
Attachment #385116 - Flags: review?(neil) → review?(bzbarsky)
Comment on attachment 385116 [details] [diff] [review] Mochitest to automate the testcase Looks OK to me but I spotted a couple more nits: >+ .createInstance() >+ .QueryInterface(Components.interfaces.nsIFind); This can be abbreviated to .createInstance(Components.interfaces.nsIFind); >+<input type="text" value="minefield minefield"></input></body> Unlike <textarea>, <input> doesn't have a closing tag.
Comment on attachment 385116 [details] [diff] [review] Mochitest to automate the testcase r+ with Neil's nits. Do you need me to push this?
Attachment #385116 - Flags: review?(bzbarsky) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/a55b20529273
Flags: in-testsuite? → in-testsuite+
Should this be marked fixed now?
Sorry, I wasn't sure it was safe to mark fixed when a test was requested, and I overlooked it when the test was checked in.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 839944
No longer depends on: 839944
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: