Closed Bug 273304 Opened 20 years ago Closed 16 years ago

Search results remain highlighted when turning off highlight again.

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: omeyer, Assigned: boissomag)

References

(
URL
)

Details

Attachments

(4 files, 2 obsolete files)

The test web page.
7.61 KB, application/x-zip-compressed
Details
Simplified testcase
174 bytes, text/html
Details
Patch3
3.34 KB, patch
Details | Diff | Splinter Review
Patch3 for trunk
3.58 KB, patch
masayuki
: review-
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20041122 Firefox/1.0 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.5) Gecko/20041122 Firefox/1.0 If you use the fast search bar on the given page to search for 'because m' (without the quotes) and then pressn highlight all occurances get highlighted. If you press highlight again. Three highlights remain visible. I expect them to return to normal. You can repeat this by pressing reload with turns of highlighting completely and pressing highlight twice. Reproducible: Always Steps to Reproduce: 0. Open the given page. 1. Ctrl+F 2. type: because m 3. press highlight 4. press highlight Actual Results: Three 'Because m' are highlighted Expected Results: ALL highlightes should be removed.
Attached file The test web page. —
As google search results will change I included the saved webpage I use to reproduce the bug.
This seems to correlate with part of the search result being bold. Searching for 'crying ' also produces this result. If you search for 'e but' the word 'but' becomes bold after pressing highlight. See also Bug #263849
Attached file Simplified testcase —
Apparently highlighting becomes confused when it has to go outside an element: highlighting "because" works, highlighting "because " (or anything beyond the space) fails.
Status: UNCONFIRMED → NEW
Ever confirmed: true
With Firefox 1.0 20050126 fr, the highlight function seems very bugged: When I click "highlight", only the actual found word (which is already highlighted in blue) becomes highlighted in green, and the highlight function stays activated even if I click again on it.
*** Bug 302933 has been marked as a duplicate of this bug. ***
I can recreate on Linux, so setting OS to All.
OS: Windows 2000 → All
Notice also that in the test case, the "m" become bold. If you replace "<b>because</b>" by "<b id="__firefox-findbar-search-id">because</b>" then the highlight disappear but the m stay bold. The highlighted doc is: <html><head><title>Bug 273304 testcase</title></head><body> <b><span id="__firefox-findbar-search-id" style="padding: 0pt; background-color: yellow; color: black; display: inline; font-size: inherit;"><b>because</b> m</span></b>aybe </body></html> So there is really something wrong in the way the span element is inserted. Why <b> is duplicated ? -- Firefox 1.5.0.1
Attached patch Patch1 (obsolete) — — Splinter Review
The patch should take care of the problem that arise when extractContents duplicate a node. The span element was always inserted in the start container, I added two cases where it can be inserted in the end container or between both. Ex: <b>BOLD</b>mid<b>BOLD</b> 1) searching for "idBO" extractContents = id<b>BO</b> --> insert in startContainer (which is "mid") 2) searching for "LDmi" extractContents = <b>LD</b>mi --> insert in endContainer (which is "mid") 3) searching for LDmidBO extractContents = <b>LD</b>mid<b>BO</b> --> insert beetwen start and end
I'd say that this bug depends on bug 332148.
Comment on attachment 217017 [details] [diff] [review] Patch1 > + var bFound = false; We don't use Hungarian Notation. You should use just 'found'. > + if (fchild.nodeType == 1 && lchild.nodeType == 1) { > + var before = endContainer.parentNode; > + } else { > + if (fchild.nodeType == 1) { > + container = endContainer; > + offset = 0; > + } else { > + container = startContainer; > + offset = startOffset; > + } > + > + var before = container.splitText(offset); > + } Why there are |before| variables? I think that the |before| variable should be on outer block.
Assignee: firefox → nobody
QA Contact: fast.find
Attached patch Patch2 (obsolete) — — Splinter Review
- Corrected "before" mistake and "bFound" - There was still some cases where the highlight was not removed correctly
Attachment #217017 - Attachment is obsolete: true
Comment on attachment 217310 [details] [diff] [review] Patch2 I haven't fully understood what exactly this patch does (I'll try to do that later), but I have some general comments about the code added. >Index: findBar.js > while ((retRange = finder.Find(gLastHighlightString, searchRange,startPt, endPt))) { > var startContainer = retRange.startContainer; > var elem = null; >+ var found = false; > try { > elem = startContainer.parentNode; > } > catch (e) { } > > if (elem && elem.getAttribute("id") == "__firefox-findbar-search-id") { >+ found = true; >+ } >+ >+ if (!found) { >+ try { >+ elem = endContainer.parentNode; >+ } >+ catch (e) { } >+ >+ if (elem && elem.getAttribute("id") == "__firefox-findbar-search-id") { >+ found = true; >+ } >+ } >+ >+ if (!found) { >+ try { >+ elem = startContainer.parentNode; >+ while (elem=elem.parentNode) { >+ if (elem.getAttribute("id") == "__firefox-findbar-search-id") { >+ found = true; >+ break; >+ } >+ } >+ } >+ catch (e) { } >+ } >+ If I understand this code correctly, I think it can be changed to the following: function checkNode(aNode) { if (aNode && aNode.getAttribute("id") == "__firefox-findbar-search-id") return aNode; return null; } var elem = checkNode(elem) || checkNode(endContainer.parentNode); if (!elem) { elem = startContainer.parentNode; while (elem && !(elem = checkNode(elem))) elem = elem.parentNode; } >+ if (found) { Then this becomes |if (elem) {| > function highlight(range, node) > { >+ var container, offset, before; > var startContainer = range.startContainer; > var startOffset = range.startOffset; >+ var endContainer = range.endContainer; > var endOffset = range.endOffset; > var docfrag = range.extractContents(); >- var before = startContainer.splitText(startOffset); >+ var fchild = docfrag.firstChild; >+ var lchild = docfrag.lastChild; >+ >+ if (fchild.nodeType == 1 && lchild.nodeType == 1) { >+ before = endContainer.parentNode; >+ } else { >+ if (fchild.nodeType == 1) { >+ container = endContainer; >+ offset = 0; >+ } else { >+ container = startContainer; >+ offset = startOffset; >+ } Indentation is weird above. No need for brackets around single-line statements.
Well, a little explanation using exemple from Comment #9 After highlight we get for each case: 1) <b>BOLD</b>m <span>id<b>BO</b></span> <b>LD</b> the span element is the parent of the start container 2) In the same way, the span element is the parent of the end container 3) <b>BO</b> <span><b>LD</b>mid<b>BO</b></span> <b>LD</b> The span element is the parent of both parent's containers in that case. But we don't know how much nested elements can be inside the span element. So we have to loop to find it. > var elem = checkNode(elem) || checkNode(endContainer.parentNode); > if (!elem) { > elem = startContainer.parentNode; > while (elem && !(elem = checkNode(elem))) > elem = elem.parentNode; > } Ah! It looks better but it's not completely correct. The following works: try { var elem = checkNode(startContainer.parentNode); if (!elem) { elem = endContainer.parentNode; while (!checkNode(elem)) elem = elem.parentNode; } } catch (e) { elem = null; }
(In reply to comment #14) > > var elem = checkNode(elem) || checkNode(endContainer.parentNode); > > if (!elem) { > > elem = startContainer.parentNode; > > while (elem && !(elem = checkNode(elem))) > > elem = elem.parentNode; > > } > > Ah! It looks better but it's not completely correct. The following works: > > try { > var elem = checkNode(startContainer.parentNode); > if (!elem) { > elem = endContainer.parentNode; > while (!checkNode(elem)) > elem = elem.parentNode; > } > } > catch (e) { elem = null; } Why is the try{} block needed (what throws)? Won't !checkNode(elem) infinitely recurse if there are no matches? That code doesn't seem equivalent to that of the patch, either: after looping, the final elem would be the parent of the one that passed checkNode.
> Why is the try{} block needed (what throws)? Won't !checkNode(elem) infinitely > recurse if there are no matches? The try{} stop the loop when there is no more parent, so there is no need to test for elem. And without the try{}, only the first match is processed. > That code doesn't seem equivalent to that of > the patch, either: after looping, the final elem would > be the parent of the one that passed checkNode. I don't think so.
elem.parentNode returns null if the element has no parent. checkNode(null) returns null, which won't stop the loop. Relying on property accesses on a null object and try/catch blocks for flow control should be avoided, since it can mask other exceptions and is hard to follow.
Well, Ok. But the try catch is needed nevertheless.
(In reply to comment #18) > Well, Ok. But the try catch is needed nevertheless. Why?
I've already answered, even if I don't know the cause. It's on the original script too.
Yes, I know it's in the current code, but we should not assume that it is required just because it is pre-existing. If there is no apparent reason for it to be there, please remove it.
Since without it the script doesn't work, it will stay here unless someone has a better solution.
Yes, but *why* won't the script work? You say you don't know why it's needed, then you say that without it it won't work, so which one is it? :) In other words, what exception is thrown and then caught?
(In reply to comment #16) > test for elem. And without the try{}, only the first match is processed. ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ That's all I can say currently.
I finally took some time to look at it. There is an exception because elem may not have a getAttribute method. It can be fixed by replacing startPt.setStartAfter(elem); by startPt.setStartAfter(next); when setting the range for the new iteration. I'm not sure if it's completely safe though.
Attached patch Patch3 — — Splinter Review
Updated using Gavin Sharp idea.
Attachment #217310 - Attachment is obsolete: true
Attached patch Patch3 for trunk — — Splinter Review
This is patch3 updated to work with the trunk (previous was for 1.8 branch)
Assignee: nobody → boissomag
Status: NEW → ASSIGNED
Attachment #225281 - Flags: review?
I don't see this any more using Firefox 2.0 beta 2 on Windows XP.
Doh, I don't see it any more on that URL (which has probably since changed), but I DO still see it on the 'simplified testcase'. Sorry, I should have checked that out too first.
Comment on attachment 225281 [details] [diff] [review] Patch3 for trunk Er... seems like email was not accepted here too. Explanations of the patch are in Comment #9 Comment #14 and Comment #25
Attachment #225281 - Flags: review? → review?(masayuki)
Comment on attachment 225281 [details] [diff] [review] Patch3 for trunk > + if ((elem = startContainer.parentNode) && > + elem.getAttribute("id") != "__firefox-findbar-search-id") I don't like this format. I misread the condition in first time. You should use another format like endContainer case. I.e., var elem = startContainer.parentNode; if (elem && elem.getAttribute("id") != "__firefox-findbar-search-id") > - this.mStartPt.setStartAfter(elem); > + this.mStartPt.setStartAfter(next); I don't think that this change is right... I think that you need to check whether the node is element node before calling getAttribute, right? > + var fchild = docfrag.firstChild; > + var lchild = docfrag.lastChild; I don't like these names. Please use readable name like others. I.e., fchild -> firstChild, lchild -> lastChild. > + if (fchild.nodeType == 1 && lchild.nodeType == 1) Don't use '1', you should use a constant like here. http://lxr.mozilla.org/mozilla/source/toolkit/components/typeaheadfind/content/findBar.js#605
Attachment #225281 - Flags: review?(masayuki) → review-
(In reply to comment #31) > > - this.mStartPt.setStartAfter(elem); > > + this.mStartPt.setStartAfter(next); > > I don't think that this change is right... In fact, with setStartAfter(elem), it can stop twice on the same result because the new start point will end up before the reinserted search string. > I think that you need to check whether the node is element > node before calling getAttribute, right? Yes, but this would only hide the problem.
There's rumors on the Internets that pkasting has a patch for this.
(In reply to comment #33) > There's rumors on the Internets that pkasting has a patch for this. I do not have a patch for this. However, this bug will be auto-fixed by bug 263683 if I ever fix that.
This bug came up while I was doing litmus testing on Minefield. As described above the highlighting does not remove. Steps to reproduce: 1. Go to www.mozilla.org 2. Ctrl+f, type "firefox" in find box, click highlight all 3. Replace "firefox" with "mozilla" in find box. 4. Unclick highlight all Both "mozilla" and "firefox" remain highlighted. For verification, I tested the same on FF2.0.0.3. This problem appears to be isolated to Minefield.
Product: Firefox → Toolkit
Bug 263683 is now fixed on trunk, so this should be fixed too.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Depends on: 263683
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: