1500620 - TypeError: setting getter-only property "editable" emailWizard.js:1238

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[:aceman]

When setting up an account via the account wizard, I get:

TypeError: setting getter-only property "editable" emailWizard.js:1268

It seems some occurrences of 'editable' property weren't fixed in bug 1486051:
comm/mail/components/accountcreation/content/emailWizard.js:
1238 menulist.editable = false;
1244 menulist.editable = true;

Comment 1

[Geoff Lankow (:darktrojan)]

It's working for me. Has the menulist in question got the right binding? (chrome://messenger/content/menulist.xml#menulist-editable)

Comment 2

[:aceman]

I don't know, but even in that binding the 'editable' property is readonly, so it has no setter.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1500620-editable-1.diff

You're right, it doesn't work. I don't get the error, but that's not really important. Since setting .editable = true is never going to work, I've made both changes with attributes instead of properties.

Comment 4

[Ben Bucksch (:BenB)]

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menulist#a-editable

Did this change upstream? Did they forget to update the docs? Do the attributes work?

Comment 5

[Ben Bucksch (:BenB)]

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/menulist#p-editable (both)

According to current docs, there is both an attribute and a property called "editable", which both do what we intend here.

Comment 6

[:aceman]

The docs may be outdated.
I think they dropped editable menulists in m-c completelly, we just adopted it for c-c, see bug 1486051 and bug 1457216 (m-c).
In the adoption, it seems we made it a getter only and to switch to it you need to set the attribute. Then the chrome://messenger/content/menulist.xml#menulist-editable binding gets applied.
The change is in v63.

Comment 7

[Ben Bucksch (:BenB)]

Wouldn't it be easier to just make the property settable? That's just one line of code as well. That would fix everything at once.

Comment 8

[Jorg K (CEST = GMT+2)]

I think we might not have done the best job in bug 1486051 comment #30:
https://hg.mozilla.org/comm-central/rev/bf51f2112d4f

Comment 9

[:aceman]

I'd guess the normal menulist gets the binding from m-c which doesn't have the editable property. Once we set the attribute to true, we get the menulist-editable (from c-c) attached and it gets at least the getter to check the value. It could be made into a setter so that you can set it to 'false'. But I'd guess it couldn't be used to set it to true (and get a editable menulist from a normal one), as the normal one doesn't have the property. Probably darktrojan will know the details.

Comment 10

[:aceman]

Comment on attachment 9019274 [details] [diff] [review]
1500620-editable-1.diff

Review of attachment 9019274 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this solves the error and makes the SMTP picker in the account wizzard work now. But choosing another item (server) in the menulist and the choosing the first (new) server I get:
ReferenceError: reference to undefined property "inputField" in emailWizard.js:885:1
It may be related, maybe at the time of this call the editable menulist binding isn't yet bound to the element. 'inputField' again is only defined on the menulist-editable.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #9)
> I'd guess the normal menulist gets the binding from m-c which doesn't have
> the editable property. Once we set the attribute to true, we get the
> menulist-editable (from c-c) attached and it gets at least the getter to
> check the value. It could be made into a setter so that you can set it to
> 'false'. But I'd guess it couldn't be used to set it to true (and get a
> editable menulist from a normal one), as the normal one doesn't have the
> property. Probably darktrojan will know the details.

Right. I don't want a property you can change from true to false, but not from false to true. That's the sort of thing that creates wasted time and swearing.

Comment 12

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #10)
> Thanks, this solves the error and makes the SMTP picker in the account
> wizzard work now. But choosing another item (server) in the menulist and the
> choosing the first (new) server I get:
> ReferenceError: reference to undefined property "inputField" in
> emailWizard.js:885:1
> It may be related, maybe at the time of this call the editable menulist
> binding isn't yet bound to the element. 'inputField' again is only defined
> on the menulist-editable.

I can't reproduce this one either.

Comment 13

[Geoff Lankow (:darktrojan)]

Is there any reason you can think of might explain why you're seeing these bugs but I'm not? Perhaps if you give a step-by-step list of what you're doing, maybe I'm doing something else.

Comment 14

[:aceman]

Yes, I also wasn't seeing the 'getter' error in one profile.
There are some prefs to toggle on to see more thorough JS errors in the error console.
Try e.g. javascript.options.strict = true .

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1500620-editable-2.diff

I think this should do the job (works for me), the binding we're changing to is already in memory, so the change should happen before we get to the end of the event loop.

Comment 16

[:aceman]

Comment on attachment 9019625 [details] [diff] [review]
1500620-editable-2.diff

What do you think Ben? Is it OK if the onChangedManualEdit() is postponed for a moment?

Comment 17

[Ben Bucksch (:BenB)]

> I'd guess the normal menulist gets the binding from m-c which doesn't have the editable property.
> Once we set the attribute to true, we get the menulist-editable (from c-c) attached

Oh, I see! The menulist[editable] is a different binding. hm, OK. Under that circumstance, it makes sense not to have a property.

> I don't want a property you can change from true to false, but not from false to true.
> That's the sort of thing that creates wasted time and swearing.

Completely agreed.

It's the same surprise that I had here, that the property doesn't work, although it's documented to exist. But I understand it would be a major rework to make it work, so that's not worth it.

> setTimeout(() => this.onChangedManualEdit())

This is not so nice. Are binding not guaranteed to bind immediately? I'd be surprised, if bindings attach lazily. That's bound to cause race conditions in lots of code.

