Editable menulists are busted on account of bug 1518932. This mainly affects the account creation wizard but there's some elsewhere. I think they need a complete rethink without XBL. Is it possible to extend the menulist custom element?

Something for the de-XBL team.

Hmm, editable menu lists broken and no test failures, that doesn't add up, see bug 1486051 comment #0. IIRC they are also used in the editable From: field which is covered by a test (test-newmsg-compose-identity.js).

Yes it's very possible to extend the menulist CE now after bug 1518932. Let's just make this bug about converting menulist-editable.

https://searchfox.org/comm-central/source/common/bindings/menulist.xml#71

Well, something is working. Customising the From: address looks very weird, but you can actually edit it, so that's most likely why the test isn't failing.

Or maybe just some CSS issue? Richard?

This fixes the weird "From:" menulist in composer when in editable mode and makes look the xbl-menulists like normal menulists.

On playing with the msgIdentity, I found that with xbl-menulists the menulist-popups don't work. This is probably why the datetime picker don't show in the event dialog.

Added the xbl- to the selector in mail/themes/windows/mail/menulist.css

This makes the custom elements menulist look right, and therefore the xbl-menulist name is no longer needed. Unfortunately I can't now figure out why the tests had problems with the CE menulist, so maybe this whole thing was just a waste of time. :(

Ugh, and I've just worked out the changes you made to #msgIdentity are just a more-specific version of what I did here, but I left them behind.

And what about switching more stuff to xbl-menulist in attachment #9040848 [details] [diff] [review] over in bug 1523384?

(In reply to :aceman from comment #12)

Comment on attachment 9040961 [details] [diff] [review]
1524457-fix-ce-menulist-1.diff

Review of attachment 9040961 [details] [diff] [review]:

::: common/bindings/menulist.css
@@ -6,5 @@

@namespace html url("http://www.w3.org/1999/xhtml"); /* namespace for HTML elements */

-xbl-menulist {

• -moz-binding: url("chrome://messenger/content/menulist.xml#xbl-menulist");

I think this was right.
Are you sure the following menulist[editable="true"] rule with our XBL
binding will override the CE defined for <menulist> ? It didn't when I
played with this.

No, I'm sure it won't. The content of our binding is added to the content added by CE. In fact we could probably remove large chunks of the binding(s) implementation because it's in the CE. But I'm not going to try that now, I've made enough mistakes this week.

::: common/bindings/menulist.xml
@@ -345,5 @@

     <html:input class="menulist-editable-input" anonid="input" allowevents="true"
                 xbl:inherits="value=label,value,disabled,tabindex,readonly,placeholder"/>
   </xul:moz-input-box>

•  <xul:dropmarker class="menulist-dropmarker" type="menu"
  

•                  xbl:inherits="open,disabled,parentfocused=focused"/>
  

Why removing this? The CE menulist has a dropmarker child.

Exactly, we shouldn't add another.

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

And what about switching more stuff to xbl-menulist in attachment #9040848 [details] [diff] [review] over in bug 1523384?

I can only update one bug at once!

Use tabbed browsing? ;-) - Actually, I had asked about the usefulness of that thing before.

I've converted the editable menulist binding to a custom element. There's still a few rough edges and I think it's probably easiest if I also convert the datepicker at the same time.

This breaks the date/time pickers and mozmill test composition/test-focus.js. The menulist can't be focused when not editable, because of something to do with containing an <input>. I think we should disable the test and fix that in a separate bug.

I mentioned that in comment 22, but I think I've now figured out how to fix it.

I'm sure there's still styling issues on Mac and Windows, but the focus should be a lot better.

This fixes the editable menulists as much as possible. On Linux and Mac they look like before. Under Windows they are a little bit different (the border colour is different and no focus ring when focused) but I can't do more.

In composer, the editable "From" menulist works again like before, also with the dark theme.

Jörg, can you test the Windows part and Geoff, can you do the Linux part? This patch needs the one from Geoff to work.

I found two things with the identity box I hadn't noticed before (they probably existed, I just didn't notice):

• When editing the box is 0.6667px taller than when not editing, which gets rounded and everything below moves down a pixel.
• The dropmarker isn't clickable when editing.

I also noticed the unstyled menulist is 1px taller when editable. I'm sure there's a spare pixel of padding we can remove.

I've made a modification so that the input box is selected if the user clicks on something in the dropdown which causes it to close.

We'll try again then.

(In reply to Geoff Lankow (:darktrojan) from comment #28)

I found two things with the identity box I hadn't noticed before (they probably existed, I just didn't notice):

• When editing the box is 0.6667px taller than when not editing, which gets rounded and everything below moves down a pixel.
• The dropmarker isn't clickable when editing.

I also noticed the unstyled menulist is 1px taller when editable. I'm sure there's a spare pixel of padding we can remove.

Both should be fixed with this patch.

