1679802 - remove <deck> from calendar-event-dialog-recurrence.xhtml

Status: RESOLVED FIXED

(Calendar :: General, task)

Description

[Khushil Mistry [:khushil324]]

Remove <deck> from https://searchfox.org/comm-central/rev/1fa5ebe1384434e904b33bb7de8f0a3d6e8bfdc5/calendar/base/content/dialogs/calendar-event-dialog-recurrence.xhtml#72

It has four children. These we can show/hide as needed.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-0.patch

Comment 2

[Alessandro Castellani [:aleca]]

Comment on attachment 9190319 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-0.patch

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

Unfortunately this doesn't work for me.
The view doesn't change in the custom recurrence dialog when I change the type in the dropdown.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +836,5 @@
>  }
>  
> +#period-box {
> +  min-height: 185px;
> +}

We should try to avoid forced height values as these won't properly account for l10n strings.
What problems are you trying to solve with this?
Could a sizeToContent() method be used instead?

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-1.patch

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9190469 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-1.patch

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

A bit better, but the dialog doesn't resize when changing option.
For example, start with a daily or weekly value in the menulist, then change to monthly or annually and you'll see that the bottom part is cut outside the dialog.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xhtml
@@ +52,2 @@
>        </vbox>
>        <vbox>

Add a flex=1 to this vbox since the width is not determined anymore by the largest card in the deck.
Without it, the menulist width will jump in size at every change.

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-2.patch

Here, window.sizeToContent() or sizeToContent() are not working as it is a combination of window, dialog and HTML:fieldset. And dialog-content-box is not shrinking once it gets expanded. Do you know any other method for such an operation?

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9190624 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-2.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +866,5 @@
> +  let windowHeight = 0;
> +  for (let element of document.querySelectorAll("fieldset")) {
> +    windowHeight += element.clientHeight;
> +  }
> +  window.innerHeight = windowHeight;

I think a regular window.sizeToContent() would work as I don't see a big problem if the dialog doesn't shrink when a selection is changed.

Comment 7

[Khushil Mistry [:khushil324]]

window.sizeToContent() is not working.

Comment 8

[Alessandro Castellani [:aleca]]

window.sizeToContent(); works for me.
What problem are you experiencing? Can you share a screenshot?

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Screenshot 2020-12-09 at 1.06.36 AM.png

I selected weekly from the menulist then I switched to monthly and then again switched back to weekly. I am adding window.sizeToContent(); in the function updateRecurrenceBox.

Comment 10

[Alessandro Castellani [:aleca]]

Yeah, as I wrote in comment 6 "I think a regular window.sizeToContent() would work as I don't see a big problem if the dialog doesn't shrink when a selection is changed."

The important aspect is the dialog expanding to accommodate the newly visible larger content.
Not shrinking the dialog is okay, and sometimes even better as we avoid for the action buttons to jump up and down at every change.

Comment 11

[Khushil Mistry [:khushil324]]

Cool, got it. Updating the patch.

Comment 12

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-3.patch

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9192049 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-3.patch

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

Looks good to me, thanks.

Comment 14

[Geoff Lankow (:darktrojan)]

Comment on attachment 9192049 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-3.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.xhtml
@@ +56,2 @@
>                      disable-on-readonly="true"
>                      disable-on-occurrence="true">

Please fix this indentation.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-4.patch

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c745b2de01d9
remove <deck> XUL element from calendar-event-dialog-recurrence.xhtml dialog. r=aleca,darktrojan

Comment 17

[Lasana Murray]

The changes and tests in bug 1679129 depend on the changes made here to work properly. Without this the first <fieldset> takes up too much space. Can we uplift this?

Comment 18

[Magnus Melin [:mkmelin]]

I think that should be safe.

Comment 19

[Lasana Murray]

Comment on attachment 9192343 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-4.patch

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: Minimal, this is needed for bug 1679129 tests to pass.
Testing completed (on c-c, etc.): https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c3eb730f2a53d2dcc85a7dba87b19d01e1c6db6d
Risk to taking this patch (and alternatives if risky): None that I can think of, removes obsolete <deck> element.

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9192343 [details] [diff] [review]
Bug-1679802_de-deck-calendar-event-dialog-recurrence-xhtml-4.patch

[Triage Comment]
Approved for esr78

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.9.0:
https://hg.mozilla.org/releases/comm-esr78/rev/f6e1457ef19a