Last Comment Bug 403090 - [FIX]Found text is no longer highlighted within INPUT text fields and TEXTAREAs
: [FIX]Found text is no longer highlighted within INPUT text fields and TEXTAREAs
Status: RESOLVED FIXED
: regression, verified1.8.1.12
Product: SeaMonkey
Classification: Client Software
Component: Search (show other bugs)
: 1.8 Branch
: All All
: -- minor (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 407432 (view as bug list)
Depends on:
Blocks: 393770
  Show dependency treegraph
 
Reported: 2007-11-08 13:56 PST by Darren
Modified: 2008-07-31 04:30 PDT (History)
12 users (show)
dveditz: blocking1.8.1.12-
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
<label>-less testcase (129 bytes, text/html)
2007-11-09 12:06 PST, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details
Fix (2.06 KB, patch)
2007-11-10 01:09 PST, Boris Zbarsky [:bz]
mats: review+
mats: superreview+
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review
Trunk patch as described (3.04 KB, patch)
2007-11-10 01:15 PST, Boris Zbarsky [:bz]
mats: review+
mats: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Darren 2007-11-08 13:56:40 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.10pre) Gecko/20071108 Camino/1.5.4pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.10pre) Gecko/20071108 Camino/1.5.4pre

When searching for a string within a page that has form fields or textarea fields with text within the fields, Camino no longer highlights the found text string. Cursor appears to move, but visually, there's very little indication that the string has been found within the field.

Reproducible: Always

Steps to Reproduce:
1. Visit a page with a TEXTAREA or INPUT field with some text prefilled.
2. Search for a known string within the TEXTAREA or INPUT field.
3. Camino will fail to hilite the found text.
Actual Results:  
TEXTAREA scrolls and cursor is moved, but text string is not color highlighted

Expected Results:  
Text should be highlighted.

This feature was previously working around version 1.5.1.
Comment 1 Chris Lawson (gone) 2007-11-08 20:42:37 PST
This broke somewhere between 20071003 and 20071004, probably due to bug 388784.
Comment 2 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-08 20:55:31 PST
Confirming.

This broke between 2007-10-03 00:00 and 2007-10-04 01:00 Camino branch builds.

Checkins: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-03+00%3A00&maxdate=2007-10-04+02%3A00&cvsroot=%2Fcvsroot

Lots of fun-looking bugs in there; bug 388784 and bug 393770 seem vaguely possible. If I knew anything about code, I'd lean toward Smaug's bug over Mats's bug.

