Closed Bug 1500620 Opened 10 months ago Closed 9 months ago

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

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set

Tracking

(thunderbird64 fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird64 --- fixed
thunderbird65 --- fixed

People

(Reporter: aceman, Assigned: darktrojan)

References

Details

Attachments

(1 file, 3 obsolete files)

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;
Summary: TypeError: setting getter-only property "editable" emailWizard.js:1268 → TypeError: setting getter-only property "editable" emailWizard.js:1238
It's working for me. Has the menulist in question got the right binding? (chrome://messenger/content/menulist.xml#menulist-editable)
I don't know, but even in that binding the 'editable' property is readonly, so it has no setter.
Attached patch 1500620-editable-1.diff (obsolete) — Splinter Review
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.
Attachment #9019274 - Flags: review?(acelists)
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?
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.
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.
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.
I think we might not have done the best job in bug 1486051 comment #30:
https://hg.mozilla.org/comm-central/rev/bf51f2112d4f
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 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.
Attachment #9019274 - Flags: feedback+
(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.
(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.
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.
Flags: needinfo?(acelists)
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 .
Flags: needinfo?(acelists)
Attached patch 1500620-editable-2.diff (obsolete) — Splinter Review
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.
Attachment #9019274 - Attachment is obsolete: true
Attachment #9019274 - Flags: review?(acelists)
Attachment #9019625 - Flags: review?(acelists)
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?
Attachment #9019625 - Flags: feedback?(ben.bucksch)
> 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 on attachment 9019625 [details] [diff] [review]
1500620-editable-2.diff

r+ under the conditions of the last comment
Attachment #9019625 - Flags: feedback?(ben.bucksch) → review+
(xref bug 266590, which kind-of states the opposite, that some bindings are attached *before* the DOM is updated.)
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.
> I thought it was well-known that XBL is an unreliable piece of junk.

:-)
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...
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 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.
Attachment #9019625 - Flags: review?(acelists) → review-
Attached patch 1500620-editable-3.diff (obsolete) — Splinter Review
How about now?
Attachment #9019625 - Attachment is obsolete: true
Attachment #9020717 - Flags: review?(acelists)
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
(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".
(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 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.
Attachment #9020717 - Flags: review?(acelists) → review+
Attachment #9020717 - Attachment is obsolete: true
Attachment #9024683 - Flags: review+
Keywords: checkin-needed
Attachment #9024683 - Flags: approval-comm-beta?
Attachment #9024683 - Flags: approval-comm-beta? → approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fbd01a6e230f
Make menulist editable/non-editable with attributes instead of properties. r=aceman
Status: NEW → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
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.
Flags: needinfo?(geoff)
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.
Flags: needinfo?(geoff)
Filed and posted a fix to bug 1507304.
Depends on: 1507304
Depends on: 1516134
You need to log in before you can comment on or make changes to this bug.