search

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

Fix automatic resizing of the dialog #3

Closed yan12125 closed 6 years ago

yan12125 commented 7 years ago

As described in https://github.com/spasche/openinbrowser/issues/23#issuecomment-334268040

Rob--W commented 6 years ago

Thanks for this PR. Could you explain how this patch is supposed to fix the bug? In particular, I wonder whether it is possible for the bug to occur with a different configuration (e.g. different window manager, zoom factor, pixel density, etc.)

yan12125 commented 6 years ago

I didn't dig into Firefox internals. Here's my guess: a 10x10 window is too small to even contain a title bar, so the "content area" (I'm not sure if this term is correct or not) of the dialog is not shown. On my machine, scripts in dialog.html are not executed until the content area is visible. For example, if I insert <script>console.log(new Date)</script> into dialog.html, the logged time is when I resize the dialog manually. If I make the content area visible at first, things are going well.

I'm not sure about other configurations. I can checkout it out on other machines.

Rob--W commented 6 years ago

Can you change it to width 300 and height 150?

The goal of starting with a small width and then resizing it is to allow dialog.js to expand the popup just enough to fit the content. If the width/height is too large, then the dialog would take too much space. 300x150 is still small enough and probably a safe value.

I am suggesting 300 as a width because it was set in https://bugzilla.mozilla.org/show_bug.cgi?id=951928 (335px as minimum on macOS, 300px elsewhere). The height suggestion of 150 is based on opening a new Firefox window, showing the menu items (e.g. press Alt) and measuring the height of the window.

yan12125 commented 6 years ago

Thanks for the suggestion. I use 335 instead of 300 as the original bug occurs on macOS.

BTW, I tested the original resolution on Arch Linux (xfwm4) and that works, so this is a mac-only issue.

Rob--W commented 6 years ago

Merged, thanks.