tonyr Reporter
	Description • 24 years ago

calling setAttribute( 'value', "labelString") multiple times on a checkbox 
appends the text to the label rather than replacing it like it should.

	tonyr Reporter
	Comment 1 • 24 years ago

I have no idea what component this should belong to.

	John Morrison
	Comment 3 • 24 years ago

This is probably in the XBL binding for <checkbox> ... ben?

	John Morrison
	Comment 4 • 24 years ago

Changing Summary from "setAttribute for checkbox not working"

Dave: If I dump out the properties of a <checkbox>, I get duplicate 
instances for all the properties of the XULElement interface: 

  id
  className
  style
  database
  builder
  resource
  controllers
  boxObject

	John Morrison
	Comment 6 • 24 years ago

Here's the bug with the duplicate properties. Also note in the testcase (the 
second one) that the |value| of <checkbox/> has two values (apparently).

This does not happen with e.g., <text> or <button>

	Brendan Eich [:brendan] Assignee
	Comment 7 • 24 years ago

This looks like a JS API coherence bug: JS_SetPrototype is not doing the right
thing with the obj's map pointer.

/be

	Brendan Eich [:brendan] Assignee
	Comment 8 • 24 years ago

Adding js1.5 keyword.  Maybe only XBL uses JS_SetPrototype, but we need to fix
this for JS1.5 customers too.

/be

	Brendan Eich [:brendan] Assignee
	Comment 9 • 24 years ago

Re-summarizing.

	John Morrison
	Comment 10 • 24 years ago

Neat. Uh, but did I stumble into something not related to the original bug 
report. In other words, is the JS_SetPrototype issue behind the double display 
of the value attribute, or do I need to open a new bug for Tony's originally
reported problem.

	Brendan Eich [:brendan] Assignee
	Comment 11 • 24 years ago

The JS_SetPrototype bug is behind the duplicates in the for-in loop's output.

/be

	Brendan Eich [:brendan] Assignee
	Comment 13 • 24 years ago

Adding shaver for JS love.

/be

	Brendan Eich [:brendan] Assignee
	Comment 14 • 24 years ago

There was an unrelated buglet with XBL: it wasn't supplying the JSPROP_ENUMERATE
attribute when defining properties, so none of the binding props showed up in
the for-in loop output.  But hyatt just checked in that one-line fix today,
a=brendan.

/be

	Brendan Eich [:brendan] Assignee
	Comment 15 • 24 years ago

Is this really a blocker?

/be

	John Morrison
	Comment 16 • 24 years ago

I split off the original bug report as a new bug (for Ben), so Brendan can
set his own priority for this bug.

Brendan, your earlier response meant that "the duplicate instances of 
anonymous content for the <checkbox> is unrelated to this JS_SetPrototype 
issue". Yes?

	Brendan Eich [:brendan] Assignee
	Comment 17 • 24 years ago

No, the recurring property ids was due to this JS_SetPrototype bug.  The fact 
that no binding properties were enumerated was due to the unreported buglet that 
I identified for hyatt, and which he fixed pronto.

What's ben's bug, exactly?

/be

	John Morrison
	Comment 18 • 24 years ago

Ben's bug is: give a <checkbox>, which has anonymous content of 

      <xul:box flex="1" class="internal-box" autostretch="never" valign="top">
        <xul:box class="checkmark-box" autostretch="never">
          <xul:image class="checkbox-check"/>
        </xul:box>
        <xul:image class="checkbox-icon" inherits="src"/>
        <xul:html inherits="value,accesskey,crop" flex="1">
          <children/>
        </xul:html>
      </xul:box>

when you successively call <checkbox>.setAttribute('value', 'foo2');, 
you wind up with two text Nodes appended to the <xul:html> element.
Each time you call it, the oldest Node is removed, and the new Node
becomes the sibling of the remaining text Node. 

Unfortunately, the testcases above can't be used right now, they will
crash when loaded over the network, and won't load from disk -- that's
bug #41876

	Brendan Eich [:brendan] Assignee
	Comment 21 • 24 years ago

Adding mccabe.  Looking for lots of review and patch-testing.

/be

	Brendan Eich [:brendan] Assignee
	Comment 24 • 24 years ago

Phil, can you reapply the latest (diff -u) patch and rerun the testsuite?  I'm 
also hoping for others to try this patch, beat on it in mozilla, and bug me, as 
well as to re-code-buddy it (shaver? bueller?).

/be

	John Morrison
	Comment 25 • 24 years ago

I presume the above message was meant for pschwartau, so passing this one off
to Phil. See brendan's comment above.

	Brendan Eich [:brendan] Assignee
	Comment 26 • 24 years ago

Argh!  I went through a windows ftp client and got stinking ^Ms in the patch. 
Reattaching, sorry.

/be

	Brendan Eich [:brendan] Assignee
	Comment 28 • 24 years ago

I missed the most important case, waugh!  When an unmutated object is delegating
all its properties to its prototype, and you set a proto-property that has a
setter, or the new shared attribute, you do not want to make a new scope for the
object and clone that proto-prop into the object's new scope!

Patch and jsobj.c-only diff -wu for review attachments coming up.

/be

	Brendan Eich [:brendan] Assignee
	Comment 31 • 24 years ago

Phil gave me a clean bill of health last time, and if my "Final" patch (diff -u)
just above does the trick, I'm checking in.  r=shaver?

/be

	Brendan Eich [:brendan] Assignee
	Comment 32 • 24 years ago

Fix checked in -- thanks to all, especially to pschwartau!

