824324 - JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 133: reference to undefined property val.value

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: patch to fix radio.xml to use val.getAttribute('value')

During a test run of "make mozmill" of local DEBUG BUILD of
thunderbird (comm-central), I noticed the following warning:
thunderbird version is
$ hg identify
99fd4744910d+ tip

JavaScript strict warning: chrome://global/content/bindings/radio.xml, line 133: reference to undefined property val.value

radio.xml is comm-central's  mozilla/toolkit/content/widgets/radio.xml

Comparing the use of 'value' in other xml files under the same directory,
I think this is fixed by the attached patch.
I show the main change below:
             val.setAttribute("selected", "true");
-            this.setAttribute("value", val.value);
+            this.setAttribute("value", val.getAttribute('value'));
           }

Comment 1

[ISHIKAWA, Chiaki]

Comment on attachment 695288 [details] [diff] [review]
patch to fix radio.xml to use val.getAttribute('value')

I re-ran "make mozmill" with DEBUG BUILD of TB and no longer see the warning now.
So I think the patch is OK.

I put gavin's e-mail address since the
the last patch to this file was reviewed by him, it seems from what I learned about  the file using
http://hg.mozilla.org/mozilla-central/annotate/7b74f4ee76c9/toolkit/content/widgets/radio.xml

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 695288 [details] [diff] [review]
patch to fix radio.xml to use val.getAttribute('value')

"val" is supposed to be a "radio" element [1], whose binding inherits a "value" property from "control-item" [2].

[1] http://hg.mozilla.org/mozilla-central/annotate/d348dbf1dab4/toolkit/content/widgets/radio.xml#l435
[2] http://hg.mozilla.org/mozilla-central/annotate/d348dbf1dab4/toolkit/content/widgets/general.xml#l60

So it seems likely that some code somewhere is trying to set the radiogroup's selectedItem to a non-radio element (or a radio element whose binding is not attached?). I think we need to track down why this is happening exactly, before deciding on the fix. Using the attribute (this patch) is probably reasonable either way, but there may also be another bug in the consumer that this is masking.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> Comment on attachment 695288 [details] [diff] [review]
> patch to fix radio.xml to use val.getAttribute('value')
> 
> "val" is supposed to be a "radio" element [1], whose binding inherits a
> "value" property from "control-item" [2].
> 
 ...
> 
> So it seems likely that some code somewhere is trying to set the
> radiogroup's selectedItem to a non-radio element (or a radio element whose
> binding is not attached?). I think we need to track down why this is
> happening exactly, before deciding on the fix. Using the attribute (this
> patch) is probably reasonable either way, but there may also be another bug
> in the consumer that this is masking.

Thank you for the feedback. I am uploading an excerpt from a log of
make mozmill run.
This may help to locate the mentioned confusion of data types, then.

Comment 4

[ISHIKAWA, Chiaki]

Attachment: An excerpt from make mozmill run of local DEBUG BUILD of TB

Comment 5

[:aceman]

Also, .value and .getAttribute('value') are not always equivalent (at least for textbox elements).

I see this warning in the account manager (when manually running a debug build) and plan to find out which element is causing it.
I can try to take this bug if you don't mind, ISHIKAWA, chiaki .

Comment 6

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #5)
> Also, .value and .getAttribute('value') are not always equivalent (at least
> for textbox elements).
> 
> I see this warning in the account manager (when manually running a debug
> build) and plan to find out which element is causing it.
> I can try to take this bug if you don't mind, ISHIKAWA, chiaki .

Please take it then! I put your address in the assignee field.

Comment 7

[:aceman]

Attachment: patch

Ok, I have found the problem in the account manager. If there is no pref value for a radiogroup stored, we try to select the first child of it. But that sometimes is not a <radio> element. E.g. I got it on an IMAP account where the radiogroup(id="imap.deleteModel").firstChild was a <grid> element. So I change it to .getItemAtIndex(0) to let the radiogroup return the first item that is a radio. Seems to work. I do the same for menulist as that does the same unsafe assumption (that .firstChild.firstChild is a menuitem). Ishikawa, can you please run the tests again with this patch and see if you still get those warnings?

Comment 8

[:aceman]

I just noticed that your "excerpt from make mozmill run of local DEBUG BUILD of TB" shows the warning in one of the account manager tests, working on the Server settings pane. That is where I saw the problem at real usage and fixed it. So the patch should really help.

Comment 9

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #7)
> Created attachment 699433 [details] [diff] [review]
> patch

> Ishikawa, can you please run the tests again with this patch and see if you
> still get those warnings?

Good news. No more warning about val.value!
(sorry, I forgot how to clear the startup cache and so
changing .js file alone was not enough and had to re-run the build [using ccache certainly helps, but it took a few hours before the test was done.]

I hope this patch makes it to the official distribution soon.

TIA

TIA

Comment 10

[Ian Neal]

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

We should be able to rely on the xml, so this should be sufficient:
  else if (type == "radiogroup" || type =="menulist") {
    if (value == undefined)
      value = formElement.getItemAtIndex(0).value;
 
    formElement.value = value;

Might be worth double checking with Neil though.

Comment 11

[:aceman]

I am not sure that is the same. Setting .value may not initialize .selectedItem.

Comment 12

[:aceman]

The change with the .querySelector() is actually not required to fix the bug.

Comment 13

[[e-mail]]

Well, the XBL for both radio and menulist constructor tries to do this:
1. If an item has the selected attribute, it becomes the selected item
2. Otherwise, if the control has a value, the item with that value becomes selected
3. Otherwise, the selected index becomes zero

I don't think case 1 applies here, but it looks as if we could do
if (value == undefined)
  formElement.selectedIndex = 0;
else
  formElement.value = value;

Comment 14

[:aceman]

Attachment: patch v2

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/35d92e1faf46