1523384 - Restore the menulist binding after its removal in bug 1518932

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1518932 removes the menulist binding by moving to custom elements.

We still extend this binding for editable menulists and some other bindings.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1523384-menulist-binding.patch

Only one review needed.

This should work. Normal menulists use the m-c custom elements menulists and the ones that extends the menulist use now the to us moved binding.

Bug 1518932 is in autoland and cam merge to m-c in one of the next merges.

Comment 2

[Jorg K (CEST = GMT+2)]

Hmm, for richlistbox our clone was called xbl-richlistbox. Shouldn't we do this here, too?

Comment 3

[Richard Marti (:Paenglab)]

I'd say no, because we don't use normal menulists with our binding but only extend other bindings with the menulist binding.

Comment 4

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=54dbcecb8cae9ed00d36c01eb4bb5f847d6148a1

Doing this on top of autoland is very easy:
https://hg.mozilla.org/try-comm-central/rev/54dbcecb8cae9ed00d36c01eb4bb5f847d6148a1#l1.14

BTW, I've included more stuff in the push, so if it breaks, it may be not you fault.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9039618 [details] [diff] [review]
1523384-menulist-binding.patch

OK, first reviewer wins ;-(

Comment 6

[Jorg K (CEST = GMT+2)]

But winning doesn't mean being right :-(

The try looks a bit terrible, looks like editable menu lists broke, since attachment reminder tests fail now.

Comment 7

[:aceman]

I assume the forking of the XBL menulist is a temporary solution.
Can we convert the editable menulist part to a Custom element that extends the m-c menulist?

For the test breakage:
E.g. test-message-filters.js::test_address_books_appear_in_message_filter_dropdown fails with
JavaScript error: chrome://messenger/content/addressbook/addrbookWidgets.xml, line 106: TypeError: menulist.appendItem is not a function
JavaScript error: chrome://messenger/content/searchWidgets.xml, line 105: TypeError: menupopup is undefined

Maybe at https://searchfox.org/comm-central/source/mailnews/base/search/content/searchWidgets.xml#89 we can't get the menulist as custom elements do not have anonymous elements?
I assume some of our menulists are custom elements now, only some (that extend it) are based on the XBL copy you make in the patch.

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #7)

I assume the forking of the XBL menulist is a temporary solution.
Can we convert the editable menulist part to a Custom element that extends
the m-c menulist?

That's the idea yes.

I assume some of our menulists are custom elements now, only some (that
extend it) are based on the XBL copy you make in the patch.

Yes, with this patch our special menulists would still be xbl, and the normal ones CEs.

Comment 9

[Jorg K (CEST = GMT+2)]

Can we do another try with a patch that renames the binding to xbl-menulist. I'd like to see whether it fixes any of the current bustage.

Comment 10

[Jorg K (CEST = GMT+2)]

Richard send me a patch that uses xbl-menulist in a PM. Here's a try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bad9b9ec9a804d4a06e753642c5afb76f9768944

Hmm, I've never posted the comment, so the result is that the number of test failures is reduced significantly. This patch won't do any harm, so in it goes.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/caa7c9cff348
Restore the menulist binding after its removal in bug 1518932. r=mkmelin

Comment 12

[Jorg K (CEST = GMT+2)]

OK, looking at the current test failures, at least these are related to the menu lists:
mozmill/folder-widget/test-message-filters.js

Comment 13

[:aceman]

Attachment: 1523384-XBLonCE.patch

This should fix mozmill/folder-widget/test-message-filters.js where the menulists were broken.
I learned a new thing:
We need to call customElements.upgrade(element) if we have an element that is a custom element inside our XBL binding. Then the CE is constructed first and then our code runs. Otherwise it is the other way round and our code can't find any of the methods of the custom element.

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=1470242 .
m-c decided to not fix this automatically and so we need to call it explicitly. It may become unneeded when converting everything to custom elements.

This call may be helpful in the preferences fixes or other places where we had these races of XBL and CE.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f6a62eca5bbe09cbe54d014252f2f59d35ec35b2

Comment 14

[:aceman]

Please fix the eslint problem 'mail/base/content/mailWidgets.xml:1461:15 | 'item' is not defined. (no-undef)' when landing the patch :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b624d188d799
Fix menulists from custom elements inside our XBL bindings; r=darktrojan
https://hg.mozilla.org/comm-central/rev/efe6ff00fe4c
Replace editable menulist with xbl-menulist in account creation wizard; rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/d6ef1a8900ea
Fix the broken To/Cc/Bcc selection in the compose window; rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/67058ffbf1ad
Replace editable menulist with xbl-menulist in calendar datepickers; rs=bustage-fix

Comment 16

[Geoff Lankow (:darktrojan) [PTO, back 28/4]]

Filed those two bugs to fix up the menulists I've just butchered.

Comment 17

[Jorg K (CEST = GMT+2)]

I think we're done here. I'll file a bug for the removal of xbl-menulist.

Comment 18

[:aceman]

Guys, has anybody tested this in the account wizard? It seems totally not working for me (linux).
Also, renaming the element to xbl-element looses all css as the rules still reference menulist (in m-c) and menulist[editable="true"] (in c-c) and not xbl-menulist[editable="true"].

I'll look at it.

Comment 19

[Richard Marti (:Paenglab)]

I'm adding the styles for xbl-menulists in bug 1524457.

Comment 20

[:aceman]

But why is it needed? Why can't we keep <menulist> and get the m-c styles?

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #18)

Guys, has anybody tested this in the account wizard? It seems totally not working for me (linux).

Why test it when tests are green ;-) - But seriously, we can't test every of the 20.001 functions manually.

