Closed Bug 1480338 Opened Last year Closed Last year

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

Categories

(Calendar :: Dialogs, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 8 obsolete files)

This has been annoying me for an age, so I've decided to fix it.
Attached patch 1480338-dialog-minsize-1.diff (obsolete) β€” β€” Splinter Review
Attachment #8996941 - Flags: review?(philipp)
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?
Attachment #8996941 - Flags: review?(philipp) → review+
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.
One more thing: when doing step 1, (addditional) toolbar buttons have been still cut off or been completely out of sight for me..
(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?
Attached patch 1480338-dialog-minsize-2.diff (obsolete) β€” β€” Splinter Review
Is this any better, MakeMyDay?
Attachment #8996941 - Attachment is obsolete: true
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.
Attachment #8998686 - Flags: feedback-
Blocks: 1484636
Depends on: 1485884
No longer depends on: 1485884
Attached patch 1480338-dialog-minsize-3.diff (obsolete) β€” β€” Splinter Review
Let's try this with CSS instead of XUL attributes.
Attachment #8998686 - Attachment is obsolete: true
Flags: needinfo?(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;
Flags: needinfo?(makemyday)
Attached patch 1480338-dialog-minsize-4.diff (obsolete) β€” β€” Splinter Review
Ah, floating point maths. Gets you every time. Let's use integer maths instead.

Nice find, MakeMyDay.
Attachment #9004118 - Attachment is obsolete: true
Attachment #9007439 - Flags: review?(philipp)
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.
Attached patch 1480338-dialog-minsize-5.diff (obsolete) β€” β€” Splinter Review
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.
Attachment #9007439 - Attachment is obsolete: true
Attachment #9007439 - Flags: review?(philipp)
Attachment #9007515 - Flags: review?(philipp)
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.
Attachment #9007515 - Flags: review?(philipp) → review?(makemyday)
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)
Attached patch 1480338-dialog-minsize-test-1.diff (obsolete) β€” β€” Splinter Review
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.
Attachment #9008642 - Flags: review?(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.
Attachment #9007515 - Flags: review?(makemyday) → review+
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
Attachment #9008642 - Flags: feedback+
Attached patch 1480338-dialog-minsize-test-2.diff (obsolete) β€” β€” Splinter Review
Attachment #9008642 - Attachment is obsolete: true
Attachment #9008642 - Flags: review?(makemyday)
Attachment #9009497 - Flags: review?(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?
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
Does this fix Bug 394195?
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.
(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`);
Attached patch 1480338-dialog-minsize-test-3.diff (obsolete) β€” β€” Splinter Review
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.
Attachment #9009497 - Attachment is obsolete: true
Attachment #9009497 - Flags: review?(makemyday)
Attachment #9010600 - Flags: review?(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
I bet the first is 649 or 651 or something like that. The second test is failing in sympathy with the first.
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.
Attachment #9007515 - Attachment is obsolete: true
Attachment #9011364 - Flags: review?(makemyday)
Attachment #9010600 - Attachment is obsolete: true
Attachment #9010600 - Flags: review?(makemyday)
Attachment #9011365 - Flags: review?(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?
Attachment #9011364 - Flags: review?(makemyday) → review+
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
Attachment #9011365 - Flags: review?(makemyday) → review+
Pushed by geoff@darktrojan.net:
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → 6.6
See Also: → 1494892
The Test fails for me locally, because on my machine, the Task Dialog won't get narrower than 694 px.
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
You need to log in before you can comment on or make changes to this bug.