1509076 - [de-xbl] Inline and Scriptify daypicker-weekday binding.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Arshad Khan [:arshad]]

Binding Link - https://searchfox.org/comm-central/source/calendar/base/content/calendar-daypicker.xml#51

Comment 1

[Arshad Khan [:arshad]]

Attachment: datepicker-weekday.patch

Comment 2

[Arshad Khan [:arshad]]

Attachment: datepicker-weekday.patch

This binding is only used in calendar-event-recurrence dialog that's why I am inlining the content and scriptifying the method into a JS object.

Comment 3

[Arshad Khan [:arshad]]

https://searchfox.org/comm-central/source/calendar/test/mozmill/shared-modules/test-item-editing-helpers.js#75 is something that is not yet fixed by bug, I am looking for ways to fix tests.

Comment 4

[Arshad Khan [:arshad]]

Attachment: datepicker-weekday.patch

Tests fixed - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=35d2cc637d666a31923936aceeb3f0aafcd18765

Comment 5

[Arshad Khan [:arshad]]

Comment on attachment 9026827 [details] [diff] [review]
datepicker-weekday.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
@@ +132,5 @@
>                         disable-on-occurrence="true"
>                         control="daypicker-weekday"/>
> +                <hbox id="daypicker-weekday"
> +                      flex="1"
> +                      disable-on-readonly="true"

I guess I missed this. Is it used?

Comment 6

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 7

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 8

[Arshad Khan [:arshad]]

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

Comment 9

[Arshad Khan [:arshad]]

test-junk-command.js::test_delete_junk_messages  is failing. Not sure why this happening. Maybe this is because of the ruleactiontargetbindings issue and not because of this patch.

Comment 10

[Arshad Khan [:arshad]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d0c5fb8a8b60d6cd8aeed4507213fd60ddaf701&selectedJob=213822067 Indeed the test fail was because of ruleaction target binding bug. Now green lights for this patch from test.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9027066 [details] [diff] [review]
daypicker-weekday.patch

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

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +291,5 @@
>  listbox[disabled="true"] {
>      color: -moz-FieldText;
>  }
>  
> +.daypicker-weekday {

#daypicker-weekday

or? I don't see a daypicker-weekday class

or maybe this is not needed then?

Comment 12

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 13

[[:MakeMyDay]]

Comment on attachment 9028080 [details] [diff] [review]
daypicker-weekday.patch

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

::: calendar/base/content/calendar-daypicker.xml
@@ +37,5 @@
>        <constructor><![CDATA[
>            this.setAttribute("autoCheck", "true");
>            this.setAttribute("type", "checkbox");
> +          this.setAttribute("disable-on-readonly", "true");
> +          this.setAttribute("disable-on-occurrence", "true");

This change seems to be in this patch as well in that of bug 1509180 - can you please check this so that the patches apply on top of each other?

Comment 14

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

I ll rebase daypickr-monthday on top of this patch.

Comment 15

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 16

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 17

[Arshad Khan [:arshad]]

I know the comments are not the best but let me know I ll update them.

Comment 18

[Arshad Khan [:arshad]]

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

Comment 19

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

Comment on attachment 9042958 [details] [diff] [review]
daypicker-weekday.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +21,5 @@
> +    /**
> +     * Method intitializing daypickerWeekday.
> +     */
> +    init() {
> +        this.weekStartOffset = Preferences.get("calendar.week.start", 0);

Let's use Services.prefs instead.

@@ +24,5 @@
> +    init() {
> +        this.weekStartOffset = Preferences.get("calendar.week.start", 0);
> +
> +        let props =
> +            Services.strings.createBundle("chrome://calendar/locale/dateFormat.properties");

There is surely a helper in calL10nUtils for this?

@@ +33,5 @@
> +            let dow = i + this.weekStartOffset;
> +            if (dow >= 7) {
> +                dow -= 7;
> +            }
> +            let day = props.GetStringFromName("day." + (dow + 1) + ".Mmm");

`day.${dow + 1}.Mmm`

@@ +69,5 @@
> +        let numChilds = mainbox.childNodes.length;
> +        for (let i = 0; i < numChilds; i++) {
> +            let child = mainbox.childNodes[i];
> +            child.removeAttribute("checked");
> +        }

We can convert this to for (let child of mainbox.childNodes).

@@ +71,5 @@
> +            let child = mainbox.childNodes[i];
> +            child.removeAttribute("checked");
> +        }
> +        for (let i = 0; i < val.length; i++) {
> +            let index = val[i] - 1 - this.weekStartOffset;

I think this one can be for..of as well

Comment 20

[Arshad Khan [:arshad]]

Attachment: daypicker-weekday.patch

Comment 21

[Arshad Khan [:arshad]]

Comment on attachment 9047885 [details] [diff] [review]
daypicker-weekday.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +240,5 @@
> +            let dow = i + this.weekStartOffset;
> +            if (dow >= 7) {
> +                dow -= 7;
> +            }
> +            let day = cal.l10n.getString("dateFormat", `day.${dow + 1}.Mmm`);

Fallen, I hope this is ok?

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cae3bf889a15
Inline and Scriptify daypicker-weekday binding. r=philipp