If a) you verified that XUL bindings are really that broken by design b) you've tested that this fixes the bug in comment 10, and c) you added a code comment "// give menulist[editable] binding time to attach", then I'm OK with this.

Comment 18

[Ben Bucksch (:BenB)]

Comment on attachment 9019625 [details] [diff] [review]
1500620-editable-2.diff

r+ under the conditions of the last comment

Comment 19

[Ben Bucksch (:BenB)]

(xref bug 266590, which kind-of states the opposite, that some bindings are attached *before* the DOM is updated.)

Comment 20

[Geoff Lankow (:darktrojan)]

Bindings attach whenever they feel like it. (Yes, I tested this exact case just to prove it.) I thought it was well-known that XBL is an unreliable piece of junk.

Comment 21

[Ben Bucksch (:BenB)]

> I thought it was well-known that XBL is an unreliable piece of junk.

:-)

Comment 22

[:aceman]

It may not be just XBL, but everything in m-c is getting async and that causes all sorts of problems like this. I think they are overdoing it. E.g. there is a difference whether you have JS code inline a chrome page (xhtml) or whether the JS is in a separate file. In the latter case some other code may execute between displaying the page and it's JS being run, that can postpone the JS loading indefinitely (and it happens, I have a 100% reproducible case). So you can see the unfinished page on screen for a non-trivial amount of time, which is bad and even harmful (shows misleading content). And of course m-c is pushing and mass-converting all internal chrome pages into the latter variant, due to some Content security policy...

Comment 23

[Jorg K (CEST = GMT+2)]

Only that we use that "junk" for 176 bindings :-( - BTW, what does this do?
      // Ensure the XBL binding is created eagerly.
      list.appendChild(MozXULElement.parseXULToFragment("<richlistitem/>"));

Comment 24

[:aceman]

Comment on attachment 9019625 [details] [diff] [review]
1500620-editable-2.diff

Review of attachment 9019625 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this does not work for me, I still get the error for inputField.

Comment 25

[Geoff Lankow (:darktrojan)]

Attachment: 1500620-editable-3.diff

How about now?

Comment 26

[Ben Bucksch (:BenB)]

Comment on attachment 9020717 [details] [diff] [review]
1500620-editable-3.diff

Review of attachment 9020717 [details] [diff] [review]:
-----------------------------------------------------------------

::: common/bindings/menulist.xml
@@ +23,5 @@
>      <implementation>
> +      <constructor><![CDATA[
> +        this.mSelectedInternal = null;
> +        this.mAttributeObserver = null;
> +        this.setInitialSelection();

Is there a way that you can call the super class constructor, instead of copying it?

::: mail/components/accountcreation/content/emailWizard.js
@@ +1248,5 @@
>        _show("outgoing_authMethod");
> +
> +      // We cannot rely on the editable menulist binding being
> +      // attached immediately.
> +      menulist.addEventListener("bindingattached", () => {

Much better

Comment 27

[Geoff Lankow (:darktrojan)]

(In reply to Ben Bucksch (:BenB) from comment #26)
> Is there a way that you can call the super class constructor, instead of
> copying it?

If there is, I don't know what it is. I can get to this.__proto__.__proto__.constructor, but it requires me to use "new".

Comment 28

[:aceman]

(In reply to Geoff Lankow (:darktrojan) from comment #27)
> If there is, I don't know what it is. I can get to
> this.__proto__.__proto__.constructor, but it requires me to use "new".

Does that have drawback?

Comment 29

[:aceman]

Comment on attachment 9020717 [details] [diff] [review]
1500620-editable-3.diff

Review of attachment 9020717 [details] [diff] [review]:
-----------------------------------------------------------------

Yes this works for me, thanks.
We need this in some form to get a working account creation wizard.

::: common/bindings/menulist.xml
@@ +23,5 @@
>      <implementation>
> +      <constructor><![CDATA[
> +        this.mSelectedInternal = null;
> +        this.mAttributeObserver = null;
> +        this.setInitialSelection();

If nobody can find a better way to do this without copying it, at least add a comment here that the block needs to be kept in sync with chrome://global/content/bindings/menulist.xml#menulist.

Comment 30

[Geoff Lankow (:darktrojan)]

Attachment: 1500620-editable-4.diff

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fbd01a6e230f
Make menulist editable/non-editable with attributes instead of properties. r=aceman

Comment 32

[Markus Adrario [:Taraman]]

This patch causes a lot of warnings on the calendar-event-dialog in our test-logs:
> [task 2018-11-13T18:26:53.016Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.032Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.033Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.159Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 640: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.160Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.202Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 640: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.203Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.305Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.306Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.323Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.326Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.436Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 640: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.437Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.474Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 640: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.475Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.573Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.574Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 1442: TypeError: this.initialize is not a function
> [task 2018-11-13T18:26:53.764Z] 18:26:53     INFO -  JavaScript error: chrome://calendar/content/datetimepickers/datetimepickers.xml, line 383: TypeError: this.initialize is not a function

This seems to happen on every opening of te dialog. I did not check yet if it happens elsewhere as I'm in a hurry right now.

Comment 33

[Geoff Lankow (:darktrojan)]

What the? Why does that have code that calls initialize, but no method called initialize? I'll file a follow-up bug, because I'm sick of seeing this bug in my inbox.

Comment 34

[Geoff Lankow (:darktrojan)]

Filed and posted a fix to bug 1507304.

Comment 35

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 3):
https://hg.mozilla.org/releases/comm-beta/rev/7b73575205fc