1524457 - [de-xbl] convert menulist-editable to custom element (and switch emailWizard.xul xbl-menulists to this new element)

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Geoff Lankow (:darktrojan)]

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?

Comment 1

[Jorg K (CEST = GMT+2)]

Something for the de-XBL team.

Comment 2

[Jorg K (CEST = GMT+2)]

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

Comment 3

[Magnus Melin [:mkmelin]]

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

Comment 4

[Magnus Melin [:mkmelin]]

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

Comment 5

[Jorg K (CEST = GMT+2)]

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?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1524457-fix-xbl-menulist-and-msgIdentity.patch

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.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040817 [details] [diff] [review]
1524457-fix-xbl-menulist-and-msgIdentity.patch

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

::: mail/themes/windows/mail/menulist.css
@@ +21,5 @@
> +}
> +
> +/* ..... disabled state ..... */
> +
> +menulist[disabled="true"] {

xbl missing here?

Comment 8

[Richard Marti (:Paenglab)]

Attachment: 1524457-fix-xbl-menulist-and-msgIdentity.patch

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

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9040827 [details] [diff] [review]
1524457-fix-xbl-menulist-and-msgIdentity.patch

Editable From: works now. Account creation has non-functioning SMTP server popup (as previously noted).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8b066e0a55ab
Fix the CSS of xbl-menulists and the msgIdentity when it is editable. r=jorgk

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-fix-ce-menulist-1.diff

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

Comment 12

[:aceman]

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.

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

Comment 13

[Geoff Lankow (:darktrojan)]

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.

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Geoff Lankow (:darktrojan)]

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

Comment 16

[Geoff Lankow (:darktrojan)]

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

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Geoff Lankow (:darktrojan)]

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

Oh and I've still got xbl-menulists in calendar. I'll have another look at this whole thing tomorrow.

Comment 19

[Richard Marti (:Paenglab)]

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

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

This works for me on macOS and Windows. I suppose you have tested it on Linux. ;-)

With removing of the xbl-menulist binding, the menulists in Calendar's event dialog are broken. So I think you should convert them now to normal <menulist> too.

On Windows there is now no dropmarker on the editable From menulist. This needs that you remove the selector at https://hg.mozilla.org/comm-central/rev/8b066e0a55ab#l6.15.

::: common/bindings/menulist.css
@@ +10,5 @@
>    -moz-binding: url("chrome://messenger/content/menulist.xml#menulist-editable");
>    -moz-user-focus: ignore;
>  }
>  
> +menulist:not([editable="true"]) .menulist-editable-box,

Not sure this line is needed as on normal menulists no .menulist-editable-box exists.

Comment 20

[:aceman]

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

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

I feel uneasy about the mix of CE and XBL and even the switch from XBL to CE (when editable=true is removed in account wizard).

Also, this re-introduces the account wizard problem again:
ReferenceError: reference to undefined property "inputField" in emailWizard.js:1118:1

Comment 21

[Geoff Lankow (:darktrojan)]

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.

Comment 22

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-1.diff

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.

Comment 23

[Richard Marti (:Paenglab)]

Comment on attachment 9041360 [details] [diff] [review]
1524457-menulist-customelement-1.diff

This needs some styling to look like before but this can be done in a other patch.

One issue I encounter is, that the normal menulist is no more focusable through tabbing and when in editable="true" mode the focus is always in the textbox and not in the menulist. To test it try to tab in the composer window. ui-r- because of this.

Comment 24

[Geoff Lankow (:darktrojan)]

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

Comment 25

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-2.diff

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

Comment 26

[Richard Marti (:Paenglab)]

Comment on attachment 9041938 [details] [diff] [review]
1524457-menulist-customelement-2.diff

Yes, it needs some CSS tweaks, but works now.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: 1524457-editable-menulist-css.patch

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.

Comment 28

[Geoff Lankow (:darktrojan)]

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.

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-3.diff

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.

Comment 30

[Richard Marti (:Paenglab)]

Comment on attachment 9042292 [details] [diff] [review]
1524457-menulist-customelement-3.diff

The patch does not apply on mail/base/content/mailWidgets.js because of the landing of bug 1521016.

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-4.diff

We'll try again then.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: 1524457-editable-menulist-css.patch

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

Comment 33

[Geoff Lankow (:darktrojan)]

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

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

That's better, thanks.

Comment 34

[Geoff Lankow (:darktrojan)]

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.

Comment 35

[Geoff Lankow (:darktrojan)]

Attachment: editable menulist is taller than non-editable

Comment 36

