Rob--W / open-in-browser

A browser extension that offers the ability to open files directly in the browser instead of downloading them.
Other
88 stars 16 forks source link

Avoid repositioning and drastically resizing dialog after mapping #7

Closed dphlin closed 6 years ago

dphlin commented 6 years ago

It is generally advisable not to explicitly position windows, especially dialog-type windows, leaving it to the window manager to do so instead.

The extension's dialog behaviour differs from firefox's native save-as/open dialog:

I would suggest:

I note also that firefox does not set PPosition; the extension's newly opened window has a PPosition of (0,0). This is possibly a mozilla bug.

Rob--W commented 6 years ago

I can try to have a better initial estimate. However, I think that it is necessary to reposition the new dialog in order to get it at a consistent position (an improvement of #3).

Have you experimented with not setting an explicit left/right position when the dialog is opened, and checking whether the dialog is positioned as expected? If the results are positive I will consider not setting a position, or adding a preference to not change the position.

dphlin commented 6 years ago

On repositioning, when the window is first mapped (after creation without specifying left and top), the position is as expected/desired (albeit about 25px square) -- it is the subsequent resize and move. Locally disabling the move using resizeDialog(false) keeps the x-y position according to what the window manager decided.

I'll test and compare some other environments to see what the firefox's native behaviour would have been.

Rob--W commented 6 years ago

This patch could be used to avoid unnecessary moves (together with 97e1f5f1a870a4ea98722d27f61da735e5d8180c). Can you perform extensive tests and report whether when the extension behaves in a more desirable way; with or without the following patch?

diff --git a/extension/dialog.js b/extension/dialog.js
index f0a6ffc..7cfb56c 100644
--- a/extension/dialog.js
+++ b/extension/dialog.js
@@ -87,6 +87,12 @@ function resizeDialog(/*boolean*/ moveDialog) {
     var HEIGHT = dialogMain.scrollHeight + verticalDialogPadding;
     dialogMain.style.minWidth = '';

+    if (Math.abs(window.outerHeight - HEIGHT) < 10 &&
+        Math.abs(window.outerWidth - WIDTH) < 10) {
+        // If the popup size hardly changes, don't move it.
+        moveDialog = false;
+    }
+
     if (moveDialog === true) {
         chrome.windows.update(chrome.windows.WINDOW_ID_CURRENT, {
             width: WIDTH,
Rob--W commented 6 years ago

I went with a different solution (bfc6b58d91d6b02282df5d5326c2696d3230a366). If the position is close to the calculated position, then I will not move the dialog. Together with the previous patch to persist the dialog size, the issue should now be fully fixed.

Please test the latest master branch and report back whether the extension behaves as desired.

Rob--W commented 6 years ago

No reply - assuming that the bug has been fixed.