313653 - Find bar gives incorrect results with tabs--"Highlight all" does not update

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[VanillaMozilla]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051024 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051024 Firefox/1.6a1

The Find bar background color is supposed to be red if the string is not found and white if it is found.  The toolbar also displays a "Phrase not found" notice if the string is not found.  When switching between tabs, pressing the "Highlight all" button will cause highlighting to be updated, but not the background color or "Phrase not found" notice.  This gives the user false information about whether the search string is found.

Reproducible: Always

Steps to Reproduce:
Note:  steps to reproduce will work with virtually any Web site, not just the examples.
1."Begin finding when you begin typing" should not be checked.
2.In a new browser window open http://www.mozilla.org/projects/deerpark/alpha2.html .  In a new tab open http://www.mozilla.org/support/ and switch to that tab.
3.Press <Ctrl>F to open the Find box.
4.Search for a string that is on the first tab but not on this tab.  Example: "broken" (without quote marks).
5. Switch back to the first tab.  Press "Highlight all".
6. In the first tab search for a string that is present on this tab but not on the second.  Example:  "Reporter tool" (without the quotes).
7. Switch to the second tab and press "Highlight all".
Actual Results:  
Background is still red after step 5 and white after step 7.  "Phrase not found" is still displayed after step 5.

Expected Results:  
Red background and "Phrase not found" should be updated after pressing "Highlight all".  Background should be white after step 5 and red after step 7.  "Phrase not found" should not be displayed after step 5.

Also verified for Linux and Windows 98SE.

Comment 1

[VanillaMozilla]

Bug 313657 exacerbates this bug.  After pressing the "Highlight all" button the window does not scroll to the highlighted search string.  Consequently, the user has no way of knowing the string was found.

Comment 2

[VanillaMozilla]

Also confirmed for 1.5RC1.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

Comment 3

[Jason Barnabe (np)]

Confirming with Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9a1) Gecko/20051128 Firefox/1.6a1. Patch coming up.

Comment 4

[Jason Barnabe (np)]

Attachment: patch

Essentially, when we press highlight we have to update the find status and not assume it's right already.

I also took the liberty of replacing Components.interfaces.nsITypeAheadFind throughout the file with an object-level variable.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 204409 [details] [diff] [review]
patch

>Index: toolkit/components/typeaheadfind/content/findBar.js

>+  mnsITypeAheadFind: Components.interfaces.nsITypeAheadFind,

This is only used for the defines (and "mnsI" looks weird), so might as well just use global consts like at http://lxr.mozilla.org/seamonkey/source/toolkit/components/typeaheadfind/content/findBar.js#41 . Might want to take the opportunity to prefix those differently, though I suppose FIND_ is probably unique enough (the comment mentions not setting any global vars right under the definition of global vars :).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Though on second thought, it's probably better to move the existing global constants to the object (without changing the names, the m prefix is unnecessary) and fix the browser.js users.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 204409 [details] [diff] [review]
patch

ok, that'll work, but let's not do the const bits in this patch.  Its unnecessary refactoring, and mnsI looks just plain odd.  If you want to do this.kTypeAheadFind ok, but let's refactor all of the consts at once, in the scope of another bug.

Comment 8

[Mike Connor [:mconnor]]

(as a note, k = const, m = member var, g = global var, which is why kTypeAheadFind is correct)

Comment 9

[Jason Barnabe (np)]

Attachment: patch updated to comments

Comment 10

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/typeaheadfind/content/findBar.js; new revision: 1.33;

Comment 11

[Peter van der Woude [:Peter6]]

Can we get this one one branch  (if it's safe enough) ?

Comment 12

[Peter van der Woude [:Peter6]]

this caused bug 321481

Comment 13

[:Gavin Sharp [email: [e-mail]]]

If this is taken on the branch for 1.8.1, it needs the patch from bug 313653 as well. Might be a good idea to let it bake a while, then post a rollup patch and ask for a?1.8.1.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

It also kind-of depends on bug 313149 landing there - but 1.8 and trunk browser/toolkit might be fully synced for at some point soon, so it might not matter.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Fixed on the 1.8 branch by bug 313149.