[Richard Marti (:Paenglab)]

Attachment: 1524457-editable-menulist-css.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").

Comment 37

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-5.diff

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

Comment 38

[Ben Bucksch (:BenB)]

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.

Comment 39

[Magnus Melin [:mkmelin]]

Comment on attachment 9042871 [details] [diff] [review]
1524457-menulist-customelement-5.diff

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

::: editor/ui/dialogs/content/EditorPublish.xul
@@ +25,5 @@
>    <script type="application/javascript" src="chrome://editor/content/editorUtilities.js"/>
>    <script type="application/javascript" src="chrome://editor/content/EdDialogCommon.js"/>
>    <script type="application/javascript" src="chrome://editor/content/EditorPublish.js"/>
>    <script type="application/javascript" src="chrome://editor/content/publishprefs.js"/>
> +  <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.

::: mail/base/content/mailWidgets.js
@@ +504,5 @@
> +      this.mAttributeObserver = null;
> +      this.setInitialSelection();
> +
> +      this._inputBox.addEventListener("keypress", (event) => {
> +          if (event.key == "ArrowDown") {

KeyEvent.DOM_VK_DOWN

@@ +539,5 @@
> +          </hbox>
> +          <dropmarker class="menulist-dropmarker" type="menu" role="none"/>
> +        `), true);
> +
> +      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).

@@ +607,5 @@
> +  const MenuBaseControl = BaseControlMixin(MozXULMenuElement);
> +  MenuBaseControl.implementCustomInterface(MozEditableMenulist, [Ci.nsIDOMXULMenuListElement,
> +                                                                 Ci.nsIDOMXULSelectControlElement]);
> +
> +  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

@@ +649,5 @@
> +          </hbox>
> +          <dropmarker class="menulist-dropmarker" type="menu" role="none"/>
> +        `), true);
> +
> +      Object.defineProperty(this, "fragment", { value: frag });

here too

::: mail/components/accountcreation/content/emailWizard.xul
@@ +317,5 @@
>              <textbox id="incoming_hostname"
>                       oninput="gEmailConfigWizard.onInputHostname();"
>                       class="host uri-element"/>
> +            <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

::: mail/themes/osx/mail/menulist.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +menulist[editable="true"] {

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

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

Comment 40

[Magnus Melin [:mkmelin]]

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

Comment 41

[Geoff Lankow (:darktrojan)]

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

Comment 42

[Ben Bucksch (:BenB)]

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.

Comment 43

[:aceman]

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

Comment 44

[Geoff Lankow (:darktrojan)]

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

Comment 45

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-6.diff

Comment 46

[Magnus Melin [:mkmelin]]

Comment on attachment 9044077 [details] [diff] [review]
1524457-menulist-customelement-6.diff

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

::: mail/base/content/mailWidgets.js
@@ +486,5 @@
>  customElements.define("mail-emailaddress", MozMailEmailaddress);
>  customElements.define("mail-emailheaderfield", MozMailEmailheaderfield);
> +
> +customElements.whenDefined("menulist").then(() => {
> +  class MozEditableMenulist extends customElements.get("menulist") {

s/MozEditableMenulist/MozMenulistEditable/

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

@@ +606,5 @@
> +                                                                 Ci.nsIDOMXULSelectControlElement]);
> +
> +  customElements.define("menulist-editable", MozEditableMenulist, { extends: "menulist" });
> +
> +  class MozDescriptionMenulist extends MozEditableMenulist {

s/MozDescriptionMenulist/MozMenulistDescription/

Comment 47

[Geoff Lankow (:darktrojan)]

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

Comment 48

[Magnus Melin [:mkmelin]]

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.

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 9044077 [details] [diff] [review]
1524457-menulist-customelement-6.diff

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

::: mail/base/content/mailWidgets.js
@@ +495,5 @@
> +
> +      this.prepend(MozEditableMenulist.fragment.cloneNode(true));
> +      this.inputField = this.children[0];
> +      this._labelBox = this.children[1];
> +      this._dropmarker = this.children[2];

it would be way more readable and less error prone to just do this instead:

this.inputField = this.querySelector(".menulist-input");
this._labelBox = this.querySelector(".menulist-label-box");
this._dropmarker = this.querySelector("menulist-dropmarker");

@@ +507,5 @@
> +      this.inputField.addEventListener("keypress", (event) => {
> +          if (event.key == "ArrowDown") {
> +              this.open = true;
> +          }
> +      });

wrong indentions

@@ +602,5 @@
> +
> +  const MozXULMenuElement = MozElementMixin(XULMenuElement);
> +  const MenuBaseControl = BaseControlMixin(MozXULMenuElement);
> +  MenuBaseControl.implementCustomInterface(MozEditableMenulist, [Ci.nsIDOMXULMenuListElement,
> +                                                                 Ci.nsIDOMXULSelectControlElement]);

I think the general way we tend to to format these is

MenuBaseControl.implementCustomInterface(
  MozEditableMenulist, [Ci.nsIDOMXULMenuListElement, Ci.nsIDOMXULSelectControlElement]
);

@@ +612,5 @@
> +      if (this.delayConnectedCallback()) {
> +        return;
> +      }
> +
> +      this.prepend(MozDescriptionMenulist.fragment.cloneNode(true));

since description is all that's different, perhaps it would be better to do

let description = ...
this.querySelector("."menulist-label").after(decription);

@@ +654,5 @@
> +    }
> +
> +    get selectedItem() {
> +      return super.selectedItem;
> +    }

not needed

Comment 50

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-7.diff

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

Comment 51

[Jorg K (CEST = GMT+2)]

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

Sorry, this has been hanging in my queue, but it needs the JS part first, right? Please re-request.

Comment 52

[Magnus Melin [:mkmelin]]

Comment on attachment 9044577 [details] [diff] [review]
1524457-menulist-customelement-7.diff

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

When opening compose, I get
JavaScript error: chrome://chat/content/conversation-browser.js, line 1: SyntaxError: redeclaration of let MozConversationBrowser 

I don't think you handle setting the "editable" attribute atm.


Besides these, seems to be working

::: mail/base/content/mailWidgets.js
@@ +486,5 @@
>  customElements.define("mail-emailaddress", MozMailEmailaddress);
>  customElements.define("mail-emailheaderfield", MozMailEmailheaderfield);
> +
> +customElements.whenDefined("menulist").then(() => {
> +  class MozMenulistEditable extends customElements.get("menulist") {

Let's add some documentation too like

/**
 * MozMenulistEditable is a menulist widget that can be made editable by setting editable="true".
 * With an additional type="description" the list also contains an additional label that can hold
 * for instance, a description of a menu item.
 * It is typically used e.g. for the "Custom From Address..." feature to let the user chose and edit
 * the address to send from. 
 * @extends {MozMenuList}
 */

@@ +554,5 @@
> +      return {
> +        ".menulist-icon": "src=image",
> +        ".menulist-label": "value=label,crop,accesskey,highlightable",
> +        ".menulist-highlightable-label": "text=label,crop,accesskey,highlightable",
> +        ".menulist-dropmarker": "disabled,open",

get these 4 from the superclass, and add our own after?

Comment 53

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-8.diff

Comment 54

[Richard Marti (:Paenglab)]

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

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

Comment 55

[Richard Marti (:Paenglab)]

Attachment: 1524457-menulist-customelement-8.diff

Updated CSS patch.

Comment 56

[Richard Marti (:Paenglab)]

Attachment: 1524457-editable-menulist-css.patch

Wrong patch. :-(

Comment 57

[Magnus Melin [:mkmelin]]

(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

Comment 58

[Richard Marti (:Paenglab)]

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

Comment 59

[Magnus Melin [:mkmelin]]

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

Comment 60

[Magnus Melin [:mkmelin]]

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

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

::: editor/ui/dialogs/content/EdAdvancedEdit.xul
@@ +31,5 @@
>  
>    <!-- global dialog functions -->
>    <script type="application/javascript" src="chrome://editor/content/EdAdvancedEdit.js"/>
>  
> +  <script type="application/javascript" src="chrome://messenger/content/customElements.js"/>

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

Comment 61

[Magnus Melin [:mkmelin]]

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

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

::: mail/themes/linux/mail/menulist.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +menulist[editable="true"] {

I'd still like the [is="menulist-editable"] too for clarity

Comment 62

[Richard Marti (:Paenglab)]

Attachment: 1524457-editable-menulist-css.patch

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

Comment 63

[:aceman]

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.

Comment 64

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-9.diff

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

Comment 65

[Geoff Lankow (:darktrojan)]

Attachment: 1524457-menulist-customelement-10.diff

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

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2cad3d30f52c
Convert menulist-editable to custom element; r=mkmelin
https://hg.mozilla.org/comm-central/rev/ed928f6d6a69
Fix the editable menulist CSS. r=darktrojan, jorgk

Comment 68

[John Bieling (:TbSync)]

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

Comment 69

[Geoff Lankow (:darktrojan)]

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