After Find Again (Ctrl+G / F3), text highlighting color becomes green

RESOLVED FIXED in mozilla1.8.1

Status

()

Toolkit
Find Toolbar
P4
normal
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Caspy7, Assigned: Peter Kasting)

Tracking

({fixed1.8.1})

Trunk
mozilla1.8.1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
How to reproduce:
1. insure "find text as you type" is on
2. go to http://www.money-net.net/welcome.htm
   note this is a framed page, I don't believe this bug exists in non framed pages
3. type "a" (without quotes"
4. type <ctrl> + g to find the next occurance of "a"
5. click on the webpage to cancel the search.
6. click and drag to select a portion of text.

Expected behavior: text is highlighted with system defaulted color (blue for me)
Actual behavior: text is hightlighted bright green (current default color for
this feature).

This behavior can be undone by doing another find as you type search and hitting
escape to cancel.
This improper behavior does not occur when one hits escape to cancel the search
or attempts it on a non-framed webpage (in my experience)
(Reporter)

Comment 1

15 years ago
It appears the same problem with green highlighting can be observed in
non-framed pages by doing the following:

1. insure "find text as you type" is on
2. insure there are multiple tabs open.
3. type "a" (without quotes)
4. type <ctrl> + g to find the next occurance of "a"
5. click on a different tab
6. type <ctrl> + g to find an occurance of "a"
7. click on the original tab.
8. click and drag to select a portion of text.

I hope this helps to point towards the real problem.

Updated

14 years ago
Target Milestone: --- → mozilla1.9beta

Updated

14 years ago
Priority: -- → P4

Comment 2

13 years ago
I've found that all you need to do is escape out of the find bar once you've
located an instance of whatever you're searching for, hit CTRL+G to find another
one, then start highlighting. It doesn't seem to matter whether the page has
frames or not. 

Comment 3

13 years ago
Looks like a dupe of bug 193910

Comment 4

12 years ago
this does not only happen to framed page
Summary: After Find Again cancelled with a click in a framed page, text highlighting stays green → After Find Again (Ctrl+G / F3), text highlighting color becomes green

Comment 5

12 years ago
*** Bug 295489 has been marked as a duplicate of this bug. ***

Comment 6

12 years ago
*** Bug 269945 has been marked as a duplicate of this bug. ***

Comment 7

12 years ago
Firefox only. Doesn't happen in Mozilla/Seamonkey -> Firefox
Assignee: aaronleventhal → nobody
Component: Keyboard: Find as you Type → Find Toolbar / FastFind
Product: Core → Firefox
QA Contact: bugzilla → fast.find
Target Milestone: mozilla1.9beta → ---

Comment 8

12 years ago
*** Bug 267971 has been marked as a duplicate of this bug. ***

Comment 9

12 years ago
I cannot reproduce comment 0 or comment 1 using a current nightly. Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051019 Firefox/1.5

worksforme
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME

Comment 10

12 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051019
Firefox/1.6a1 ID:2005101911

I reproduced this with the following steps:

1. Make sure "begin finding when you begin typing" is on.
2. Type "a" (no quotes) and type <Ctrl>+g to find the next occurance of "a".
3. Switch to another tab and hit <Ctrl>+g in that tab. 

The find bar is not shown and anything you select in that tab will be green
thereafter.
Status: RESOLVED → REOPENED
Depends on: 193910
Resolution: WORKSFORME → ---

Comment 11

12 years ago
How to reproduce:

1. insure "begin finding when you begin typing" is on
2. type "a" (without quotes)
3. close find bar
4. press F3/Ctrl + G to find the next occurance of "a"
5. click on the webpage to cancel the search.
6. click and drag to select a portion of text.

Expected behavior: text is highlighted with system defaulted color (blue for me)

Actual behavior: text is hightlighted bright green (current default color for
this feature).
No longer depends on: 193910

Comment 12

12 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051020 Firefox/1.5

Sorry, :P

Updated

12 years ago
OS: Windows 98 → All
taking.
Assignee: nobody → masayuki
Status: REOPENED → NEW
Hardware: PC → All
Target Milestone: --- → Firefox1.6-
*** Bug 316013 has been marked as a duplicate of this bug. ***

Comment 15

12 years ago
*** Bug 326907 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

12 years ago
*** Bug 195623 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

12 years ago
Taking, I have a patch for this.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

12 years ago
OK, apparently "accept bug" doesn't also assign it to me...
Assignee: masayuki → pkasting
Status: ASSIGNED → NEW
(Assignee)

Comment 19

12 years ago
Created attachment 224644 [details] [diff] [review]
patch v1

(and of course, assigning the bug to myself meant it went back to NEW... whatever)

This fixes this bug and makes some handling of SELECTION_ATTENTION a bit better/more consistent:
* When using SELECTION_ATTENTION to select over an image, use the correct background color (the ATTENTION color, not the DISABLED color)
* Treat ATTENTION just like ON in the documentViewer blur handler (for whose purposes it is just like ON)
* Other miscellaneous code/comment cleanup

The actual bug fix could be considered a hack, but there doesn't seem to be a "better" way to do this.  The Find code is the only code that uses SELECTION_ATTENTION, and when the Find bar isn't onscreen, nothing about its code can turn the selection "back to normal" when the user starts goofing with the selection.  So, the fix here is to make the FrameSelection automatically reset itself to ON when it's cleared in ATTENTION mode.  If the Find bar wants to show another ATTENTION selection, it can (and does!) set the selection type back to ATTENTION.  Thus, ATTENTION is not a "permanent" state change, so much as a temporary state change "for this selection", which fits with what the Find code is trying to do.

I purposefully did not go farther in trying to reset to ON if, say, the selection changes at all (for example, if the user uses shift + arrows to extend the selection).  Not only would this be more invasive and unlikely to even be noticed by most users, but I actually consider being able to change the ATTENTION selection a "feature", in the sense that you can use it to quickly draw your eye to where on the page the selection is when you do a Find and can't locate the result.

I needed to make some changes in findBar.js to change the color after we find a new match rather than before, since the act of finding a new match will trip my new code :)