Comment 22

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #20)

But why is it needed? Why can't we keep <menulist> and get the m-c styles?

Look at the msgIdentity in composer and switch to edit mode. Then you see that both bindings are active. With my patch in bug 1524457 the CE part is hidden. This is only a workaround until editable menulists are moved to CE.

Comment 23

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #20)

But why is it needed? Why can't we keep <menulist> and get the m-c styles?

Good question, this landed as bustage-fix, you'd have to ask Geoff.

Comment 24

[:aceman]

(In reply to Jorg K (GMT+1) from comment #21)

But seriously, we can't test every of the 20.001 functions manually.

Sure, but there could be some expectation to test the widgets that are specifically touched by a patch :)
Otherwise why only emailWizard was changed and not composer identity picker and also not the editable menulists in editor?

Comment 25

[Geoff Lankow (:darktrojan) [PTO, back 28/4]]

I did test the widgets and I know they are broken, that's why we have bug 1524457, which Richard is now fixing. Probably the ones in editor need to be xbl-menulist too, but since they have no tests I didn't notice them at the time.

Comment 26

[Richard Marti (:Paenglab)]

With xbl-menulists the menulist-popups don't work.

Comment 27

[Geoff Lankow (:darktrojan) [PTO, back 28/4]]

Attachment: 1523384-more-xbl-menulists-1.diff

Changes the editor menulists to xbl-menulist. At least they're all broken the same way now.

Comment 28

[Jorg K (CEST = GMT+2)]

Meaning that popups won't work? Advanced edit shows the content twice, but at least something works.

Comment 29

[:aceman]

And why isn't the #msgIdentity a xbl-menulist? Yes, it isn't an editable menulist initially, but do we really want it to switch from CE to XBL menulist when it gets the 'editable' attribute?

Anyway, to fix this properly we would have to copy all css rules in m-c for 'menulist' to 'xbl-menulist'.
Maybe should just do bug 1524457 before introducing that css fork.

Comment 30

[:aceman]

Attachment: 1523384.patch init CEs

Anyway, I think we need this in our binding, as the <moz-input-box> and <dropmarker> are custom elements now.

Comment 31

[Jorg K (CEST = GMT+2)]

What's the benefit? Does it make anything work?

Comment 32

[Jorg K (CEST = GMT+2)]

Why is attachment #9040848 [details] [diff] [review] worth having? Doesn't it turn "ugly" into "non-working"?

Comment 33

[Geoff Lankow (:darktrojan) [PTO, back 28/4]]

(In reply to Jorg K (GMT+1) from comment #32)

Why is attachment #9040848 [details] [diff] [review] worth having? Doesn't it turn "ugly" into "non-working"?

I had been under the impression that making CE menulist editable was both ugly and non-working - but apparently that wasn't the case. I made this patch so that when I fixed the non-working XBL menulist (which I intended to do, and in fact have a patch for), it would fix all of them at once. But that's not needed.

Comment 34

[:aceman]

(In reply to Jorg K (GMT+1) from comment #31)

What's the benefit? Does it make anything work?

It should be more correct even without fixing any immediate bug. You can't access properties of a CE before it is initialized (upgraded). A XBL binding extending a CE may want to access its properties, as we have seen in other bugs.

Comment 35

[:aceman]

Opening the Advanced pane in Preferences produces:
ReferenceError: reference to undefined property "mSelectedInternal" in menulist.js:253:5

Comment 36

[Richard Marti (:Paenglab)]

In composer, the new "To:" menulist in the next row gets double content after entering an address and press enter.

Comment 37

[Geoff Lankow (:darktrojan) [PTO, back 28/4]]

Comment on attachment 9040887 [details] [diff] [review]
1523384.patch init CEs

This is going to be unnecessary because of bug 1524457.

Comment 38

[:aceman]

Attachment: 1523384-fix.patch

Magnus noticed the "searchvalue" now has 2 constructors after my patch. My constructor on top actually didn't run so it didn't show my code was bad. Kill the surplus constructor and move the customElements.upgrade() after we fill in the menuitem labels otherwise the menulist ends up with blank items.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 9050154 [details] [diff] [review]
1523384-fix.patch

I'm *trying* to keep away from this stuff ;-) - But thanks.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/706bbea02e7d
fix double constructor in "searchvalue" binding. r=darktrojan