1511229 - [de-xbl] Inline and Scriptify recurrence-preview binding.

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Comment 1

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9028854 [details] [diff] [review]
recurrence-preview.patch

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

Looks like it works

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xul
@@ +508,5 @@
>  
>    <!-- preview -->
>    <groupbox id="preview-border" flex="1">
>      <caption id="recurrence-preview-label">&event.recurrence.preview.label;</caption>
> +    <hbox id="recurrence-preview" flex="1" style="overflow: hidden;">

put styling into the css file, though it looks like this is an unrelated error - also on trunk the months are slightly too large so the last one is cut off by some pixels.

Comment 3

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Magnus, could you please post a screenshot of the horizontal cutting thing that you were talking about? I guess it is not related to this patch.

Comment 4

[Magnus Melin [:mkmelin]]

Yeah not related to this patch, and very minor, so let's not investigate this further atm.

Comment 5

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Comment 6

[[:MakeMyDay]]

Comment on attachment 9042785 [details] [diff] [review]
recurrence-preview.patch

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

Thanks, r+ with the below comments considered. If you don't have different OS to investigate for the last comment, maybe Paenglab could help.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +22,5 @@
> +        this.mResizeHandler = null;
> +
> +        this.mDateTime = null;
> +
> +        this.mResizeHandler = this.onResize.bind(this);

Can you please remove the empty lines above.

@@ +52,5 @@
> +        // this is a simple division which always yields a positive integer value.
> +        let numHorizontal =
> +            (containerWidth -
> +                (containerWidth % contentWidth)) /
> +            contentWidth;

Can you please change this to

let cWidth = containerWidth % contentWidth;
let numHorizontal = (containerWidth - cWidth) / contentWidth;

@@ +62,5 @@
> +        // this is a simple division which always yields a positive integer value.
> +        let numVertical =
> +            (containerHeight -
> +                (containerHeight % contentHeight)) /
> +            contentHeight;

and this to

let cHeight = containerHeight % contentHeight;
let numVertical = (containerHeight - cHeight) / contentHeight;

::: calendar/base/content/dialogs/calendar-event-dialog.css
@@ +16,2 @@
>      -moz-user-focus: normal;
>  }

Remove the recurrence-preview block entirely. It's non-functional with the replacement of recurrence-preview with the box element.

@@ +63,5 @@
>  }
> +
> +#recurrence-preview {
> +    overflow: hidden;
> +}

css files in calendar/base/content are not intended to define styles but only hold the binding definitions. Styling should go into calendar/base/themes/common or if OS specific into its sibbling folder. For this definition, calendar/base/themes/common/dialogs/calendar-event-dialog.css might be an appropriate venue, since we have already a definition for ther recurrence dialog therein.

However, on Windows, this styling doesn't seem to be required (I see no difference with or without it, so you probably want to move this into a OS specific definition.

Comment 7

[[:MakeMyDay]]

One more thing: can you please add JSDoc descriptions to RecurrencePreview and its functions?

Comment 8

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Makemyday, I have addressed all you comments. Lemme know if you want to update some of the doc comments that I have added. The overflow property doesn't do much even on *nix OSes so removing it.

Comment 9

[Arshad Khan [:arshad]]

Richard I have removed overflow:hidden css rule from recurrence-preview element. It doesn't do much on windows and macos. It just crops few pixel of content maybe 3 or 4px. wdyt about it?

Comment 10

[Arshad Khan [:arshad]]

Richard, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

Comment 11

[Richard Marti (:Paenglab)]

I don't know. This is too deep for me. Maybe better you ask Brian.

Comment 12

[Arshad Khan [:arshad]]

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

Comment 13

[Arshad Khan [:arshad]]

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

Comment 14

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Comment 15

[[:MakeMyDay]]

Comment on attachment 9042942 [details] [diff] [review]
recurrence-preview.patch

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

Doc blocks look ok in general, just some nit

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +27,5 @@
> +        this.mDateTime = null;
> +        this.mResizeHandler = this.onResize.bind(this);
> +        window.addEventListener("resize", this.mResizeHandler, true);
> +    },
> +    /**

Can you please add an empty line before the doc block here and befor the other blocks below?

@@ +35,5 @@
> +        window.removeEventListener("resize", this.mResizeHandler, true);
> +    },
> +    /**
> +     * Setter for mDateTime property.
> +     * @param {date} val - The date value that is to be set.

The typing is not accurate here - the argument is not a JS date but an calIDateTime object.

@@ +42,5 @@
> +        this.mDateTime = val.clone();
> +        return this.mDateTime;
> +    },
> +    /**
> +     * Getter for mDateTime property.

Add a @return value here - type as per comment above.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Arshad Khan [:arshad] from comment #12)

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

It's not ringing a bell for me. Paolo, do you remember anything about this?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/542023c45213
Inline and Scriptify recurrence-preview binding. r=MakeMyDay

Comment 18

[[:MakeMyDay]]

Arshad, can you please still fix the nits in comment 15?

Comment 19

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Comment 20

[Arshad Khan [:arshad]]

Attachment: recurrence-preview.patch

Comment 21

[Arshad Khan [:arshad]]

Attachment: rp-doc-nits.patch

Comment 22

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #16)

(In reply to Arshad Khan [:arshad] from comment #12)

Brian, when is it important to add -moz-user-focus: normal rule? nearly all the xbl bindings have that but when doing the conversion when should I remove them and when leave them?

It's not ringing a bell for me. Paolo, do you remember anything about this?

The default is "-moz-user-focus: ignore;", and I think you need to still set it to "normal" if you want the element to be accessible with the keyboard.

Comment 23

[[:MakeMyDay]]

Comment on attachment 9043199 [details] [diff] [review]
rp-doc-nits.patch

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

Thanks, r+ with this fixed

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +39,4 @@
>      /**
>       * Setter for mDateTime property
> +     * @param {calIDateTime object} val - The date value that is to be set.
> +     * @returns {calIDateTime object} - mDateTime that is cloned value of original val argument.

Make it just

* @param   {calIDateTime} val  The datetime value that is to be set.	
* @returns {calIDateTime}      A clone of the passed in argument.