1531296 - [de-xbl] convert task-progress-menupopup, task-priority-menupopup and task-menupopup to custom element

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Magnus Melin [:mkmelin]]

Get rid of task-progress-menupopup, task-priority-menupopup and task-menupopup bindings.

https://searchfox.org/comm-central/search?q=task-menupopup&path=

I'd imagine you want to have the task-menupopup base class which you'd not expose in window.customElements, but only use for the child classes.

Please name them something like calendar-task-progress-menupopup, calendar-task-priority-menupopup.

This would be similar to bug 1528201.

Comment 1

[Paul Morris [:pmorris]]

I've run into an issue using the approach from bug 1528201. The task menus have accesskeys and they aren't working. I suspect that making the custom element a child of 'menupopup' is the problem. Some options:

1. work on getting accesskeys to work using this same "child of menupopup" approach (via event listeners, etc.)
2. go ahead and convert menupopup to a custom element and inherit from that

I'm going to try #2, since that seems like the more robust long-term solution. The relevant 'popup' binding looks not so large or complex.

Comment 2

[Paul Morris [:pmorris]]

Converting menupopup to a custom element worked, and that includes the accesskeys. So that's the way forward. Now to check with Firefox devs about plans for de-xbl-ing the menupopup element, like if it is just a simple 1-to-1 conversion or something else.

Comment 3

[Magnus Melin [:mkmelin]]

Ok, file a bug and ask :bgrins what he thinks.

Comment 4

