Closed Bug 1523384 Opened 3 years ago Closed 3 years ago

Restore the menulist binding after its removal in bug 1518932

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 67.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(4 files, 1 obsolete file)

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

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

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.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9039618 - Flags: review?(mkmelin+mozilla)
Attachment #9039618 - Flags: review?(jorgk)

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

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

Attachment #9039618 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9039618 [details] [diff] [review]
1523384-menulist-binding.patch

OK, first reviewer wins ;-(
Attachment #9039618 - Flags: review?(jorgk)

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.

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.

(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.

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.

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.

Keywords: leave-open
Target Milestone: --- → Thunderbird 67.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/caa7c9cff348
Restore the menulist binding after its removal in bug 1518932. r=mkmelin

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

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

Attachment #9040567 - Flags: review?(geoff)

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

Attachment #9040567 - Flags: review?(geoff) → review+
Pushed by geoff@darktrojan.net:
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
Depends on: 1524456
Depends on: 1524457

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

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1524497

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.

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

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

(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.

(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.

(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.

(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?

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.

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

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

Attachment #9040848 - Flags: review?(acelists)

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

Attachment #9040848 - Flags: review?(acelists) → review+

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.

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

Attachment #9040887 - Flags: review?(geoff)

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

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

Attachment #9040848 - Attachment is obsolete: true

(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.

(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.

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

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

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

This is going to be unnecessary because of bug 1524457.
Attachment #9040887 - Flags: review?(geoff)
Depends on: 1527535

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.

Attachment #9050154 - Flags: review?(jorgk)
Comment on attachment 9050154 [details] [diff] [review]
1523384-fix.patch

I'm *trying* to keep away from this stuff ;-) - But thanks.
Attachment #9050154 - Flags: review?(jorgk) → review?(geoff)
Blocks: 1534345
Attachment #9050154 - Flags: review?(geoff) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/706bbea02e7d
fix double constructor in "searchvalue" binding. r=darktrojan
Regressions: 1551081
You need to log in before you can comment on or make changes to this bug.