/be

	John Morrison
	Comment 33 • 24 years ago

Reopening. The 'double enumeration' of the XULElement interface still
occurs. I'll attach a simpler testcase in a moment (but it's doing the
same thing as the second attachment to this bug report; just a bit
cleaner).

  1) Load this document then click the button. Look in the console for a
  dump of the properties.  Notice that the attributes of the XULElement
  interface are enumerated twice.

  2) Now, click the checkbox, and then click the button. The attributes
  of the XULElement are no longer enumerated twice. A new property
  'onxblclick' is now enumerated. (Note: I can find no reference to such
  a property in lxr (?)).

What is enumerated in (1) is, in order:

a) the eight attributes of XULElement (id, className, style, database,
   builder, resource, controllers, boxObject)
b) the checked property from the <checkbox> XBL
c) the value, crop, disabled, src, accesskey, imgalign properties from
   the <basetext> XBL that is extended by <checkbox>
d) the eight attributes of XULElement a second time
e) tagName from DOM2 Document interface
f) the attributes of DOM2 Node interface (nodeName, nodeValue,
   nodeType, parentNode, childNodes, firstChild, lastChild,
   previousSibling, nextSibling, attributes, ownerDocument, namespaceURI,
   prefix, localName)

What is enumerated in (2) is, in order, the same as in (1), with the 
exception that in (a), the property 'onxblclick' is enumerated instead
of the first enumeration of the XULElement properties.

(Tested on win95, mac, linux, optimized builds for 20000712nn [using a
modified version to get an alert on the mac]).

	Brendan Eich [:brendan] Assignee
	Comment 35 • 24 years ago

jrgm, thanks for keeping after this -- did you change the order of something, or
did my fix never make a previous testcase's symptom go away?  I thought several
of us verified the fix.  Anyway, this is a JS engine bug that can be reproduced
in the js shell:

js> o={}
[object Object]
js> o2={}
[object Object]
js> o.__proto__ = o2
[object Object]
js> Object.prototype.foo=1                  
1
js> Object.prototype.bar=2
2
js> for(i in o)print(i)
foo
bar
js> o2.baz=3
3
js> for(i in o)print(i)
foo
bar
baz
foo
bar

I'll have a patch soon.  I dimly recall this bug from long ago, but must not
have fixed it ever.  It's inherent in the way JS shares scopes among unmutated
objects and their prototypes.  The bug bites when you "mutate in the middle" of
the prototype chain.

	Brendan Eich [:brendan] Assignee
	Comment 36 • 24 years ago

This is purely a bug in the for..in loop implementation.  Patch (with a small
optimization that I removed from jsobj.c making a modified come-back) soon.

/be

	Brendan Eich [:brendan] Assignee
	Comment 39 • 24 years ago

Looking for patch testing and code review.

	Brendan Eich [:brendan] Assignee
	Comment 41 • 24 years ago

My final offer... I mean patch.  Hoping for test and review buddies.

/be

	Brendan Eich [:brendan] Assignee
	Comment 42 • 24 years ago

for..in fix checked in.

/be

	Phil Schwartau
	Comment 43 • 24 years ago

Using Mozilla tip builds 2000-07-14 1PM PDT on WinNT, Linux.

I can verify that there is no longer any double enumeration of the
XUL Element interface in John's testcases, on either WinNT or Linux. 
There is also no double enumeration in Brendan's testcase.

Hoewever, I'm seeing something I don't understand. I save attachment 11322 [details] 
locally as 11322.xul, and then open it in Mozilla. The first time it loads, 
I click on the test button and get ouput in the console: 30 properties of 
the checkbox, listed without repetition. I check the checkbox, and click
the button again. In the console, I now see 31 properties: the original 30,
plus a new property, 'onxblclick'.  So far, so good. 

But I uncheck the checkbox and click the test button, and still get 31 
properties in the console. That is, the 'onxblclick' property persists, 
even though the UI looks the same as it was when it first loaded. 

This can't be right, can it? It seems like the list of properties should
either be a constant 31, or else toggle back and forth from 30 to 31.

For the record, here is the output of Brendan's test case in the JS shell,
on both WinNT and Linux as of 2000-07-14 1PM PDT:

js> o={}
[object Object]
js> o2={}
[object Object]
js> o._proto_ = o2
[object Object]
js> Object.prototype.foo=1
1
js> Object.prototype.bar=2
2
js> for (i in o) print(i)
_proto_
foo
bar
js> o2.baz=3
3
js> for (i in o) print(i)
_proto_
foo
bar

	Brendan Eich [:brendan] Assignee
	Comment 44 • 24 years ago

Phil, your transcription of my testcase lost an underscore on both sides of 
"proto" -- the magic property is __proto__ (double-underscores on both sides), 
not _proto_.

The onxblclick property I know nothing about -- jrgm?  Maybe hyatt can say; he 
may have mentioned this to me before, but I can't recall more than the name.  I 
see no hits from http://lxr.mozilla.org/mozilla/search?string=onxblclick, which 
is strange.

/be

	David Hyatt
	Comment 45 • 24 years ago

It's just a temporary property.  I should arguably clean up after myself after 
executing the handler.  I forgot to, and so the property lurks.  It's harmless.

	Phil Schwartau
	Comment 46 • 24 years ago

May I mark this bug "Verified", then? Or is there still any more to be done?

	John Morrison
	Comment 47 • 24 years ago

Nope. It's done, I believe. (I'll save you a trip and just mark it verified 
now).