[de-xbl] convert charsetpicker binding to customized built-in based on menulist

RESOLVED FIXED in Thunderbird 67.0

Status

defect
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: mkmelin, Assigned: mkmelin)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 67.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 months ago

+++ This bug was initially created as a clone of Bug #1524497 +++

charsetpicker should likely be converted to <menulist is="charset-picker">

https://searchfox.org/comm-central/source/mailnews/base/content/charsetList.xml#13

Assignee

Updated

5 months ago
Summary: [de-xbl] convert charsetpicker binding to customized built-in based on menulit → [de-xbl] convert charsetpicker binding to customized built-in based on menulist
Assignee

Comment 1

3 months ago

Make it a custom element. The initial selection doesn't show (but opening it you can see it's correct). This is an other bug also seen in the calendar dialogs for selecting calendars.

I fixed some boilerplates since they were not formatted correctly for whatever reason.

Off to try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=847783718185e02b912719de84bbfb82e10fc722

Attachment #9051226 - Flags: review?(paul)
Assignee

Updated

3 months ago
Status: NEW → ASSIGNED
Assignee

Comment 2

3 months ago

Try looks good.

Comment on attachment 9051226 [details] [diff] [review]
bug1524508_charsetpicker.patch

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

Looks good.  I had a few suggestions, so r+ from me with those addressed.  I haven't yet applied the patch and manually tested it, but wanted to go ahead and post my code review.  (Is manual testing needed if the try run was good?  Just clarifying expectations for reviewers / reviewing.)

::: mailnews/base/content/menulist-charsetpicker.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
> +
> +customElements.whenDefined("menulist").then(() => {
> +  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

This could be const or if not const then let instead of var.

@@ +20,5 @@
>  
> +    connectedCallback() {
> +      super.connectedCallback();
> +      if (this.delayConnectedCallback()) {
> +        return;

Do we need the delayConnectedCallback check in this case?

@@ +56,5 @@
> +        let strCharset = charsetBundle.GetStringFromName(
> +          item.toLowerCase() + ".title");
> +
> +        menuLabels.push({ label: strCharset, value: item });
> +      });

We could use map instead of forEach here:

let menuLabels = charsetValues.map((item) => {
  ...
  return { label: strCharset, value: item };
});

@@ +100,5 @@
> +});
> +
> +// The menulist CE is defined lazily. Create one now to get menulist defined,
> +// allowing us to inherit from it.
> +delete document.createElement("menulist");

Hmm, what about wrapping this in a check to see if it is already defined?
Attachment #9051226 - Flags: review?(paul) → review+
Assignee

Comment 4

3 months ago

Re reviewing and testing things out: judgement call - if it's easy to test, usually worth doing.

Thx, regarding delayConnectedCallback(): probably not need, but still preferable. See bug 1495946 comment 22.

Assignee

Comment 5

3 months ago
Attachment #9051226 - Attachment is obsolete: true
Attachment #9051530 - Flags: review+
Assignee

Updated

3 months ago
Keywords: checkin-needed

Comment 6

3 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/86cdbc8a3cef
[de-xbl] convert charsetpicker binding to customized built-in based on menulist. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

3 months ago
Target Milestone: --- → Thunderbird 67.0
Depends on: 1535955
You need to log in before you can comment on or make changes to this bug.