I tried to see if this was broken in SeaMonkey 1.1.x also (since we're two apps that don't use Firefox TAF), but I can't get any SeaMonkey branch builds to run properly on 10.3.9 or 10.5 :/
Comment 3 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-08 21:35:27 PST
(In reply to comment #2)
> I tried to see if this was broken in SeaMonkey 1.1.x also (since we're two apps
> that don't use Firefox TAF), but I can't get any SeaMonkey branch builds to run
> properly on 10.3.9 or 10.5 :/

Nevermind; apparently SM on the branch can't run correctly from the .dmg.  SeaMonkey's Find (Cmd-F) shows this lack-of-highlight bug as well.
Comment 4 Samuel Sidler (old account; do not CC) 2007-11-08 21:54:54 PST
This happens in SeaMonkey 1.1.6 on Windows as well.
Comment 5 Boris Zbarsky [:bz] 2007-11-08 22:22:52 PST
So this is not a problem in Firefox on branch?
Comment 6 Samuel Sidler (old account; do not CC) 2007-11-08 22:40:26 PST
No, it's only a problem for uses of the XPFE find-as-you-type implementation, I believe. I can't reproduce it in Firefox 2.0.0.9.
Comment 7 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-08 23:10:36 PST
s/find-as-you-type/standard non-find-as-you-type find/

(On the branch, XPFE FAYT places the cursor but doesn't highlight; that's another bug that was filed long ago, stolen pre-Firefox 2 to use as a place to stash the Firefox fix for FAYT highlight, and closed, but it's not this bug.  This bug is about the standard Find impl that's not based on FAYT/TAF code.)
Comment 8 Boris Zbarsky [:bz] 2007-11-08 23:13:39 PST
Hmm.  Anyone happen to know where in the find code the highlighting's supposed to be happening?
Comment 9 Olli Pettay [:smaug] 2007-11-09 00:34:56 PST
Does this happen with pages which don't have <label> connected to <input> or <textarea>? Or does XPFE n-FAYT-f use <label> for something?
Bug 388784 is only about focusing <label>.
Comment 10 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-09 12:06:46 PST
Created attachment 288033 [details]
<label>-less testcase

I can reproduce the failure using this very simple testcase :(
Comment 11 Boris Zbarsky [:bz] 2007-11-10 01:03:19 PST
This is absolutely a regression from bug 393770.  That patch changed what IsNativeAnonymous() returns for the textnodes inside inputs, and the find code depended on the old value.
Comment 12 Boris Zbarsky [:bz] 2007-11-10 01:09:27 PST
Created attachment 288113 [details] [diff] [review]
Fix

This seems to fix the problem for me.  On trunk, it looks like the use of nsIFrame::GetSelectionController is what saves the day... In fact, on trunk this code should just not walk out of anonymous content perhaps.  If we ever DO return true for IsNativeAnonymous on those textnodes on trunk, it will break.
Comment 13 Boris Zbarsky [:bz] 2007-11-10 01:15:00 PST
Created attachment 288114 [details] [diff] [review]
Trunk patch as described
Comment 14 Daniel Veditz [:dveditz] 2007-11-13 11:14:09 PST
1.8.1.10 is being rushed, this will have to catch the next train after some trunk baking of the patch
Comment 15 Boris Zbarsky [:bz] 2007-11-13 11:59:12 PST
Daniel, there isn't going to be any trunk baking.  This is a branch-only regression.  The trunk patch just adjusts the code to be a little less regression-prone in the future, but doesn't change any behavior.

It might still be good to get some branch baking, of course, so I agree that we'll have to make do with 1.8.1.11.
Comment 16 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-11-13 16:21:38 PST
bz's patch does fix the issue for us in Camino (in case anyone was waiting for that info).
Comment 17 Mats Palmgren (vacation) 2007-11-18 08:32:54 PST
Comment on attachment 288113 [details] [diff] [review]
Fix

>-    if (!content->IsNativeAnonymous()) {
>+    if (!IsNativeAnonymous(content)) {

Maybe we should us a different name to reduce the risk of mistaking
one for the other? (IsNativeAnonymousDescendent(content) perhaps?)

Looks fine otherwise.  r+sr=mats
Comment 18 Mats Palmgren (vacation) 2007-11-18 08:33:51 PST
Comment on attachment 288114 [details] [diff] [review]
Trunk patch as described

ditto
Comment 19 Boris Zbarsky [:bz] 2007-11-18 10:23:17 PST
Comment on attachment 288113 [details] [diff] [review]
Fix

Requesting branch approvals.  This is quite safe, but I would probably put it in .11 if .10 is anywhere close to being done.
Comment 20 Boris Zbarsky [:bz] 2007-11-18 10:24:41 PST
Comment on attachment 288114 [details] [diff] [review]
Trunk patch as described

Requesting trunk approval.  This is bulletproofing/cleanup that should prevent this bug regressing on trunk if our anonymous content behavior is tweaked slightly.
Comment 21 Boris Zbarsky [:bz] 2007-11-18 10:52:48 PST
Checked in trunk patch, with s/IsNativeAnonymous/IsInNativeAnonymousSubtree/g.  Leaving bug open, since this is filed as a 1.8 branch bug.

It would be nice to have tests here; I just don't know our Find APIs well enough to tell which ones to use for said test.  Can someone help?
Comment 22 Smokey Ardisson (offline for a while; not following bugs - do not email) 2007-12-07 15:38:48 PST
*** Bug 407432 has been marked as a duplicate of this bug. ***
Comment 23 Daniel Veditz [:dveditz] 2007-12-17 16:15:11 PST
Not blocking because it won't stop us shipping Firefox, but will consider the patch for approval
Comment 24 Daniel Veditz [:dveditz] 2007-12-17 16:16:22 PST
Comment on attachment 288113 [details] [diff] [review]
Fix

approved for 1.8.1.12, a=dveditz for release-drivers
Comment 25 Boris Zbarsky [:bz] 2007-12-17 16:23:53 PST
Fixed on branch.
Comment 26 Al Billings [:abillings] 2008-01-18 12:47:25 PST
Sam, can you verify the fix using a Camino build, please?
Comment 27 Samuel Sidler (old account; do not CC) 2008-01-18 13:21:09 PST
Verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.12pre) Gecko/20080116 Camino/1.6b1 (like Firefox/2.0.0.12pre)
Comment 28 Darren 2008-01-18 15:42:00 PST
With ONE CAVEAT, this bug is FIXED in the latest nitely I have:
Version 2008011701 (1.5.5pre)

Command-F Find works as expected. Thanks!
/-key "Find as you type" does NOT hilite text.
Comment 29 Smokey Ardisson (offline for a while; not following bugs - do not email) 2008-01-18 15:51:36 PST
(In reply to comment #28)
> With ONE CAVEAT, this bug is FIXED in the latest nitely I have:
> Version 2008011701 (1.5.5pre)
> 
> Command-F Find works as expected. Thanks!
> /-key "Find as you type" does NOT hilite text.

Darren, "find as you type" has never worked correctly; see comment 7.  This bug was explicitly about Command-F.

Setting this back to FIXED.
Comment 30 Smokey Ardisson (offline for a while; not following bugs - do not email) 2008-01-18 16:01:27 PST
FWIW, I tracked down the bug I mentioned in comment 7 again.  It's bug 189039 and it stayed in Core:Keyboard:Find-as-you-type (the component for xpfe fayt), but the patch only touched toolkit/components/typeaheadfind/ (and some other toolkit code).
Comment 31 Vicki Brown 2008-01-31 10:43:45 PST
Confirmed in nightly. I just wanted to say THANK YOU.
(bug id=407432 was mine.  I NEED this functionality!)

Note You need to log in before you can comment on or make changes to this bug.