Closed Bug 253793 Opened 16 years ago Closed 16 years ago

Find in page won't stop if you go to the next page

Categories

(Toolkit :: Find Toolbar, defect, P3, major)

1.7 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla1.7.5

People

(Reporter: Peter6, Assigned: bugzilla)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 2 obsolete files)

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040730
Firefox/0.9.1+

1. Open a page
2. press ctrl+F and type the letter "a" in the findbar
3. press highlight
4. close the findbar
5. goto bookmarks and open another random page
6 .note that all "a"'s are highlighted even though you closed the findbar

to prevent this to happen you must clear the findbar before you "close" it

expected:
to stop searching/highlighting if the findbar is closed.
Attached patch patch to fix (obsolete) — Splinter Review
This patch resets the highlight find setting to OFF when opening a new URL in
the current window by using a bookmark.
Comment on attachment 154953 [details] [diff] [review]
patch to fix

request review
Attachment #154953 - Flags: review?(firefox)
requesting blocking-aviary1.0PR since patch exists
Flags: blocking-aviary1.0PR?
Reply to Comment #1: Closing the findbar should be considered as the end of
interest in the results of the search. Consequently, resetting the highlight
find setting when changing URL is not logical: a user may well be searching for
something over several URLs. On the other hand, text which was highlighted
should be un-highlighted when the findbar is closed.

Whiteboard: [have patch]
(In reply to comment #4)
> Consequently, resetting the highlight
> find setting when changing URL is not logical: a user may well be searching for
> something over several URLs. On the other hand, text which was highlighted
> should be un-highlighted when the findbar is closed.
> 

The patch only turns off the highlight when opening a new url through a
bookmark.  If you click a url on the page, the highlight stays on.  For example,
if using a search engine the highlight will remain as you click through pages. 
If you use a bookmark to go to a new URL, the highlight will turn off but the
find value will remain.  If you want the highlight on the new URL, all you need
to do is re-click highlight.

I think for most people when they use a bookmark to change pages, instead of
surfing through the page, they are finished with the original reason for the
highlight request.

(In reply to Comment #5)

What bothered me was that found (and highlited) items remained highlighted even
after I closed the Findbar.

I agree with the strategy regarding changing URLs.
one liner.

blake, can you review?
Whiteboard: [have patch] → [have patch] need review blake, ben
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0PR-
Flags: blocking-aviary1.0+
Comment on attachment 154953 [details] [diff] [review]
patch to fix

1. Please use -p when making patches. 
2. This would be best (and more reliably) done in onLocationChange in the
browser's status handler
3. Use .checked = false, not removeAttribute("checked").
Attachment #154953 - Flags: review?(firefox) → review-
(In reply to comment #8)
> 2. This would be best (and more reliably) done in onLocationChange in the
> browser's status handler

Is there a place in onLocationChange that is active only when using a bookmark
to change the location, or is this active on any change (including "clicking"
through a page URL)?

The reason I chose the location in the patch was I followed the code to this
location from following the bookmark code.

I really only want the highlight to reset if a bookmark is used, not if a url is
clicked in a page.  See comment #5 for more explanation. 

I might agree with you except for the bugs with highlight that are unlikely to
be fixed prior to 1.0 - best to have it disable fully for now when the page
changes. 
Whiteboard: [have patch] need review blake, ben → [have patch] needs rework
Attached patch patch with changes ben requested (obsolete) — Splinter Review
This patch turns off the highlight in onLocationChange as Ben requested.  

I tried ".checked = false" instead of ".removeAttribute("checked")", but it did
not work.
Attachment #154953 - Attachment is obsolete: true
Attached patch patchSplinter Review
Same as the last patch, just removes errant blank line
Attachment #161432 - Attachment is obsolete: true
Attachment #161612 - Flags: review?(bugs)
Priority: -- → P3
Whiteboard: [have patch] needs rework → [have patch] - need review ben
Whiteboard: [have patch] - need review ben → [have patch] - ready to land
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Branch checkin 2004-10-13 15:24.
Keywords: fixed-aviary1.0
Whiteboard: [have patch] - ready to land
Target Milestone: --- → Firefox1.0
(In reply to comment #10)
> I might agree with you except for the bugs with highlight that are unlikely to
> be fixed prior to 1.0 - best to have it disable fully for now when the page
> changes. 

and then go to all the hassle of someone submitting another bug somewhere down
the line for this to behave as it should, meanwhile everyone has the hassle of
trying to re-search every page they want to highlight, when it could be fixed
here and now and not need touching again.
vrfy'd fixed with 200410150x-0.9+ bits on linux fc2 and mac os x 10.3.5.
Status: RESOLVED → VERIFIED
OS: Windows 2000 → All
Hardware: PC → All
Product: Firefox → Toolkit
Depends on: 1316513
You need to log in before you can comment on or make changes to this bug.