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

RESOLVED FIXED in 7.0

Status

enhancement
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: mkmelin, Assigned: pmorris)

Tracking

(Blocks 1 bug)

Trunk
Dependency tree / graph

Details

Attachments

(1 attachment, 7 obsolete attachments)

Reporter

Description

4 months ago

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.

Assignee

Comment 1

4 months ago

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.

Assignee

Comment 2

4 months ago

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.

Reporter

Comment 3

4 months ago

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

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?

Assignee

Comment 5

4 months ago

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.

Assignee

Updated

4 months ago
Depends on: 1531870
Depends on: 1531877
Assignee

Comment 6

3 months ago
Posted patch task-menupopups-0.patch (obsolete) — Splinter Review

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.

Attachment #9055016 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 7

3 months ago
Posted patch task-menupopups-1.patch (obsolete) — Splinter Review

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

Attachment #9055016 - Attachment is obsolete: true
Attachment #9055016 - Flags: review?(mkmelin+mozilla)
Attachment #9055032 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 8

3 months ago

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

Assignee

Comment 9

3 months ago

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.

Assignee

Comment 10

3 months ago
Posted patch task-menupopups-2.patch (obsolete) — Splinter Review

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

Attachment #9055032 - Attachment is obsolete: true
Attachment #9055032 - Flags: review?(mkmelin+mozilla)
Attachment #9055523 - Flags: review?(mkmelin+mozilla)

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

Reporter

Comment 12

3 months ago
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.)
Reporter

Comment 13

3 months ago
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
Attachment #9055523 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 14

3 months ago

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

Assignee

Comment 15

3 months ago
Posted patch task-menupopups-3.patch (obsolete) — Splinter Review

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.

Attachment #9055523 - Attachment is obsolete: true
Attachment #9055897 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 16

3 months ago
Posted patch task-menupopups-4.patch (obsolete) — Splinter Review

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

All mozmill calendar tests passed locally.

Attachment #9055897 - Attachment is obsolete: true
Attachment #9055897 - Flags: review?(mkmelin+mozilla)
Attachment #9055908 - Flags: review?(mkmelin+mozilla)
Reporter

Comment 17

2 months ago
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);
Attachment #9055908 - Flags: review?(philipp)
Attachment #9055908 - Flags: review?(mkmelin+mozilla)
Attachment #9055908 - Flags: feedback+
Assignee

Comment 18

2 months ago
Posted patch task-menupopups-5.patch (obsolete) — Splinter Review

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

Attachment #9055908 - Attachment is obsolete: true
Attachment #9055908 - Flags: review?(philipp)
Attachment #9056125 - Flags: review?(philipp)
Reporter

Updated

2 months ago
Status: NEW → ASSIGNED
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.
Attachment #9056125 - Flags: review?(philipp) → review+
Assignee

Comment 20

2 months ago
Posted patch task-menupopups-6.patch (obsolete) — Splinter Review

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

Attachment #9056125 - Attachment is obsolete: true
Assignee

Comment 21

2 months ago

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

Assignee

Comment 22

2 months ago

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.

Attachment #9058259 - Attachment is obsolete: true
Assignee

Comment 23

2 months ago

.

Keywords: checkin-needed

Comment 24

2 months ago

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

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

Updated

2 months ago
Target Milestone: --- → 7.0
You need to log in before you can comment on or make changes to this bug.