[FIX]Type Ahead Find settings are backwards

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
XUL
P1
minor
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Erik Broderson, Assigned: bz)

Tracking

({regression})

Trunk
mozilla1.6beta
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

14 years ago
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 ;)
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.
Assignee: general → hyatt
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Toolkit/Widgets: XUL
Ever confirmed: true
Keywords: regression
OS: MacOS X → All
QA Contact: general → shrir
Hardware: Macintosh → All
Created attachment 136148 [details] [diff] [review]
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...
Not to mention that slice(i, i) would always return an empty array....
Created attachment 136153 [details] [diff] [review]
Tested patch

This fixes the bug... the first patch doesn't work, of course.	;)
Attachment #136148 - Attachment is obsolete: true
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.
Attachment #136153 - Flags: superreview?(brendan)
Attachment #136153 - Flags: review?(neil.parkwaycc.co.uk)
Taking and targeting optimistically...
Assignee: hyatt → bz-vacation
Priority: -- → P1
Summary: Type Ahead Find settings are backwards → [FIX]Type Ahead Find settings are backwards
Target Milestone: --- → mozilla1.6beta
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

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

Comment 9

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

--BDS

Comment 10

14 years ago
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?
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

14 years ago
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.
Attachment #136153 - Flags: review?(neil.parkwaycc.co.uk) → review+

Comment 13

14 years ago
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.
Attachment #136153 - Flags: review+ → review-

Comment 14

14 years ago
Created attachment 136598 [details] [diff] [review]
bz's patch with extra null check
Attachment #136153 - Attachment is obsolete: true

Updated

14 years ago
Attachment #136598 - Flags: superreview?(alecf)
Attachment #136598 - Flags: review+

Comment 15

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

seems reasonable. sr=alecf
Attachment #136598 - Flags: superreview?(alecf) → superreview+

Comment 16

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

Fix visible UI issues (affects several dialogs).
Attachment #136598 - Flags: approval1.6b?

Comment 17

14 years ago
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
Attachment #136598 - Flags: approval1.6b? → approval1.6b+
Attachment #136153 - Flags: superreview?(brendan)
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...
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
This patch caused bug 227612, actually...  Further details there.

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.