193966 - Keyboard Navigation:Find automatically...any text on page option doesn't set if other preferences are changed.

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[prabhath]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030211
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030211

I'm using RedHat 8.0 and when resetting the preferences of Mozilla (i.e.
creating a new user and running Mozilla for the first time under that user) try
changing several settings in the Preferences. Then set the option "Find
automatically...Any Text on Page" radio button and then click OK. Go back into
preferences and you'll notice that all your other options were set, but the
Keyboard Navigation option was reset. Not a biggie, but gets annoying when doing
this often and having a lot of preferences to redo everytime. 

Also, I've noticed this problem on the WINDOWS XP & MAC OS X versions, although
not for this particular preference. This seems to occur when you set several
preferences at once.

Reproducible: Always

Steps to Reproduce:
1. Create New user
2. Run Mozilla for first time under that user
3. Select several preference changes (i.e. suppress popups, do the mail
settings, etc, etc)
4. Set the Keyboard Navigation:Find automatically...:Any Text on Page option
5. Click OK on preferences.
6. Check the same preference setting, it got reset.



Expected Results:  
Set the preference setting. Also, since this occurs on different preference
settings (i don't remember which) maybe a lookover of the preference code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Over to keyboard nav.  Looks like someone forgot to list the
findAsYouTypeAutoWhat radiogroup in _elementIDs in that pref panel.

Comment 2

[Jay Davis]

I am seeing this problem on W2K.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like you also need to add tabNavigationLinks and tabNavigationForms to the
list (see bug 194412)

Comment 4

[Asa Dotzler [:asa]]

aaronl, can you get something to us quickly? We shouldn't ship this in a final
if at all possible. 

Comment 5

[Aaron Leventhal]

I'll take another look when I get back from Chicago this week. I remember that I
have to handle those prefs in the oncommand handlers, instead by listing the
id's, because they do some special stuff.

Comment 6

[Asa Dotzler [:asa]]

This seems like it should be a simple fix and we should get it for 1.3. Adding
blocking1.3+ flag.

Comment 7

[Aaron Leventhal]

Attachment: Some key nav prefs don't work with the elementID's trick. We have to store these temporarily until saveKeyNavPrefs can set them

Also changes the variable name linksOnlyPref to gLinksOnlyPref

Comment 8

[Samir Gehani]

Comment on attachment 115936 [details] [diff] [review]
Some key nav prefs don't work with the elementID's trick. We have to store these temporarily until saveKeyNavPrefs can set them

r=sgehani

Comment 9

[jag (Peter Annema)]

Comment on attachment 115936 [details] [diff] [review]
Some key nav prefs don't work with the elementID's trick. We have to store these temporarily until saveKeyNavPrefs can set them

nsIPref is deprecated, use nsIPrefService and -Branch.

Or use
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/co﷒0﷓

I think there is a mechanism for loading/saving prefs that don't directly get
reflected in the UI. One of them is to store them in the pageData object that
is persisted across panel switches, see e.g.
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/co﷒1﷓
That way you won't have to use global prefs or store them on the prefsTreeNode.

Another is to use SetFields and GetFields, see
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/nsWidgetS
tateManager.js#113

Comment 10

[Aaron Leventhal]

Attachment: New patch using Jag's suggestions

Should I carry Samir's r= or get a new one?

Comment 11

[Aaron Leventhal]

Attachment: New patch using Jag's suggestions

Comment 12

[Aaron Leventhal]

Comment on attachment 116141 [details] [diff] [review]
New patch using Jag's suggestions

Or should I carry Samir's r= conditional on Jag's sr=?

Comment 13

[jag (Peter Annema)]

Comment on attachment 116141 [details] [diff] [review]
New patch using Jag's suggestions

Remove the dumps, inside saveKeyNavPrefs use |data| instead of |gData|.

The check for |!gData| in initPrefs is useless, the rest of your code won't
work if it doesn't exist (which would indicate a big problem elsewhere).

Use |!("tabNavPref" in gData)|. You could use its absence as an indicator that
"linksOnlyPref" is absent too and combine the two pref reads. They don't need a
try/catch btw unless they don't have a default value set in all.js.

If init is successful, you can expect "tabNavPref" and "linksOnlyPref" to exist
in |data| in |saveKeyNavPrefs|, no need to check for their existence. If init
wasn't successful, you're screwed way more than that you could recover from
there by using |in| checks.

Comment 14

[Aaron Leventhal]

Attachment: Get rid of unnecessary try/catch, null checks, etc.

Comment 15

[jag (Peter Annema)]

Comment on attachment 116162 [details] [diff] [review]
Get rid of unnecessary try/catch, null checks, etc.

sr=jag

Comment 16

[Christopher Blizzard (:blizzard)]

Comment on attachment 116162 [details] [diff] [review]
Get rid of unnecessary try/catch, null checks, etc.

a=blizzard

Comment 17

[Asa Dotzler [:asa]]

03/05/2003 09:42	aaronl%netscape.com 	mozilla/ xpfe/ components/ prefwindow/
resources/ content/ pref-keynav.xul 	1.2.4.1 	MOZILLA_1_3_BRANCH  	5/5  	Bug
193966. Some keynav prefs not getting saved when switching to another pref panel
before hitting OK. r=samir, sr=jag, a=blizzard
03/05/2003 09:42	aaronl%netscape.com 	mozilla/ xpfe/ components/ prefwindow/
resources/ content/ pref-keynav.js 	1.1.8.1 	MOZILLA_1_3_BRANCH  	24/43 

Comment 18

[sairuh (rarely reading bugmail)]

aaronl, could you check this into the trunk? (unless you already did.)

Comment 19

[Aaron Leventhal]

checked in

Comment 20

[sairuh (rarely reading bugmail)]

looking at page info (unless that's an incorrect way), it's text/xml.

Comment 21

[sairuh (rarely reading bugmail)]

ignore last comment. wrong bug (so many tabs ;).

Comment 22

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.03.13 comm trunk on all platforms.