[Brian Grinstead [:bgrins]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1397874#c9 for menupopup discussion. The easiest thing might be to file a bug to land the https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Popup.js implementation as MozElements.Popup or similar, but not customElements.define("menupoup") yet.

That way you can use it on inherited bindings and we could separately tackle the places implementations underneath at https://bgrins.github.io/xbl-analysis/tree/#popup.

Then for your bindings you could do something like:

menupopup[is="task-progress"] { -moz-binding: none; }

customElements.define("task-progress", class TaskProgress extends MozElements.Popup {
 
});

<menupopup is="task-progress">

Does that make sense?

Comment 5

[Paul Morris [:pmorris]]

Thanks Brian, yeah, that makes sense. I'll file a bug to convert the popup binding, but without doing customElements.define("menupoup") yet. Later we'd just remove the

menupopup[is="task-progress"] { -moz-binding: none; }

from Thunderbird code after menupopup was defined as a custom element.

Comment 6

[Paul Morris [:pmorris]]

Attachment: task-menupopups-0.patch

The calendar mozmill tests are all passing here locally. I manually tested the priority and progress popup menus by making the tasks tab active and then checking in:

• the "Events and Tasks" menu
• the context menus (when right-clicking on a task in tasks list and today pane)
• the toolbar buttons at the top of the task detail panel in the tasks tab (when a tab is selected)

(A check mark is present to indicate the current value and selecting an item changes the current value. The accesskeys work as expected.)

The toolbar button instances are apparently connected, disconnected, and reconnected during startup. When I tried making connectedCallback only run once, the event listeners were never reconnected after the disconnection. So connectedCallback needs to be able to run more than once.

I did some refactoring of some code that was not easy to follow.

Comment 7

[Paul Morris [:pmorris]]

Attachment: task-menupopups-1.patch

Take two. I had overlooked some details on how extending MozElements.MozMenuPopup is supposed to work. Everything still works except for the accesskeys. (My hunch is that this is likely a problem that falls outside the scope of this patch.)

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Paul Morris [:pmorris] from comment #6)

The toolbar button instances are apparently connected, disconnected, and
reconnected during startup. When I tried making connectedCallback only run
once, the event listeners were never reconnected after the disconnection.

But why were the event listeners disconnected? Internal event listeners do not in general need to be disconnected in disconnectedCallback() AFAIK.

Comment 9

[Paul Morris [:pmorris]]

Ah, good to know. The automatic converter converted the XBL destructor into the disconnectedCallback containing code to remove the event listener. I'll remove that, and make connectedCallback only execute once.

Comment 10

[Paul Morris [:pmorris]]

Attachment: task-menupopups-2.patch

Dropped the disconnectedCallback removing the event listener, made the connectedCallback only run once, and a few other simplifications.

Comment 11

[Brian Grinstead [:bgrins]]

You can also use this.addEventListener in the constructor if that's easier for your case (this only works if the listener is added to the element itself and not to a child).

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9055032 [details] [diff] [review]
task-menupopups-1.patch

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

(had some unsubmitted comments in this older version of the patch)

::: calendar/base/content/calendar-menus.js
@@ +12,5 @@
> +     * For the given element, set an attribute on all of its child nodes and their commands.
> +     *
> +     * @param {Element} element       The parent node.
> +     * @param {string} attribute      The attribute to set.
> +     * @param {string|boolean} value  The value to set.

attribute values are always strings

this function is only used once though, so there is no need to have this complication. you can just inline the loop

@@ +74,5 @@
> +        if (!tasksSelected) {
> +            setAttributeOnElementChildren(parent, "disabled", false);
> +        }
> +
> +        if (propertyValue || propertyValue == 0) {

"" == 0 is true, so this looks like nonsense

@@ +149,1 @@
>  

fits in one line I think, especially for calendar which allows longer lines. 100ch was it?

::: calendar/base/content/calendar-task-tree.js
@@ +79,5 @@
>      document.getElementById("task-context-menu-separator-filter").hidden =
>          (idnode == "calendar-task-tree");
>  
>      let tasksSelected = (items.length > 0);
> +    setAttributeOnElementChildren(aEvent.target, "disabled", !tasksSelected);

I'm not too fond of unnecessary helper functions that complicate matters. You can remove setAttributeOnElementChildren and use something like

for (let child of aEvent.target.children) {
  child.setAttribute("disabled", !tasksSelected);
}

That's not *exactly* what the old code does, but it's what the function name applied, and what I think is the intention (setting the disabling for a context menu item would originally have had an odd side effect of disabling some commands globally. That seems unexpected, and exactly why I don't like unnecessary helpers.)

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9055523 [details] [diff] [review]
task-menupopups-2.patch

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

Please update for the comments, looks like there's more than one similar case to adjust, but I only commented on one occurrence now

::: calendar/base/content/calendar-menus.js
@@ +80,5 @@
> +            super.connectedCallback();
> +
> +            const scrollbox = this.querySelector(".popup-internal-box");
> +            scrollbox.appendChild(MozXULElement.parseXULToFragment(`
> +                <menuitem id="percent-0-menuitem"

you shouldn't use ids for these, that could cause problems

Comment 14

[Paul Morris [:pmorris]]

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

Comment on attachment 9055032 [details] [diff] [review]
task-menupopups-1.patch

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

(had some unsubmitted comments in this older version of the patch)

::: calendar/base/content/calendar-menus.js
@@ +12,5 @@

• * For the given element, set an attribute on all of its child nodes and their commands.
  

• *
  

• * @param {Element} element       The parent node.
  

• * @param {string} attribute      The attribute to set.
  

• * @param {string|boolean} value  The value to set.
  

attribute values are always strings

Yes, but this is documenting the parameters of the function, which we expect to be a boolean or a string. Boolean true/false is converted to a string after it is passed to the function, when it is set on the element.

this function is only used once though, so there is no need to have this
complication. you can just inline the loop

This particular function "setAttributeOnElementChildrenAndCommands" was removed in the next version (#2) of the patch. It contained pre-existing code that set both the command and the element itself to disabled=false. (I had moved that code out into its own function.) Setting both is unneeded because the disabled state of the command is also applied to the element itself. So in version #2 I replaced this function with the pre-existing function that sets a disabled state on either the command or the element (not both). So we no longer have a case of a function that is used only once.

(Even if it was only used once I still find code easier to understand if it is divided up into smaller functions like this, but we apparently have different views on that.)

@@ +74,5 @@

•    if (!tasksSelected) {
  

•        setAttributeOnElementChildren(parent, "disabled", false);
  

•    }
  

•    if (propertyValue || propertyValue == 0) {
  

"" == 0 is true, so this looks like nonsense

Ah, good point. (This was pre-existing code.) I'll replace it with propertyValue === 0 to prevent type coercion.

@@ +149,1 @@

fits in one line I think, especially for calendar which allows longer lines.
100ch was it?

This is actually longer than 100ch when you try to put it on one line. (Which is why I wrapped it.)

::: calendar/base/content/calendar-task-tree.js
@@ +79,5 @@

 document.getElementById("task-context-menu-separator-filter").hidden =
     (idnode == "calendar-task-tree");

 let tasksSelected = (items.length > 0);

• setAttributeOnElementChildren(aEvent.target, "disabled", !tasksSelected);

I'm not too fond of unnecessary helper functions that complicate matters.
You can remove setAttributeOnElementChildren and use something like

for (let child of aEvent.target.children) {
child.setAttribute("disabled", !tasksSelected);
}

That's not exactly what the old code does, but it's what the function name
applied, and what I think is the intention (setting the disabling for a
context menu item would originally have had an odd side effect of disabling
some commands globally. That seems unexpected, and exactly why I don't like
unnecessary helpers.)

Hmm, we seem to have different views on the benefits/drawbacks of helper functions. I find they make the code easier to understand by breaking it down into smaller pieces, and allowing for code re-use/D.R.Y., etc.

In this case I found we had code that was doing the same thing in two places, so I started using the function that was already defined in calendar-task-tree.js, to remove the code duplication in calendar-menus.js.

So you are proposing changing what the existing code does (so it only disables the elements and not the commands). That seems okay to me.

I actually was confused by the pre-existing code here. When no tasks were selected, but the popup is shown, it would set disabled to false. (Which doesn't actually seem to be possible, I don't think the popup can appear when no tasks are selected.) But that seemed backwards. Why enable these commands/elements when no tasks were selected? Surely the intent was to disable them when no tasks were selected, right? So I changed it to setting disabled to true.

But, as you point out, we probably don't want to disable the commands globally (should this popup ever be shown when no tasks are selected). So I think it does make sense to change the code in calendar-menus.js to not disable the commands, only the elements. (As a precautionary measure in case the popup is ever shown with no tasks selected.)

In that case it doesn't make sense to reuse the function from calendar-task-tree.js. And then we don't need to touch any code there, as that is out of scope for this bug.

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

Comment on attachment 9055523 [details] [diff] [review]
task-menupopups-2.patch

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

Please update for the comments, looks like there's more than one similar
case to adjust, but I only commented on one occurrence now

::: calendar/base/content/calendar-menus.js
@@ +80,5 @@

•        super.connectedCallback();
  

•        const scrollbox = this.querySelector(".popup-internal-box");
  

•        scrollbox.appendChild(MozXULElement.parseXULToFragment(`
  

•            <menuitem id="percent-0-menuitem"
  

you shouldn't use ids for these, that could cause problems

Ah, good point. I've removed the ids. They weren't being used for anything.

Patch on the way with changes.

Comment 15

[Paul Morris [:pmorris]]

Attachment: task-menupopups-3.patch

Okay, I've removed the code to disable the commands/elements if the popup is shown when there are no tasks selected. Currently there's no way for this to happen. If a task isn't selected the parent menu item is disabled, or the detail pane is not shown, so there's no way for the popup to be shown.

I don't think we'd ever want to show these popups if there wasn't a task selected. So if something breaks and that happens in the future, we'd want to fix the popup being shown rather than making the menuitems disabled. This simplifies the code.

I'm still using the helper function from calendar-task-tree.js to set checked to false on all the menuitems. I've changed the function's name to convey that it sets the attribute on the elements or their commands. It didn't make sense to me to repeat that code in two places.

(We could look at modifying the behavior of the code in calendar-task-tree.js but I'd rather do that in a follow-up bug instead of here, since it doesn't really impact converting these two menupopups.)

I'm still using the 'getPropertyValue' helper function, which is only used once, but I think it's clearer this way. (Also smaller functions like this are easier to unit test.) But if you insist then I can inline it.

I've made the other changes as described above.

Comment 16

[Paul Morris [:pmorris]]

Attachment: task-menupopups-4.patch

Forgot to update a test after removing the ids. Fixed in this patch, converted the ids to classes.

All mozmill calendar tests passed locally.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9055908 [details] [diff] [review]
task-menupopups-4.patch

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

Looks good to me, let's have Philipp have a look over still

::: calendar/base/content/calendar-menus.js
@@ +113,5 @@
> +            this.addEventListener(
> +                "popupshowing",
> +                updateMenuItemsState.bind(null, scrollbox, "percentComplete"),
> +                true
> +            );

slightly unusual formatting. perhaps just

this.addEventListener("popupshowing",
    updateMenuItemsState.bind(null, scrollbox, "percentComplete"), true);

Comment 18

[Paul Morris [:pmorris]]

Attachment: task-menupopups-5.patch

Okay, I've moved away from that style of formatting in a few places. Over to Philipp for review.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9056125 [details] [diff] [review]
task-menupopups-5.patch

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

::: calendar/base/content/calendar-menus.js
@@ +20,5 @@
> +    const getPropertyValue = (propertyKey, tasks) => {
> +        let propertyValue = null;
> +        const tasksSelected = (tasks != null) && (tasks.length > 0);
> +        if (tasksSelected) {
> +            if (tasks.every(task => task[propertyKey] == tasks[0][propertyKey])) {

Can we merge these if's? I think I enabled an eslint rule for this, but I may be mistaking.

Comment 20

[Paul Morris [:pmorris]]

Attachment: task-menupopups-6.patch

Thanks Philipp. I've merged those ifs. ESLint did not complain about them before, so sounds like a rule update is needed.

Here's the try server run:

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

Comment 21

[Paul Morris [:pmorris]]

Hmm, a quick search didn't turn up an eslint rule for double ifs like this. Only one for similar cases in an else branch:
https://eslint.org/docs/rules/no-lonely-if

Comment 22

[Paul Morris [:pmorris]]

Attachment: task-menupopups-7.patch

Rebased on trunk and fixed an eslint issue caught by the previous try server run.

Here's the new try server run which looks good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=56781e1cf106aeb2d6e77dac8a9ca029943deee7&selectedJob=240540000

The 'Linux x64 debug' Z4 failing test seems unrelated. So I'd say this is ready to land.

Comment 23

[Paul Morris [:pmorris]]

.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/29de53dcff07
[de-xbl] convert task progress and priority menupopup bindings to custom elements. r=philipp