289361 - Fix .checked initialization in checkbox ctor

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Note that I can't find a component for bugs in toolkit's bindings, so this one
will have to do.

Bug 274712 changed toolkit checkboxes to have a "value" property that shadows
(badly; for example the code in the constructor looks wrong) the "checked"
property.  It's not clear to me exactly why this was added.

If the idea is that XUL widgets that want to participate in the preferences
architecture need to expose some property for that architecture to read/write,
then perhaps the property would be more appropriately called "prefValue"?  And
in this case it would be quite nice to actually document this little bit of API
somewhere.  Preferably both in the preference code and in our toolkit api
documentation on Mozilla.org.

If the property was added for some other reason, I'd be curious to know the reason.

Here's hoping to hear from Ben.

Requesting blocking, given that if this ends up as part of our toolkit in a
release we won't be able to remove or change it without major pain in the future...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Of course it looks like I can't request blocking for the relevant milestone
(1.1) here... :(  Could we please fix the bugzilla Toolkit product to be usable?
 Please?

Comment 2

[Asaf Romano (gone)]

(poke timeless for bug 286830 and bug 287789 ;) )

Comment 3

[Asaf Romano (gone)]

Note: same applies to the colorpicker widget:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/colorpicker.xml#453

Comment 4

[Boris Zbarsky [:bzbarsky]]

True (though the colorpicker doesn't have the constructor bizarreness checkbox
does).

Comment 5

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

The rationale was to have a single property value that can be used to initialize
a widget. "value" is used for pretty much all others for this purpose.

Comment 6

[Boris Zbarsky [:bzbarsky]]

One problem is that for checkboxes in other markup languages (HTML comes to
mind) the "value" property already exists and means something quite different.

A second problem is the actual code (again, the code in the constructor looks
bogus to me).

Third problem is complete lack of documentation for this api change.

I'm not really in the position of defining the toolkit api, but I'm assuming
that bryner/bsmedberg/darin/ben/neil will sort out what the api they want to
expose here is.

Comment 7

[Brendan Eich [:brendan]]

So we could consider this a freebie for Ben and move on, from now on following
mconnor's write-up for toolkit changes, which requires review.  But I'm tired of
special pleading like that.  Plus, since the rationale (pretty obvious, but good
to hear in advance of the change, and get consensus) is now written down, it's
worth a brief discussion.

I made checkboxes and radiobuttons have a boolean checked property in 1995 when
creating the DOM ("Level 0", thanks for labeling it without specifying it, w3c
losers), not a value property, for two reasons:

1.  As bz notes, and as the w3c codified in 
http://www.w3.org/TR/REC-DOM-Level-1/level-one-html.html#ID-011100101, value is
the string content of the VALUE attribute associated with the input's NAME
attribute's content, that's sent to the server when the form is submitted.

2.  Because JS has dynamic types, a property's name is the one strong way to
distinguish something that's boolean- from something that's string-valued.  The
value is coerced on assignment, so this seemed like a good idea to catch bugs or
at least avoid confusion.

#1 is decisive for HTML; #2 by itself is not hugely convincing.  What Ben did
for XUL (apart from any bugs in the constructor) makes input-tweaking JS code
generic: no special cases for checkbox and radio type.  That's cool.

It's 2005 and the HTML 4 spec has been out for what, over 7 years.  Is the
generic-programming win Ben wanted good for all XUL consumers?  Is it worth the
difference from HTML, which could be confusing for XUL hackers recruited from
among the ranks of web developers? Might we have to harmonize HTML and XUL in
the future?  Or is this something that should be done only for prefwindow?

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> ... checkboxes and radiobuttons have a boolean checked property in 1995 when
> creating the DOM ("Level 0", thanks for labeling it without specifying it, w3c
> losers), ....

