1547229 - [de-xbl] rework daypicker binding (which inherits from button-base)

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Magnus Melin [:mkmelin]]

daypicker inherits from button-base which will be removed in bug 1519577.
https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/content/calendar-daypicker.xml#16

<daypicker> is used only in he recurrence dialog (e.g. when repeat weekly chosen): https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul#140

Given it's very limited usage, not sure this needs to be a custom element. It could be clearer just doing it in code. Not sure what it does, looks like dispatching day selections to https://searchfox.org/comm-central/rev/01f50ef57ff126a8a8146f9091a44d3ff0b31d4e/calendar/resources/content/datetimepickers/datetimepickers.js#805 (?)

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

I updated the daypicker to be a custom element.
Since the element has a lot of attributes and uses a couple of hbox for the styling, I think it's easier to handle it as a custom element.
If you think this is a bit of an overkill, I can quickly convert it to a simple element.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

Slightly updated code.
I'm seeing this error after a monthly daypicker is clicked.
It doesn't happen for the weekly daypicker and I can't figure out where this recursion is happening.

JavaScript error: resource://calendar/calendar-js/calRecurrenceInfo.js, line 480: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "Same occurrence found twice, protecting you from death by recursion" {file: "resource://calendar/modules/ical.js" line: 7193}]'[JavaScript Error: "Same occurrence found twice, protecting you from death by recursion" {file: "resource://calendar/modules/ical.js" line: 7193}]' when calling method: [calIRecurrenceRule::getOccurrences]

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

Rolling back to the previous patch as I introduced some bugs while trying to clean it up.
Sorry about that.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9066887 [details] [diff] [review]
dexbl-daypicker.patch

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

::: calendar/base/content/calendar-day-picker.js
@@ +3,5 @@
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* global MozXULElement, setBooleanAttribute, getSummarizedStyleValues, cal */
> +
> +class MozCalendarDayPicker extends customElements.get("button") {

please add documentation  too and @extends MozButton

@@ +6,5 @@
> +
> +class MozCalendarDayPicker extends customElements.get("button") {
> +    static get observedAttributes() {
> +        return ["label"];
> +    }

instead of using observedAttributes attributeChangedCallback and the related helpers you should be able to only have

static get inheritedAttributes() {
  return { ".toolbarbutton-text" : "label=value" }
}

@@ +9,5 @@
> +        return ["label"];
> +    }
> +
> +    connectedCallback() {
> +        if (this.delayConnectedCallback()) {

also need to check if (this.hasChildNodes()) 
... to prevent adding more when connectedCallback() runs more than once

@@ +14,5 @@
> +            return;
> +        }
> +
> +        const hbox = document.createElement("hbox");
> +        hbox.classList.add("daypickerclass");

daypickerclass is never used so we can remove it

@@ +18,5 @@
> +        hbox.classList.add("daypickerclass");
> +        hbox.setAttribute("flex", "1");
> +        hbox.setAttribute("align", "center");
> +
> +        const label = document.createElement("label");

createXULElement
(we're making sure the namespace is correct now)

@@ +66,5 @@
> +        }
> +    }
> +}
> +
> +customElements.define("daypicker", MozCalendarDayPicker);

normally CE must have a dash in their name, so let's fix that here now (there are also bugs for the internal hack that they don't, like you can't inherit from them)

So change to calendar-daypicker and MozCalendarDaypicker

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

Thanks for the quick review.
I updated everything and also fixed that console error I was getting, caused by the use of const variables.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9066961 [details] [diff] [review]
dexbl-daypicker.patch

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

Something's wrong, the "buttons" are all empty, no text showing. 
Looking more closely, I'm not sure you should be adding the label and all that as children. This is now inheriting a button, and that already has what's needed internally, no?

For the file name: calendar-daypicker.js without the extra hypen

Should still warp the implementation in 

// Wrap in a block to prevent leaking to window scope.
{

::: calendar/base/content/calendar-day-picker.js
@@ +3,5 @@
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * The MozCalendarDaypicker widget is used to display the days of
> + * the week and month for the reminder repeater dialog.

... and month for event recurrence.

@@ +7,5 @@
> + * the week and month for the reminder repeater dialog.
> + *
> + * @extends {MozButton}
> + */
> +

nit: remove the empty line

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

I needed to call super.connectedCallback(); in order for the button to behave as expected.
The label should show up now.

Should I add this attributes when I'm defining the custom element? { extends: "button" }
I thought I had to but as soon as I add it the button goes completely blank.

Comment 8

[Magnus Melin [:mkmelin]]

You can only inherit for the right element (daypicker needs to be a <button is="calendar-caypicker">, not <calendar-daypicker)

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: dexbl-daypicker.patch

Updated accordingly.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: dexbl-daypicker.patch

Looks good. I just fixed a few style nits

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4ae242ef0d4c
[de-xbl] convert the daypicker binding to <button is="calendar-daypicker">. r=mkmelin

Comment 12

[Jorg K (CEST = GMT+2)]

Need target 7.1.