Since this patch sort of sprawls over a couple different locations, I wasn't sure who the right set of reviewers would be... please reassign as appropriate.

I'd like to get this fix into Fx2 if possible.
Attachment #224644 - Flags: superreview?(jst)
Attachment #224644 - Flags: review?(mconnor)
(Assignee)

Comment 20

12 years ago
Nominating for Fx2 B1.  Not only is this a mildly annoying bug, it may turn out to be a blocker for bug 189039 (find in textareas), since it seems more aggravating with that fixed.  While the change is a little creepy-looking, it only affects a selection type used by Find.
Flags: blocking-firefox2?
Comment on attachment 224644 [details] [diff] [review]
patch v1

r=me for the toolkit bits, jst is out for this week, maybe try bz instead?
Attachment #224644 - Flags: review?(mconnor) → review+
(Assignee)

Comment 22

12 years ago
Comment on attachment 224644 [details] [diff] [review]
patch v1

OK, switching sr? to bz.
Attachment #224644 - Flags: superreview?(jst) → superreview?(bzbarsky)
I'm actually out of town Thurs -> Monday, then again soon after... I might be able to get to this next week, but if not then it won't happen till sometime in mid July.
(Assignee)

Comment 24

12 years ago
Comment on attachment 224644 [details] [diff] [review]
patch v1

Hmmm... OK, how about dbaron then?  Since much of this patch is in layout/.
Attachment #224644 - Flags: superreview?(bzbarsky) → superreview?(dbaron)
nice to fix, but not a blocker
Flags: blocking-firefox2? → blocking-firefox2-
(Assignee)

Comment 26

12 years ago
Comment on attachment 224644 [details] [diff] [review]
patch v1

Preemptively adding branch approval request
Attachment #224644 - Flags: approval-branch-1.8.1?(dbaron)
Comment on attachment 224644 [details] [diff] [review]
patch v1

I'd love to see us figure out what the model really is here.  Clearly
find has to display one (or preferably more) things highlighted.  The
question is what influenced find has on the normal selection and whether
one of those highlighted things is the normal selection just displayed
differently.

And I'd also love to see the selection code provide a better API for
things like this.

One substantial comment in GetImage:
    container = mDisabledContainer;
does not do what you think.  You can't rebind a reference in C++, but
that's what you wanted to do here.  This is rebinding what the reference
refers to.  Although it's ugly, you need to make container a pointer to
an nsCOMPtr.  (Or you could start with a pointer and then assign *ptr
into a reference.)

Also, a few nits:

You can remove the
    if (!container)
         return NS_ERROR_OUT_OF_MEMORY;
since do_CreateInstance won't return null and success.  (That really
should be documented better in the code, including the interfaces all of
whose implementations have to follow the rules to make that true.  But
we really do rely on it, and most C++ objects have factories implemented
by the same macros that do do it right reliably.)
Likewise for the result of do_GetService and:
  if (NS_SUCCEEDED(result) && look)
although in that case you're probably better off with only the
null-check, since you're not propagating the error, and you can save
passing the &result to do_GetService.

You should also add a space after the comma in:
   do_CreateInstance("@mozilla.org/image/container;1",&result);

And in nsDisplaySelectionOverlay::Paint
   nsILookAndFeel::nsColorID lookandFeel;
This was already here, but could you rename the variable to |colorID|
instead of |lookandFeel| (the half-camel-case name for what usually
refers to a service.)  You already used |colorID| elsewhere, and I wrote
this comment before reading that part of the patch (because I had to use
the HTML view of the diff to read it).
Attachment #224644 - Flags: superreview?(dbaron)
Attachment #224644 - Flags: superreview+
Attachment #224644 - Flags: approval-branch-1.8.1?(dbaron)
Attachment #224644 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 28