(In reply to Geoff Lankow (:darktrojan) from comment #34)

Comment on attachment 9042641 [details] [diff] [review]
1524457-editable-menulist-css.patch

Review of attachment 9042641 [details] [diff] [review]:

Actually, the unstyled menulist is still 1px taller when editable. I think
it was like that before everything changed last week, but now seems a good
time to fix it.

With my theme they have the same height.

New patch to revert the change in emailWizard.xul (removal of the align="center").

I've made the down arrow key open the popup, since that seems to be a useful thing.

Just a general warning:
Careful here not to break anything. There are many different cases/setups, different states, and the event handlers are very fragile. It took a lot of time to get this work in all cases.

Oh, and I also got some

JavaScript error: chrome://messenger/content/accountcreation/emailWizard.js, line 1456: TypeError: menulist.inputField is undefined

... but not sure exactly when

(In reply to Magnus Melin [:mkmelin] from comment #39)

•  Object.defineProperty(this, "fragment", { value: frag });
  

What is this for?
At any rate, it looks wrong. You shouldn't use "this" in a static method
(and I'm surprised that's even allowed at all).

I nicked it from people who should know what they're doing. (I'm not convinced about that though.)

can we please keep both id and is on the same row, for easier grepping (I think id first)

NIT: My preference would be to put is= first, because it's a type and a specialization of the <menulist> element, could just as well be part of the tag name, and should therefore be closest to the <menulist> tag in the code.

(In reply to Magnus Melin [:mkmelin] from comment #40)

Oh, and I also got some
JavaScript error: chrome://messenger/content/accountcreation/emailWizard.js, line 1456: TypeError: menulist.inputField is undefined

I have also seen that in comment 20.

(In reply to Magnus Melin [:mkmelin] from comment #39)

• <script type="application/javascript" src="chrome://messenger/content/customElements.js"/>

I'd assume this file needs the css too.
It's basically dead code though.

Already has it.

::: mail/base/content/mailWidgets.js

•      if (event.key == "ArrowDown") {
  

KeyEvent.DOM_VK_DOWN

That has been deprecated for years.

•  Object.defineProperty(this, "fragment", { value: frag });
  

What is this for?
At any rate, it looks wrong. You shouldn't use "this" in a static method
(and I'm surprised that's even allowed at all).

Killed it.

• customElements.define("editable-menulist", MozEditableMenulist, { extends: "menulist" });

I think we should do these the other way around: menulist-editable. Like it
was in xbl too.
That way you can understand that it's a menulist, with an editable variant

Okay.

::: mail/components/accountcreation/content/emailWizard.xul

•        <menulist is="editable-menulist"
  

•                  id="incoming_port"
  

can we please keep both id and is on the same row, for easier grepping (I
think id first). here and elsewhere

Yes, but I agree with Ben, is first.

::: mail/themes/osx/mail/menulist.css

+menulist[editable="true"] {

for all of these menulist[editable="true"] we may want, for clarity have

menulist[is="menulist-editable"][editable="true"]

Okay.

(In reply to Magnus Melin [:mkmelin] from comment #46)

Looks like this should also have a attributeChangedCallback() where it would
call super + the own _updateAttributes()

also add a
static get observedAttributes()
... to do super + own attribute checking

I don't think this is necessary because we don't want to listen to any more attributes than we already do. I think I should move the this.inheritAttribute(this.inputField, "value"); line into the constructor though, as that's the only point at which it should happen.

If you don't observer, you can't handle it if someone does ml.setAttribute("editable", "true");
Same for value and disabled, which would lead to subtle bugs.

It looks like bug 1527680 can save me a whole heap of hassle here.

Updated CSS patch.

Wrong patch. :-(

(In reply to Richard Marti (:Paenglab) from comment #54)

Why to you add in the CSS files the [is="menulist-editable"] together with
the [editable="true"]? Isn't this a pleonasm?

No, the menulist-editable menulist can be editable="false" too, when not editing

(In reply to Magnus Melin [:mkmelin] from comment #57)

(In reply to Richard Marti (:Paenglab) from comment #54)

Why to you add in the CSS files the [is="menulist-editable"] together with
the [editable="true"]? Isn't this a pleonasm?

No, the menulist-editable menulist can be editable="false" too, when not editing

But a normal menulist has never editable="true" and thus the [is="menulist-editable"] isn't really needed.

Now all needed selectors should have added the [is="menulist-editable"], so it's okay how it is now.

(In reply to Richard Marti (:Paenglab) from comment #58)

But a normal menulist has never editable="true" and thus the
[is="menulist-editable"] isn't really needed.

Yes, strictly not. Just for clarity.

(In reply to Magnus Melin [:mkmelin] from comment #60)

Comment on attachment 9044769 [details] [diff] [review]
1524457-menulist-customelement-8.diff

Review of attachment 9044769 [details] [diff] [review]:

The advanced editor seems to be missing the needed styles, so the menu looks
a bit funny.

With my patch it should look normal. I also added the [is="menulist-editable"].

I also wondered about the is= when we used editable=true till now. But if it is to distinguish element that should get our CE even when they do not have editable=true yet (but can get it via JS later), then that looks reasonable.

I found some bugs affecting the "add new attribute" menulist in the advanced dialog.

More minor changes. I have a Try run in progress, and if when it passes, I am going to ship this thing.

For me it looks like the new menulist (CE) does not update its displayed value when something is selected from the drop down.

I observe that effect for example at the "port" menulist of the email account creation dialog of daily 67.0a1 (2019-03-05) (64-bit).

Yeah I see that. It works up until the point you enter something in the textbox, and then it stops working. Weird.