1679129 - Incomplete overview section in recurring task editing box (only shows 3 months even if there would be room for more)

Status: RESOLVED FIXED

(Calendar :: Dialogs, defect)

Description

[P. Mergey]

Attachment: resized-box.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:83.0) Gecko/20100101 Firefox/83.0

Steps to reproduce:

Since some recent updates, it seems that the settings box for recurring tasks doesn't work properly. I try to edit a recurring task with a custom weekly repetition.

Actual results:

Only the three first months are displayed in the overview section of the recurring task edition box. The issue seems persistent even if the box is resized.

(I have only noticed this issue from version 78.4.2, but it may potentially have existed from some previous releases.)

Expected results:

More than three months should be able to be correctly displayed, at least by resizing the box (as was the case in previous versions).

Comment 1

[Magnus Melin [:mkmelin]]

It's a preview. AFAIK it was always only the three months.

Comment 2

[P. Mergey]

Greetings,

(In reply to Magnus Melin [:mkmelin] from comment #1)

(...) AFAIK it was always only the three months.

I can't revert to an earlier version of Thunderbird to take a screenshot of this preview, but I'm absolutely sure this box previously allowed more than three months to be displayed: I was using it for a long time to check recurring events over a full year (so at least twelve months were displayed before).

Comment 3

[Richard Marti (:Paenglab)]

Attachment: recurring-TB68.png

It showed more. See screenshot of TB68.

Comment 4

[Magnus Melin [:mkmelin]]

I guess something goes wrong around here: https://searchfox.org/comm-central/rev/8783b24ad5915927163977ea19a9968659b90d4c/calendar/base/content/dialogs/calendar-event-dialog-recurrence.js#76

Comment 5

[Lasana Murray]

Attachment: Screenshot_2020-12-30_16-20-19.png

I notice on linux even on 68 the boxes only take up one row.

Comment 6

[Lasana Murray]

(In reply to Richard Marti (:Paenglab) from comment #3)

Created attachment 9190782 [details]
recurring-TB68.png

It showed more. See screenshot of TB68.

Richard, what platform is this screenshot?

Comment 7

[Richard Marti (:Paenglab)]

It's Windows. To get a second row the dialog has to have a bigger gap than the first row's height below the first row. Then the second row jumps in.

Comment 8

[Lasana Murray]

Attachment: Screenshot_2020-12-30_18-44-49.png

Doesn't seem so (edited via devtools).

Comment 9

[Lasana Murray]

Attachment: bug1679129.patch

Using an <hbox> instead of a <box> allows the minimonth boxes to take up more horizontal space.
However, on Linux at least, it seems the parent nodes don't fill up the available vertical space so I only get one row.

The way the dialog works, it clones the first minimonth the needed number of times on resize, then inserts them into DOM. That was causing an error however; the properties of the minimonth instances end up only partially initialized. I fixed that by clearing the child elements of the cloned instances so connectedCallback() properly runs.

I also added some tests to resize and maximize the browser however the resizeTo API seems inconsistent. Occasionally I noticed it does not actually resize when I try it in the console or does not resize to the scale I specify. I also noticed the same behavior on the try server for linux builds (osx and win are ok).

Seems like some window dimensions, the wrong number of minimonths are created on linux like 1200x1024. Not sure why.

Comment 10

[Lasana Murray]

Test needs some more work.
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=5a21eb30a8deca38f041f2766cb626a7c3ec04fe

Comment 11

[Lasana Murray]

Attachment: bug1679129.patch

Ignore my comment on the previous patch about incorrect box count, I updated the logic in the test and it's consistent now.

Comment 12

[Lasana Murray]

Latest try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=7f8cb13106a1c4f1cdf6a70990e5423643b7a5f2

Comment 13

[Geoff Lankow (:darktrojan)]

(In reply to Lasana Murray from comment #9)

Using an <hbox> instead of a <box> allows the minimonth boxes to take up more horizontal space.
However, on Linux at least, it seems the parent nodes don't fill up the available vertical space so I only get one row.

The layout of this dialog has been completely messed up by various XUL features (XBL, <groupbox>, <grid>, etc.) being removed and reimplemented somehow. Time for a bit of a rethink. This is what I worked out, which isn't complete at all but did get pretty close to working properly:

• Give #preview-border the CSS -moz-box-flex: 1; display: flex;. This will make it fill any remaining space in the dialog and give all of the available space to #recurrence-preview. You can remove the flex="1" on #recurrence-preview, it does nothing with an HTML parent.
• Keep the change to <hbox>. <box> and <hbox> used to be the same thing, but apparently they are not any more.
• Get rid of the <spacer>. It's pointless these days. Note that some of the logic accounts for the spacer, by not handling the last element in a row.

The way the dialog works, it clones the first minimonth the needed number of times on resize, then inserts them into DOM. That was causing an error however; the properties of the minimonth instances end up only partially initialized. I fixed that by clearing the child elements of the cloned instances so connectedCallback() properly runs.

Create your own <hbox> and <calendar-minimonth> elements. Custom elements and cloning are a bad combination.

I also added some tests to resize and maximize the browser however the resizeTo API seems inconsistent. Occasionally I noticed it does not actually resize when I try it in the console or does not resize to the scale I specify. I also noticed the same behavior on the try server for linux builds (osx and win are ok).

Seems like some window dimensions, the wrong number of minimonths are created on linux like 1200x1024. Not sure why.

Hmm, not sure I can help here. Sounds odd to me.

Comment 14

[Lasana Murray]

Create your own <hbox> and <calendar-minimonth> elements. Custom elements and cloning are a bad combination.

When I do this, the boxes appear to no longer have the same dimensions. Maybe that may have been the reason for cloning?

Comment 15

[Lasana Murray]

Attachment: Screenshot_2020-12-30_16-20-19.png

Comment 16

[Lasana Murray]

Attachment: Screenshot_2021-01-14_23-40-28.png

Comment 17

[Geoff Lankow (:darktrojan)]

The ones on left have no readonly attribute, or it was added after they were added to the document.

Comment 18

[Lasana Murray]

Attachment: bug1679129v2.patch

I added the -moz-box-flex rule and removed the spacer and flex attribute.
I'm leaving the node cloning because it keeps the dimensions uniform.

Comment 19

[Lasana Murray]

Attachment: bug1679129v2.patch

Cloning removed.

Comment 20

[Geoff Lankow (:darktrojan)]

Comment on attachment 9197256 [details] [diff] [review]
bug1679129v2.patch

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

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +115,1 @@
>          firstElementChild.parentNode.insertBefore(newNode, firstElementChild);

Use .childElementCount instead of .children.length here and elsewhere. You could also use hbox.appendChild now the spacer is gone.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +435,5 @@
>  #preview-border {
>    border: none;
>    padding: 0;
> +  display: flex;
> +  -moz-box-flex:1;

Missing a space.

::: calendar/test/browser/recurrence/custom/browser.ini
@@ +9,5 @@
> +  mailnews.start_page.url=about:blank
> +subsuite = thunderbird
> +tags = recurrence-custom
> +
> +[browser_minimonthCount.js]

Put this test in with the others in the parent directory. There's no need to keep it separate, which has a non-zero overhead when running and maintaining the tests.

::: calendar/test/browser/recurrence/custom/browser_minimonthCount.js
@@ +22,5 @@
> +
> +const calendar = cal.async.promisifyCalendar(_calendar);
> +
> +let originalTimezone = Services.prefs.getStringPref("calendar.timezone.local");
> +Services.prefs.setStringPref("calendar.timezone.local", "UTC");

Don't need this any more.

@@ +33,5 @@
> + * @returns {number}
> + */
> +function getExpectedBoxCount(win) {
> +  let fieldset = win.document.querySelector("#preview-border");
> +  let fieldsetRect = fieldset.getBoundingClientRect();

I'm concerned that this test would continue to pass if the fieldset is broken so that it doesn't resize with the window. (Kind of like the current situation.)

What would be better is counting the number of rows/columns, then resizing the dialog by the dimensions of a month (ie. using resizeBy instead of resizeTo) and checking the number of rows/columns changed as expected.

@@ +56,5 @@
> +/**
> + * Provides a promise that waits on the "resize" event of a window to be fired.
> + * @param {nsIDOMWindow} win
> + */
> +function waitForResize(win) {

BrowserTestUtils.waitForEvent(win, "resize") should do the trick here.

@@ +82,5 @@
> +
> +  let getEventWin = BrowserTestUtils.domWindowOpened(null, async win => {
> +    await BrowserTestUtils.waitForEvent(win, "load");
> +    return win.document.documentURI == "chrome://calendar/content/calendar-event-dialog.xhtml";
> +  });

Don't we have a utility function for this?

@@ +90,5 @@
> +  let eventWin = await getEventWin;
> +  let iframe = eventWin.document.querySelector("iframe");
> +  await BrowserTestUtils.waitForEvent(iframe.contentWindow, "load");
> +
> +  let getRepeatWin = BrowserTestUtils.domWindowOpened(null, async win => {

BrowserTestUtils.promiseAlertDialogOpen is probably a better fit here, despite the name.

@@ +147,5 @@
> +    return true;
> +  });
> +
> +  let repeatMenu = iframe.contentDocument.querySelector("#item-repeat");
> +  repeatMenu.selectedIndex = 8;

Avoid using magic numbers like this. The list could change (unlikely, but it could) and then the test would have to be fixed. Look for the item you want and use it. But I think in this case you can just use repeatMenu.value = "custom".

Comment 21

[Lasana Murray]

Attachment: bug1679129v3.patch

I did some further cleaning up here:

1. Removed some while loops that appeared to be doing nothing.
2. Converted the while loops to for loops to avoid chaos if there are bugs.
3. Removed the maximize test, the redraw is delayed for some reason and I could not find much documentation on maxmize(). Calculating how many minimonths should be shown is also cumbersome due both to how onResize() calculates available space and the difference in screen sizes between devices. The test is likely to be flaky.

Comment 22

[Geoff Lankow (:darktrojan)]

Comment on attachment 9199252 [details] [diff] [review]
bug1679129v3.patch

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

I'm liking this a lot more. You're not testing resizes that make the window smaller. I suggest going back to the original size in each loop and checking you have the original number of minimonths.

::: calendar/test/browser/recurrence/browser_customMinimonthCount.js
@@ +46,5 @@
> +  // or both.
> +  for (let x = 0; x <= 3; x++) {
> +    for (let y = 0; y <= 3; y++) {
> +      if (x + y == 0) {
> +        // Skip resizing by to 0,0.

An extra word here.

@@ +96,5 @@
> +          // Occasionally, the container's vertical height is not what we expect
> +          // (12px less when resized by 1 minimonth). This seems to be only
> +          // happening during mochitests. The onResize() handler will not render
> +          // extra rows when this happens so resize the window incrementally
> +          // here until the container has the desired vertical height.

Wow, that's weird. I experience the same thing.

@@ +120,5 @@
> +          let actualCount = container.querySelectorAll("calendar-minimonth").length;
> +          Assert.equal(
> +            actualCount,
> +            expectedCount,
> +            `minimonth count is ${expectedCount} when scaled by ${x} x ${y} minimonths`

"Scaled" isn't really the right word, and maybe adding a plus sign makes more sense. Something like "resized by +1, +3 minimonths"?

Comment 23

[Lasana Murray]

I'm liking this a lot more. You're not testing resizes that make the window smaller. I suggest going back to the original size in each loop and checking you have the original number of minimonths.

I'd prefer to avoid doing that, it's likely to break easily and I've put enough time into this as it is. On my machine at least, resizing the window via JS enough times eventually stops responding. Looking at the spec, it says that browsers are allowed to "clamp" or ignore resizes as they see fit.

https://drafts.csswg.org/cssom-view/#dom-window-resizeby

"Scaled" isn't really the right word, and maybe adding a plus sign makes more sense.

I think it does make sense, we are scaling the window dimension relative to the minimonth dimensions but I'll change it to resized.

maybe adding a plus sign makes more sense. Something like "resized by +1, +3 minimonths"?

I was going with a coordinate themed thing here but sure, no problem.

Comment 24

[Lasana Murray]

Attachment: bug1679129v4.patch

Fixed typos. Changed x,y to +x,+y.

Try run in progress:

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

Comment 25

[Geoff Lankow (:darktrojan)]

Comment on attachment 9199664 [details] [diff] [review]
bug1679129v4.patch

Okay, this'll do.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8c47e9aa5803
Make minimonths fill available space in custom recurrence dialog. r=darktrojan

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9199664 [details] [diff] [review]
bug1679129v4.patch

[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: per bug summary
Testing completed (on c-c, etc.): on beta
Risk to taking this patch (and alternatives if risky): low

Comment 28

[Wayne Mery (:wsmwk)]

Comment on attachment 9199664 [details] [diff] [review]
bug1679129v4.patch

[Triage Comment]
Approved for esr78

Comment 29

[Rob Lemley [:rjl]]

Thunderbird 78.8.1:
https://hg.mozilla.org/releases/comm-esr78/rev/23cf46e67fa4

Comment 30

[Rob Lemley [:rjl]]

Backout Thunderbird 78.8.1:
https://hg.mozilla.org/releases/comm-esr78/rev/41dfa822c6c0

Comment 31

[Rob Lemley [:rjl]]

The minimonth dialog on esr78 still had problems with this patch applied. Additionally, the tests did not work even after applying the fix from bug 1689839. Lasana, could you take a look? I double checked the build and the dialog is not working like it does in the current beta.

You can use the build at https://treeherder.mozilla.org/jobs?repo=comm-esr78&revision=b5383265242b0be09bf5a3f00619f8c069b174e7

Comment 32

[Lasana Murray]

Ok, I'll look into it.