1534382 - [de-xbl] convert agenda list bindings to custom elements

Status: RESOLVED DUPLICATE of bug 1531305

(Calendar :: General, enhancement)

Description

[Paul Morris [:pmorris]]

Convert the four agenda list bindings to custom elements:
https://dxr.mozilla.org/comm-central/source/comm/calendar/base/content/agenda-listbox.xml

Comment 1

[Paul Morris [:pmorris]]

Attachment: calendar-agenda-list-0.patch

Ready for review. Tested by checking that the events list in the today-pane worked as expected:

• "today" etc. headings toggle correctly
• event selection works (thanks to inheritedAttributes and initializeAttributeInheritance)
• double clicking an event (regular or all day) opens the event for editing (but didn't open a second "create new event" dialog)
• double clicking outside of an event but inside the today pane opens a "create new event" dialog
• right click on event and check that context menu items worked

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9050078 [details] [diff] [review]
calendar-agenda-list-0.patch

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

Unfortunately bitrotted, sorry for the delay...

::: calendar/base/content/agenda-listbox.js
@@ +126,3 @@
>                  return;
>              }
> +            this.textContent = "";

For these, maybe instead check this.hasChildNodes() and bail?

One other thing to think about is, should perhaps the event listeners move to connectedCallback()

Comment 3

[Paul Morris [:pmorris]]

Attachment: calendar-agenda-list-1.patch

Now un-bit-rotted.

• if (this.hasChildNodes())

Good call. I've added these checks. Not sure how to test moving the element in the DOM. Dragging appears to be broken, unrelated to this patch:

JavaScript error: chrome://calendar/content/calendar-dnd-listener.js, line 603: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 1 [nsIDragService.invokeDragSession]

I tried changing the date of an event, and it was moved to a different day in the todaypane (but I'm not sure if this moves or recreates the element).

• if (this.delayConnectedCallback())

I've removed these checks. The converter puts them in, but reading the code for delayConnectedCallback, it is intended as an opt-in feature, for when it is needed, and I don't see a reason to use it here. Seems to me it adds complexity without real benefit in most cases, aside from those where it's really needed.

• event listeners

I looked into whether event listeners should be added in the constructor or in connectedCallback. What I read argued that the constructor is the preferred place. 1. connectedCallback can be called several times, like when an element is moved, and there's no point in re-adding them each time. 2. events that you want to catch might happen after construction but before the element is connected. 3. there could be confusion if you add them in connectedCallback and then think you need to remove them in disconnectedCallback, which is not the case. (Although, listeners added outside the element, say to window, would need to be removed in disconnectedCallback.)

"In general, the constructor should be used to set up initial state and default values, and to set up event listeners and possibly a shadow root."
https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-conformance

They are added in constructors in these docs:
https://developers.google.com/web/fundamentals/web-components/customelements

"Best practices for adding event listeners to a host element"
"A custom element can add an event listener to itself or its own children from its constructor without any issues"
https://github.com/Polymer/lit-element/issues/139

Comment 4

[Paul Morris [:pmorris]]

Attachment: calendar-agenda-list-2.patch

            if (this.delayConnectedCallback()) {
                return;
            }

I've added these back in after reading bug 1495946 comment 22.

Comment 5

[Paul Morris [:pmorris]]

Attachment: calendar-agenda-list-3.patch

And... updated so it works for current builds.

Comment 6

[Magnus Melin [:mkmelin]]

Hmm, there's a collision with bug 1531305 unfortunately. Mea clupa. Too many ongoing conversions... :(

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9051746 [details] [diff] [review]
calendar-agenda-list-3.patch

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

Since I already looked at bug 1531305 we may want to use that. Can you check if there's much differences?

Comment 9

[Paul Morris [:pmorris]]

Marking as a duplicate of bug 1531305 since that's where work is being done for this.