Closed Bug 1547229 Opened 1 year ago Closed 1 year ago

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

Categories

(Calendar :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

Attachments

(1 file, 6 obsolete files)

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 (?)

Assignee: nobody → alessandro
Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

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.

Attachment #9066881 - Flags: review?(mkmelin+mozilla)
Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

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]
Attachment #9066881 - Attachment is obsolete: true
Attachment #9066881 - Flags: review?(mkmelin+mozilla)
Attachment #9066885 - Flags: review?(mkmelin+mozilla)
Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

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

Attachment #9066885 - Attachment is obsolete: true
Attachment #9066885 - Flags: review?(mkmelin+mozilla)
Attachment #9066887 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
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
Attachment #9066887 - Flags: review?(mkmelin+mozilla)
Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

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

Attachment #9066887 - Attachment is obsolete: true
Attachment #9066961 - Flags: review?(mkmelin+mozilla)
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
Attachment #9066961 - Flags: review?(mkmelin+mozilla) → review-
Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

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.

Attachment #9066961 - Attachment is obsolete: true
Attachment #9066981 - Flags: review?(mkmelin+mozilla)

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

Attached patch dexbl-daypicker.patch (obsolete) — Splinter Review

Updated accordingly.

Attachment #9066981 - Attachment is obsolete: true
Attachment #9066981 - Flags: review?(mkmelin+mozilla)
Attachment #9066985 - Flags: review?(mkmelin+mozilla)

Looks good. I just fixed a few style nits

Attachment #9066985 - Attachment is obsolete: true
Attachment #9066985 - Flags: review?(mkmelin+mozilla)
Attachment #9066992 - Flags: review+

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED

Need target 7.1.

Flags: needinfo?(philipp)
Target Milestone: --- → 7.0
Flags: needinfo?(philipp)
Target Milestone: 7.0 → 7.1
You need to log in before you can comment on or make changes to this bug.