226549 - [FIX]Type Ahead Find settings are backwards

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Erik Broderson]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031121
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031121

It appears that on my computer, if I set Mozilla to use TAF for links only, it
will in fact search all text on the page, and if I set it to search for all text
on the page it will really only do links only.

Reproducible: Always

Steps to Reproduce:
1. Open up the Mozilla Preferences
2. Set TAF to search for either Links Only or All Text
3. Use TAF on any webpage and it should actually use the setting that you didn't
choose in prefs



Expected Results:  
If you have it set to search for Links Only, it should really only search links
only, and if you have it set to search for any text on the page it should really
do that ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm seeing this too.  The reason is a bug in radio.xml.  If you look in the
<constructor> of <binding id="radio">, it adds itself to the end of
parent.mRadioChildren.  At some point, timeless reversed an array walk (bug
216721) so that if you have two <radio> nodes inside a <radiogroup> the
constructor of the second one will fire FIRST.  So mRadioChildren ends up
reversed and selectedIndex is all busted.  Then see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/components/prefwindow/resources/content/pref-keynav.xul&mark=76#76

Now the reason this is a bug in radio.xml is that if you dynamically create and
insert radio elements you do NOT want them adding themselves to the _end_ of
parent.mRadioChildren, which is what they currently do.  They should either add
themselves in the right place or just clear out the parent's mRadioChildren
cache.  The latter is easier.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Totally untested (no working trees) patch

I nuked the destructor code too, since it was even more bogus (slice() does NOT
change the underlying array).

We should probably fix this before 1.6 and all...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not to mention that slice(i, i) would always return an empty array....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Tested patch

This fixes the bug... the first patch doesn't work, of course.	;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136153 [details] [diff] [review]
Tested patch

I don't know what whoever put stuff inside a <parameter> was smoking -- the XBL
sink just drops that text on the floor.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Taking and targeting optimistically...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136153 [details] [diff] [review]
Tested patch

If it's OK with reviewers, I'll also remove the JS strict warning for "var i"
in this file.  If desired, I can post a patch with that included...

Comment 8

[[e-mail]]

bz, you didn't see my patch in bug 223897?
(My fault for not setting depends earlier)

Comment 9

[Benjamin Smedberg]

See also bug 226437... radio elements created from a <template> also seem to
exhibit a similar broken behavior.

--BDS

Comment 10

[[e-mail]]

Comment on attachment 136153 [details] [diff] [review]
Tested patch

>-              this.mRadioChildren[this.mRadioChildren.length++] = aNode;
I've always wondered how this works - setting anArray[anArray.length] also
changes anArray.length as a side-effect. Presumably the post-incremented length
overwrites the now identical post-incremented length. Personally I prefer using
push.

>+              this.mRadioChildren = new Array();
Personally I like the [] style.

>+          else if (aNode.localName == "radiogroup" && aNode != this)
Actually we've already established that aNode != this by now.

>       <destructor>
>         <![CDATA[
>-          var parent = this.radioGroup;
>-          for (var i = 0; i < parent.mRadioChildren.length; ++i) {
>-            if (parent.mRadioChildren[i] == this) {
>-              parent.mRadioChildren.slice(i, i);
>+          var radioList = this.radioGroup.mRadioChildren;
>+          for (var i = 0; i < radioList.length; ++i) {
>+            if (radioList[i] == this) {
>+              radioList.splice(i, 1);
>               return;
>             }
>           }
Would this also benefit from simply clearing the parent's cache?

Comment 11

[Boris Zbarsky [:bzbarsky]]

No, I did not see the other patch till after I wrote this one.  Would have saved
me a few hours time.  :(

> Actually we've already established that aNode != this by now.

Yes, but I was going for "something minimally invasive that drivers would take
for 1.6b", since this is a 1.6a-era regression....

> Would this also benefit from simply clearing the parent's cache?

Depends on the circumstances.

In any case, we should get one or the other of these patches in; I really don't
care which.

Comment 12

[[e-mail]]

Comment on attachment 136153 [details] [diff] [review]
Tested patch

>+            if (this.mRadioChildren)
>               return this.mRadioChildren;
>+            else
>+              this.mRadioChildren = new Array();
OK, so I looked at a diff -w and you're not responsible for two of the coding
nits, but you introduced an else after return here and also you changed [] to
new Array() for no reason. Otherwise r=me.

Comment 13

[[e-mail]]

Comment on attachment 136153 [details] [diff] [review]
Tested patch

>       <destructor>
>         <![CDATA[
>-          var parent = this.radioGroup;
>-          for (var i = 0; i < parent.mRadioChildren.length; ++i) {
>-            if (parent.mRadioChildren[i] == this) {
>-              parent.mRadioChildren.slice(i, i);
>+          var radioList = this.radioGroup.mRadioChildren;
>+          for (var i = 0; i < radioList.length; ++i) {
>+            if (radioList[i] == this) {
>+              radioList.splice(i, 1);
>               return;
>             }
>           }
>         ]]>
>       </destructor>
I discovered a case where the list of radio children would be null when the
radio was destroyed.

Comment 14

[[e-mail]]

Attachment: bz's patch with extra null check

Comment 15

[Alec Flett]

Comment on attachment 136598 [details] [diff] [review]
bz's patch with extra null check

seems reasonable. sr=alecf

Comment 16

[[e-mail]]

Comment on attachment 136598 [details] [diff] [review]
bz's patch with extra null check

Fix visible UI issues (affects several dialogs).

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 136598 [details] [diff] [review]
bz's patch with extra null check

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 18

[Boris Zbarsky [:bzbarsky]]

r=me on the change Neil made to that patch, and checked in.

This fixes bug 223897 too; not sure whether Neil wants to do further cleanup in
that bug...

Comment 19

[Boris Zbarsky [:bzbarsky]]

This patch caused bug 227612, actually...  Further details there.