70033 - UI elements need to be disabled if the PrefIsLocked is true.

Status: VERIFIED FIXED

(SeaMonkey :: Preferences, enhancement)

Description

[eddyk]

While the prefs backend already supports the locking feature, it would be
confusing if users are allowed to modify elements that are locked.

Comment 1

[sairuh (rarely reading bugmail)]

confirming. cc'ing blake, jag and alecf to see how feasible this'd be.

Comment 2

[Alec Flett]

this is totally feasable. We just need to fix the preference window's backend to
support it.

Comment 3

[eddyk]

I'm almost ready to put up for review of some changes to:

nsPrefWindow.js - changes the widgetStateManager to also handle disabled
properties and use PrefIsLocked

radioBindings.xml - changes to radiogroup to disable all children

If the review of these pass, I'll probably start on some of the other pref
panels that handle elements themselves, thus keeping wsm from doing its thing.

Comment 4

[eddyk]

Attachment: proposed patch to disable locked prefs and radiogroups

Comment 5

[eddyk]

who wants to review this?  Some comments on the patches:

nsPrefWindow.js
- added function getPrefIsLocked which uses nsIPref::PrefIsLocked.  Dunno if 
PrefIsLocked works yet, but if it doesn't -- that's a separate bug.

radioBindings.xml
- Given my limited experience with this stuff, I feel kinda funny about these 
changes, but it was all I could come up with.  I'm hitching a ride in 
selectedAttribute to set and unset the disabled attribute which triggers the 
setter/getter of radiogroup.  IMO, it's ugly since selectedAttribute shouldn't 
have anything to do with the disabled property, but I couldn't get the 
disabled setter/getter to be called on radiogroup init.

A quick test by having the getPrefIsLocked return true shows the majority of xul 
elements in the pref window being disabled.  The rest are works in progress.

Comment 6

[timeless]

general comments:
tabs are bad
inconsistent bracing {} is bad
debug's are bad
please try to conform to the file's prefs (esp the modeline if it generally 
describes the file)

specifics:
+ var isPrefLocked = this.getPrefIsLocked(prefstring);
+ if (isPrefLocked)
+  root.disabled="true";
why not:
root.disabled=this.getPrefIsLocked(prefstring);

+ if (v == 'true') return true;
+ return false;
please consider
return v=='true';

+          if (!val) this.removeAttribute('disabled');
+            return val;
return should not be indented here.

+          if (this.disabled)
+            this.disabled='true';
+          else
+            this.disabled=false;
please use:
this.disabled=!this.disabled;

overall concepts look great.

Comment 7

[eddyk]

thanks timeless.  I appreciate the feedback on the diffs.

However, why is debug bad?  I happy to remove it, but wouldn't it make life
easier if it was left it and there was a bug to track down?  Or is performance
greatly affected by debugs in xul?

Comment 8

[timeless]

the debug stuff is iffy, recently people have kind of requested that we not 
include debug stuff unless it's really necessary.  If you feel it's necessary 
i'm not going to reject your patch because of it. [not that i have the power to 
reject patches in general]

Comment 9

[eddyk]

Attachment: Patch redux. No tabs and other cleanup.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Does setting the .disabled property on each child radio button work? (rather 
than explicitly setting the disabled attribute). If that's possible, that's 
preferred. 

Otherwise, r=ben. This looks great. 

Comment 11

[Peter "jag" Annema]

I think .disabled works for radiobuttons. On top of that, why not use a
broadcaster instead of this loop?

Comment 12

[Peter "jag" Annema]

ignore the broadcaster comment, I was high.

Comment 13

[[e-mail]]

The problem with setting disabled is that the widget state manager only sets the
attribute, not the property, so that the property setter is not used.

Comment 14

[timeless]

fix checked in, r=timeless. thanks

Comment 15

[eddyk]

Attachment: radioBinding.xml regression fix.

Comment 16

[eddyk]

Interestingly I found that the 2/25/01 patch no longer works for radiogroups.

I've modified the patch by setting the disabled attribute in the setter/getters
for the radio value property.  I think this is a guaranteed way to be called in
order to setup the disabled property.

I've also removed the previous spot where I was setting the disabled property.

Comment 17

[Rodney Velasco]

assigning QA to luke, adding sarah to Cc.

Comment 18

[eddyk]

Attachment: alternative change. Instead of fixing the radio binding, change wsm handling of the radio element.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

element.disabled = element.getElementsByAttribute( "disabled", 
aDataObject.disabled );
getElementsByAttribute returns a NodeList. I think this is what you mean:

element.disabled = element.disabled = element.getElementsByAttribute( 
"disabled", aDataObject.disabled ).length;

Comment 20

[eddyk]

Attachment: alternate approach - have wsm set the disabled attribute for radio, instead of radio binding hack

Comment 21

[eddyk]

Instead of hacking the radio xml binding to trigger the setter/getter for the
disabled attribute, this last patch hacks nsWidgetStateManager to set the
disabled attribute for radio elements in order to trigger the getter/setter.

sr= ben via irc.

Comment 22

[timeless]

r=timeless

Comment 23

[eddyk]

checked in and seems to be working (again).  marking fixed

Comment 24

[Rodney Velasco]

updating qa contact, adding lrg to cc

Comment 25

[Rodney Velasco]

verified general pref panels on 2001052308 Commercial builds (all tier 1
platforms).  Some elements still need to adhere to locking standard, will update
tracking bug 70538 with those bugzilla ID's.