So Hixie reminded me that I mostly meant to slam Netscape and MS losers on the
DOM working group, really (and since I think highly of at least vidur, I
shouldn't say "losers"), not the w3c leadership, which was doing what its
members wanted.

/be

Comment 9

[Darin Fisher]

My 2 cents: consistency with the HTML programming model is good in general, but
I think it takes a back seat in this case as providing a generic way of
assigning a value to a control seems like a good thing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  What constitutes a "control" in XUL, and should all controls have a .value?
 For example, should the colorpicker widget have a .value?  Currently it
doesn't.  Note that this isn't an argument for or against .value but rather a
request that we, for once, spec out what our XUL language will look like and
then implement instead of just implementing things in an ad-hoc manner.

Comment 11

[Darin Fisher]

that's a good point.  i'd argue for more consistency here.  perhaps we should
move this discussion to the XUL wiki?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fine by me.  Should I open a new bug on the checkbox constructor?

Comment 13

[Darin Fisher]

Well... that, or maybe just morph this bug into fixing that problem?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ben, whichever works best for you.

Comment 15

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

How is the ctor code bogus, given that the presentation is associated with
checked="true" ? If a checkbox starts out in this state:

<checkbox value="true"/> 

then at binding attached time it will not be checked unless the value is sync'ed. 

<colorpicker/> does not implement this because you pretty much never inline a
colorpicker in a XUL dialog, but <colorpickerbutton/> does:

http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/colorpicker.xml#453

Comment 16

[Boris Zbarsky [:bzbarsky]]

> How is the ctor code bogus

The ctor code does:

  this.checked = this.value;

This is getting the value property.  The property getter is:

 53       <property name="value"      onset="return this.checked = val;"
 54                                   onget="return this.checked;"/>

So the constructor does |this.checked = this.checked|.  That seems pretty pointless.

All this theory aside, did you actually try the markup you suggest in comment
15?  It doesn't work in current trunk Firefox (so with the code under discussion).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase demonstrating that it doesn't work

Comment 18

[Boris Zbarsky [:bzbarsky]]

On a separate note, for the api discussion, if the idea is that the value
_attribute_ (as opposed to just property) does something, then we have some more
issues:

<checkbox value="true" checked="false" />
<checkbox value="false" checked="true" />

Given those two DOM nodes, are they checked or not?  What about the following
two checkboxes (which look identical in the DOM, btw, so should be just like the
two checkboxes above):

<checkbox value="false" id="foo" />
<script>document.getElementById("foo").setAttribute("checked", "true")</script>

<checkbox value="true" id="bar" />
<script>document.getElementById("bar").setAttribute("checked", "false")</script>

What about:

<checkbox id="persister" value="false" persist="value" />

What happens when the user checks this?  What happens the next time the XUL
document is loaded?  (Hint: if implemented per comment 15, the checkbox will
never be checked when the document loads.)

Finally, what about existing XUL-based apps that happened to use the "value"
attribute on a checkbox for storing something of their own?  Sounds to me like
they'd just break with no warning whatsoever.

Comment 19

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

Oh. Duh. Sorry.

resummarizing. 

Comment 20

[Brendan Eich [:brendan]]

This should get fixed sooner or later for 1.1.  Flagging so it won't get lost.

/be

Comment 21

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

Attachment: patch

Something like this better achieves what is desired - |.value|'s getter now
returns the boolean value of its attribute equivalent, and if there is no such
attribute returns |.checked|. Also, |setChecked| is updated to keep the value
attribute/property combo sync'ed as the |.checked| property changes.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193110 [details] [diff] [review]
patch

This still has issues because it can lead to issues when the checked and value
properties have different values, no?  And the various other issues I brought
up in comment 18?  Some are addressed, some are not.  the last issue I raised I
guess we're not planning to address, but I'd like to see the others addressed.

Comment 23

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

Attachment: more comprehensive

A more comprehensive approach. 

Like the previous patch, this makes sure "checked" and "value" attributes are
set when |setChecked| is called. 

This patch makes a policy declaration - "value" is the real master of the
checkbox's state. |.checked| looks at the "value" attribute first if present
before relying on "checked".

We then also add an DOMAttrModified event handler to handle attribute changes
and sync state. 

I think the combination of these two things is really the best we can do to
handle inconsistency between the attributes, but what do you think, Boris?

I'll attach the combined test case file I've been working with after this.

Comment 24

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

Attachment: combined test case (includes bz's examples from c15)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193197 [details] [diff] [review]
more comprehensive

I'd like a toolkit peer to OK the change from checked to value as the "primary"
attribute, and I'd like to see some recursion protection in the handler or in
setChecked (that is, attr changes triggered from setChecked should not reenter
setChecked in a nontrivial way).  But with those caveats, I guess this is the
best we can do if we insist on having both attributes around.

Comment 26

[jag (Peter Annema)]

Do we really need to support a |value| attribute on checkboxes? The |value|
property as a helper to simplify the preference code should be enough. Just
leave it at that and keep the |checked| attribute and property to set a
checkbox's state, no need IMO to introduce more code to try and keep these two
attributes and properties in sync.

Comment 27

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

You basically need a value attribute to allow the checkbox to be initialized
this way before bindings have been attached. If the bug that attachedness is
tied to frame construction is ever fixed, then this will not be necessary. 

Comment 28

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

Attachment: patch

After some thought, I've decided it's much easier to do this entirely within
the context of preferences than meddle with the apis for checkbox and
colorpicker. The problem is that we need the value attribute for initialization
prior to binding attachment because of the bug with bindings not being attached
prior to frame construction. Rather than mess with the XUL api to work around
other XUL bugs, this patch simply inserts some special cases into the
preferences code (which is an implementation detail and thus effectively
private) to work around. I still think a common state property is not a bad
idea, but I'll pass on it for this release.

Comment 29

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

Attachment: patch

After some thought, I've decided it's much easier to do this entirely within
the context of preferences than meddle with the apis for checkbox and
colorpicker. The problem is that we need the value attribute for initialization
prior to binding attachment because of the bug with bindings not being attached
prior to frame construction. Rather than mess with the XUL api to work around
other XUL bugs, this patch simply inserts some special cases into the
preferences code (which is an implementation detail and thus effectively
private) to work around. I still think a common state property is not a bad
idea, but I'll pass on it for this release.

Comment 30

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

This patch rolls checkbox.xml back from v1.7 to v1.6, removes the "value"
property from <colorpicker/> similarly, and introduces special handling in
preferences.xml to handle checkbox and colorpicker widgets.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 193484 [details] [diff] [review]
patch

a=me for 1.8b4/fx1.5.

/be

Comment 32

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

landed on 1.8 br, waiting for a trunk tree to build before landing there...

Comment 33

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

fixed on the trunk too.