1480338 - Event/task dialog should always be big enough for the iframe it contains

Status: RESOLVED FIXED

(Calendar :: Dialogs, enhancement)

Description

[Geoff Lankow (:darktrojan)]

This has been annoying me for an age, so I've decided to fix it.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-1.diff

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8996941 [details] [diff] [review]
1480338-dialog-minsize-1.diff

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

::: calendar/lightning/content/lightning-item-panel.js
@@ +294,5 @@
> +            // so that they can properly set up the layout they might need.
> +            // Push to the end of the event queue.
> +            setTimeout(overflowListener, 0);
> +        }, { once: true });
> +        iframe.addEventListener("resize", overflowListener);

There is an "overflow" event, would that help in any way?

Comment 3

[[:MakeMyDay]]

This seems to not allow to reduce the size again once you enlarged the dialog for more then the minimum size when reopeing again:

1. open in a dialog
2. enlarge the dialog and close the window
3. reopen the event and try the make the window smaller again

step 3 isn't possible, even if there is still more space then in step 1.

Comment 4

[[:MakeMyDay]]

One more thing: when doing step 1, (addditional) toolbar buttons have been still cut off or been completely out of sight for me..

Comment 5

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #3)
> step 3 isn't possible, even if there is still more space then in step 1.

I don't see that. Anybody else?

(In reply to [:MakeMyDay] from comment #4)
> One more thing: when doing step 1, (addditional) toolbar buttons have been
> still cut off or been completely out of sight for me..

The toolbar isn't inside the iframe, so it's not taken into account with this patch. Still, it's not worse than before, is it?

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-2.diff

Is this any better, MakeMyDay?

Comment 7

[[:MakeMyDay]]

Comment on attachment 8998686 [details] [diff] [review]
1480338-dialog-minsize-2.diff

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

Unfortunately, it doesn't. I see no different behaviour compared to the previous patch on Win10.

Apart from that if you want feedback for a patch or having a question like for the integration bar, please f? resp. NI me - otherwise your request might get off the radar.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-3.diff

Let's try this with CSS instead of XUL attributes.

Comment 9

[[:MakeMyDay]]

Comment on attachment 9004118 [details] [diff] [review]
1480338-dialog-minsize-3.diff

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

This doesn't work either, but meanwhile I understand why this happens.

Im'm using a scaled screen resolution (like Richard does, iirc). That makes the values retrieved from getBoundingClientRect being fractional numbers instad of integer values. That triggers a cascade of calls of overflowListener (even if the size would roughly fit), which then sets an inappropriate minimum height/width and prevent reducing the dialog size (even if it would still be big enough).

::: calendar/lightning/content/lightning-item-panel.js
@@ +265,5 @@
> +            let { scrollWidth, scrollHeight } = iframe.contentDocument.documentElement;
> +            let bcr = iframe.getBoundingClientRect();
> +
> +            let diffX = scrollWidth - bcr.width;
> +            let diffY = scrollHeight - bcr.height;

Changing the above definitions resolves the issue for me:

// bcr may provide fractional numbers if using a scaled screen resolution,
// so rounding and subtracting an offset of 1px is necessary to be able
// to resize the dialog on such systems
let diffX = scrollWidth - Math.round(bcr.width) - 1;
let diffY = scrollHeight - Math.round(bcr.height) - 1;

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-4.diff

Ah, floating point maths. Gets you every time. Let's use integer maths instead.

Nice find, MakeMyDay.

Comment 11

[[:MakeMyDay]]

Comment on attachment 9007439 [details] [diff] [review]
1480338-dialog-minsize-4.diff

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

::: calendar/lightning/content/lightning-item-panel.js
@@ +264,5 @@
> +            let { scrollWidth, scrollHeight } = iframe.contentDocument.documentElement;
> +            let { clientWidth, clientHeight } = iframe;
> +
> +            let diffX = scrollWidth - clientWidth;
> +            let diffY = scrollHeight - clientHeight;

Sorry to say, but that doesn't work again. The -1 offset from my previous comment is required to be able to resize with scaled resolution at all. Please add also a comment for that.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-5.diff

That is weird, but I've seen it happen now. Also it turns out I was adding a listener to one thing and removing it from another, so that was never going to work.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9007515 [details] [diff] [review]
1480338-dialog-minsize-5.diff

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

Probably best for MakeMyDay to review this. Code looks ok on a general level, but needs a test.

Comment 14

[Geoff Lankow (:darktrojan)]

Does a test that won't pass unless the "Notify attendees" checkbox in the bottom corner is visible count? Because there's one of them waiting to land in bug 1484636. (calendar/test/mozmill/eventDialog/testEventDialog.js)

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-test-1.diff

Here's a simple test added to our not-currently-active mozmill suite which tests I've fixed the bug. I think it should be fine but I've sent it to Try anyway.

Comment 16

[[:MakeMyDay]]

Comment on attachment 9007515 [details] [diff] [review]
1480338-dialog-minsize-5.diff

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

Thanks, finally it's working like a charme now, both with scaled and not scaled resolution.

Comment 17

[[:MakeMyDay]]

Comment on attachment 9008642 [details] [diff] [review]
1480338-dialog-minsize-test-1.diff

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

The test for the initial resizing looks good, but can you please also add a testing sequence for resizing after the initial loading, e.g.:

1. open the dialog
2. resize to a measure so that is obvious it's big enough to not need min-values
3. close the dialog
4. reopen (you now are sure that there are no minimum size values set)
5. try to resize to less then the min values
6. check that you end with the min values and not less
7. close the dialog
8. open the dialog
9. resize to a size bigger then the min values

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-test-2.diff

Comment 19

[[:MakeMyDay]]

Comment on attachment 9009497 [details] [diff] [review]
1480338-dialog-minsize-test-2.diff

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

The test locally runs into a timeout for me after the initial opening of the event and task windows. I added the |eventDialog| to the test list to make it run at all, am I still missing something else?

Comment 20

[[:MakeMyDay]]

For completeness, these are the exceptions I get. For testing the event dialog

>  EXCEPTION: waitFor: Timeout exceeded for '() => {
>        return (iframeNode.clientWidth + 1 > scrollWidth) &&
>            (iframeNode.clientHeight + 1 > scrollHeight);
>    }'
>    at: utils.js line 396
>       TimeoutError utils.js:396 13
>       waitFor utils.js:434 11
>       MozMillController.prototype.waitFor controller.js:687 3
>       checkLargeEnough testEventDialogSize.js:116 5
>       testEventDialog/< testEventDialogSize.js:29 9
>       invokeEventDialog test-calendar-utils.js:335 5
>       testEventDialog testEventDialogSize.js:28 5
>       Runner.prototype.wrapper frame.js:584 9
>       Runner.prototype._runTestModule frame.js:654 9
>       Runner.prototype.runTestModule frame.js:700 3
>       Runner.prototype.runTestFile frame.js:533 3
>       runTestFile frame.js:712 3
>       Bridge.prototype._execFunction server.js:177 10
>       Bridge.prototype.execFunction server.js:181 16
>       Session.prototype.receive server.js:282 3
>       AsyncRead.prototype.onDataAvailable server.js:88 3

