Closed Bug 1679129 Opened 11 months ago Closed 9 months ago

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

Categories

(Calendar :: Dialogs, defect)

Thunderbird 78
defect

Tracking

(thunderbird_esr78+ fixed, thunderbird86 affected)

RESOLVED FIXED
87 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird86 --- affected

People

(Reporter: gzm, Assigned: lasana)

References

()

Details

(Keywords: regression)

Attachments

(6 files, 6 obsolete files)

Attached image 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).

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

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).

Attached image recurring-TB68.png

It showed more. See screenshot of TB68.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Incomplete overview section in recurring task editing box → Incomplete overview section in recurring task editing box (only shows 3 months even if there would be room for more)
Component: Tasks → Dialogs
Assignee: nobody → lasana
Status: NEW → ASSIGNED

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

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

Flags: needinfo?(richard.marti)

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.

Flags: needinfo?(richard.marti)

Doesn't seem so (edited via devtools).

Attached patch bug1679129.patch (obsolete) — Splinter Review

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.

Attachment #9194955 - Flags: review?(geoff)
Attachment #9194955 - Flags: review?(geoff)
Attached patch bug1679129.patch (obsolete) — Splinter Review

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

Attachment #9194955 - Attachment is obsolete: true
Attachment #9195749 - Flags: review?(geoff)

(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.

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?

Attached image Screenshot_2020-12-30_16-20-19.png (obsolete) —
Attachment #9197251 - Attachment is obsolete: true

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

Attached patch bug1679129v2.patch (obsolete) — Splinter Review

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.

Attachment #9195749 - Attachment is obsolete: true
Attachment #9195749 - Flags: review?(geoff)
Attachment #9197255 - Flags: review?(geoff)
Attached patch bug1679129v2.patch (obsolete) — Splinter Review

Cloning removed.

Attachment #9197255 - Attachment is obsolete: true
Attachment #9197255 - Flags: review?(geoff)
Attachment #9197256 - Flags: review?(geoff)
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".
Attachment #9197256 - Flags: review?(geoff) → feedback+
Attached patch bug1679129v3.patch (obsolete) — Splinter Review

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.
Attachment #9197256 - Attachment is obsolete: true
Attachment #9199252 - Flags: review?(geoff)
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"?
Attachment #9199252 - Flags: review?(geoff)

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.

Attachment #9199252 - Attachment is obsolete: true
Attachment #9199664 - Flags: review?(geoff)

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

Okay, this'll do.

Attachment #9199664 - Flags: review?(geoff) → review+
Target Milestone: --- → 87 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8c47e9aa5803
Make minimonths fill available space in custom recurrence dialog. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

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

Attachment #9199664 - Flags: approval-comm-esr78?

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

[Triage Comment]
Approved for esr78

Attachment #9199664 - Flags: approval-comm-esr78? → approval-comm-esr78+
Attachment #9199664 - Flags: approval-comm-esr78+

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

Flags: needinfo?(lasana)
See Also: → 1689839

Ok, I'll look into it.

Flags: needinfo?(lasana)
Depends on: 1679802
You need to log in before you can comment on or make changes to this bug.