12 years ago
Created attachment 225941 [details] [diff] [review]
sr comments addressed, updated to apply cleanly

Here's the final version to go on trunk + branch
Attachment #224644 - Attachment is obsolete: true

Comment 29

12 years ago
Patch didn't apply on the branch, checked in on the trunk:

layout/base/nsDocumentViewer.cpp 1.491
layout/generic/nsFrame.cpp 3.656
layout/generic/nsSelection.cpp 3.242
toolkit/components/typeaheadfind/content/findBar.js 1.40
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 30

12 years ago
My patch does not, in fact, fix this bug in all cases.  Specifically, I fix a case where, sayu, you find a match in an anchor and then highlight some text outside the anchor; but not the case where you find, and then highlight, something in the same block of (non-anchor) text.

I'm going to attach a branch version of the trunk patch above, but it shouldn't be applied.  Mark and I aren't going to back out the trunk patch for now since it's not wrong, just incomplete.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 31

12 years ago
Created attachment 225955 [details] [diff] [review]
branch version (v1)

This is a branch version of the trunk patch, with the same caveats about functionality.
(Assignee)

Comment 32

12 years ago
Created attachment 225957 [details] [diff] [review]
trunk tweak

This tweak should fix the problem completely on trunk.  Updated branch patch coming up in a second.
Attachment #225957 - Flags: review?
(Assignee)

Comment 33

12 years ago
Created attachment 225958 [details] [diff] [review]
branch version (final)

Here's the branch version of the entire patch, including the above tweak.
Attachment #225955 - Attachment is obsolete: true
Is it possible that this caused some new balsa trace_malloc_leaks.
(Assignee)

Comment 35

12 years ago
I sure don't think so... reviewing my code I can't see how it could leak where the old code didn't.

Comment 36

12 years ago
Checkins for the leakage increase:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006%3A06%3A16%3A17%3A37%3A00&maxdate=2006%3A06%3A16%3A18%3A48%3A00&cvsroot=%2Fcvsroot

It's leaking an nsRunnable, and nsSelection does have a class in it that implements (?) nsRunnable (please excuse my lack of C++ knowledge).
(Assignee)

Comment 37

12 years ago
I'm not creating any new nsSelections anywhere.

I suppose tomorrow I can try and see if I can use a local tinderbox to test backing out my patch to see if it changes leak numbers, but I really must have missed something if this patch can possibly leak.
(Assignee)

Comment 38

12 years ago
Comment on attachment 225957 [details] [diff] [review]
trunk tweak

Didn't get a random reviewer over the weekend, so I'm sticking this quick one in dbaron's queue.  (I'm assuming in a case like this all I really need is the r+ on the trunk tweak to get the tweaked versions landed on trunk + branch).
Attachment #225957 - Flags: review? → review?(dbaron)
Attachment #225957 - Flags: review?(dbaron) → review+

Comment 39

12 years ago
Tweak checked in on trunk:
layout/generic/nsSelection.cpp 3.243

Patch checked in on MOZILLA_1_8_BRANCH for 1.8.1b1:
layout/base/nsDocumentViewer.cpp 1.442.4.14
layout/generic/nsFrame.cpp 3.574.2.10
layout/generic/nsSelection.cpp 3.199.2.12
toolkit/components/typeaheadfind/content/findBar.js 1.21.2.17
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 40

12 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060620 BonEcho/2.0a3 ID:2006062004

landed on Bon Echo ?
sometimes green, sometimes blue.
screenshot : http://img223.imageshack.us/img223/373/fayt4oh.jpg

Comment 41

12 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060620 Minefield/3.0a1 ID:2006062004 [cairo]

same with Minefield.
screenshot : http://img210.imageshack.us/img210/7484/fayt16fi.jpg
(Assignee)

Comment 42

12 years ago
"sometimes green, sometimes blue" doesn't tell me much.  Can you give me some steps as to what you're doing?
(Assignee)

Comment 43

12 years ago
Ah, I know what he means.

The highlight is cleared to SELECTION_ON when we scroll the selection into view.  It doesn't break if you don't need to scroll.

Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 44

12 years ago
This seems to be broken because the selection controller that findBar.js sets SELECTION_ATTENTION on isn't the same as the selection controller the matched text lives on (!?).  Or at least that's what it looks like in gdb.  Not sure why that's the case.

Happily, this is fixed by my patch in bug 189039, because it moves selection controller-related stuff almost completely into the C++ side of things.  (The rest of the fix that was checked in for this bug is still helpful and necessary alongside that patch.)

For now, my plan is to sit tight and wait for that patch to make it in.  If it becomes clear that patch isn't going to make it, then I need to come up with a different fix.
(Assignee)

Updated

12 years ago
Depends on: 342741
(Assignee)

Comment 45

12 years ago
At Gavin Sharp's recommendation, I'm changing this back to FIXED (since it is, indeed, fixed) and letting bug 342741 track the regression.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 46

11 years ago
*** Bug 256801 has been marked as a duplicate of this bug. ***
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.