and for testing the task dialog

>  EXCEPTION: waitFor: Timeout exceeded for '() => {
>        return (iframeNode.clientWidth + 1 > scrollWidth) &&
>            (iframeNode.clientHeight + 1 > scrollHeight);
>    }'
>    at: utils.js line 396
>       TimeoutError utils.js:396 13
>       waitFor utils.js:434 11
>       MozMillController.prototype.waitFor controller.js:687 3
>       checkLargeEnough testEventDialogSize.js:116 5
>       testTaskDialog/< testEventDialogSize.js:69 9
>       invokeEventDialog test-calendar-utils.js:335 5
>       testTaskDialog testEventDialogSize.js:68 5
>       Runner.prototype.wrapper frame.js:584 9
>       Runner.prototype._runTestModule frame.js:654 9
>       Runner.prototype.runTestModule frame.js:700 3
>       Runner.prototype.runTestFile frame.js:533 3
>       runTestFile frame.js:712 3
>       Bridge.prototype._execFunction server.js:177 10
>       Bridge.prototype.execFunction server.js:181 16
>       Session.prototype.receive server.js:282 3
>       AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 21

[Stefan Sitter [:ssitter]]

Does this fix Bug 394195?

Comment 22

[[:MakeMyDay]]

For content part of the window yes, but if you have an overly wide toolbar, this will be still visually cut off since the with of the toolbar isn't considered when calculating the appropriate minimum width.

Comment 23

[Geoff Lankow (:darktrojan)]

(In reply to [:MakeMyDay] from comment #20)
> >  EXCEPTION: waitFor: Timeout exceeded for '() => {
> >        return (iframeNode.clientWidth + 1 > scrollWidth) &&
> >            (iframeNode.clientHeight + 1 > scrollHeight);
> >    }'
> >    at: utils.js line 396
> >       TimeoutError utils.js:396 13
> >       waitFor utils.js:434 11
> >       MozMillController.prototype.waitFor controller.js:687 3
> >       checkLargeEnough testEventDialogSize.js:116 5
> >       testEventDialog/< testEventDialogSize.js:29 9

Oh man, that's the first thing it tests! :-( The window does actually get resized in the test though, right?

I wonder if it's a matter of just changing the +1s to +2s. MMD, since it's probably easier for you to check than me, would you add a line like this inside the waitFor function and see what happens:

> dump(`${iframeNode.clientWidth}, ${scrollWidth}, ${iframeNode.clientHeight}, ${scrollHeight}\n`);

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-test-3.diff

Attempt 3, in which I remember that resizeTo sets the outerWidth/Height of a window, including the decorations, not the innerWidth/Height, which doesn't include the decorations. Except on Linux, apparently, where there are no decorations but the titlebar, and that seems to be ignored.

Comment 25

[[:MakeMyDay]]

Still failing:

>  EXCEPTION: assert: Failed for '() => event.window.outerWidth == 650'
>    at: utils.js line 362
>       assert utils.js:362 11
>       MozMillController.prototype.assert controller.js:960 3
>       testEventDialog/< testEventDialogSize.js:40 9
>       invokeEventDialog test-calendar-utils.js:335 5
>       testEventDialog testEventDialogSize.js:39 5
>       Runner.prototype.wrapper frame.js:584 9
>       Runner.prototype._runTestModule frame.js:654 9
>       Runner.prototype.runTestModule frame.js:700 3
>       Runner.prototype.runTestFile frame.js:533 3
>       runTestFile frame.js:712 3
>       Bridge.prototype._execFunction server.js:177 10
>       Bridge.prototype.execFunction server.js:181 16
>       Session.prototype.receive server.js:282 3
>       AsyncRead.prototype.onDataAvailable server.js:88 3

>  EXCEPTION: waitFor: Timeout exceeded for '() => mozmill.utils.getWindows("Calendar:EventDialog").length == 0'
>    at: utils.js line 396
>       TimeoutError utils.js:396 13
>       waitFor utils.js:434 11
>       MozMillController.prototype.waitFor controller.js:687 3
>       invokeEventDialog test-calendar-utils.js:338 5
>       testTaskDialog testEventDialogSize.js:68 5
>       Runner.prototype.wrapper frame.js:584 9
>       Runner.prototype._runTestModule frame.js:654 9
>       Runner.prototype.runTestModule frame.js:700 3
>       Runner.prototype.runTestFile frame.js:533 3
>       runTestFile frame.js:712 3
>       Bridge.prototype._execFunction server.js:177 10
>       Bridge.prototype.execFunction server.js:181 16
>       Session.prototype.receive server.js:282 3
>       AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 26

[Geoff Lankow (:darktrojan)]

I bet the first is 649 or 651 or something like that. The second test is failing in sympathy with the first.

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-6.diff

There's issues with persistence that I don't think even Firefox has fully worked out (bug 1444525 and others). Our messing about with the id of the root element just makes them worse. I've simplified what we do when the dialog loads/unloads, and now use resizeTo instead of setting the width/height attributes so we correctly set the outer dimensions not the inner dimensions. I've also changed the test to ensure what we record in xulStore.json is what we should record.

However.

This all goes out the window (pun not intended) when we start scaling the UI. There seems to be a bug which causes the window to keep growing by a few pixels each way, every time it is opened. As our automated tests aren't run with a scaled UI, the test deals with exact values in unscaled enviroments, and with a tolerance elsewhere.

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 1480338-dialog-minsize-test-4.diff

Comment 29

[[:MakeMyDay]]

Comment on attachment 9011364 [details] [diff] [review]
1480338-dialog-minsize-6.diff

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

Thanks, I think we're done here. Can you still file a follow-up bug to also consider the width of the toolbar so that toolbar buttons are always visible if enabled by customizing?

Comment 30

[[:MakeMyDay]]

Comment on attachment 9011365 [details] [diff] [review]
1480338-dialog-minsize-test-4.diff

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

Thanks, r+wc

::: calendar/test/mozmill/eventDialog/testEventDialogSize.js
@@ +142,5 @@
> +    dump(`Dialog is ${outer.window.outerWidth} by ${outer.window.outerHeight}\n`);
> +}
> +
> +function getPersistedValue(type, which) {
> +    return Services.xulStore.getValue("chrome://calendar/content/calendar-event-dialog.xul", `calendar-${type}-dialog`, which);

Please don't exceed a line length of 100 characters

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5b5a269c9cde
Prevent event/task dialog being too small for the iframe inside; r=MakeMyDay
https://hg.mozilla.org/comm-central/rev/e6adb6f71893
Test event/task dialog is big enough for the iframe it contains; r=MakeMyDay

Comment 32

[Markus Adrario [:Taraman]]

The Test fails for me locally, because on my machine, the Task Dialog won't get narrower than 694 px.

Comment 33

[Markus Adrario [:Taraman]]

For the previous comment:
Machine is Ubuntu VM 64bit.

On macOSX, the dialog seems to be smaller.
On Tryserver: Dialog is 462 by 517
On my Hackintosh VM: Dialog is 473 by 517