mozilla / persona-yahoo-bridge

A ProxyIdP service for bridging major IdPs who lack support for the BrowserID protocol.
26 stars 15 forks source link

IE8 dialog resize fix #241

Closed jaredhirsch closed 11 years ago

jaredhirsch commented 11 years ago

Not quite ready, needs feedback. @shane-tomlinson mind having a look?

The problem with resizing the dialog on IE8 is that IE8 never tells you the actual height of the dialog, including browser chrome. But, if you tell IE8 to resize the window, it thinks you mean window+chrome, so you wind up with squished contents. Oddly, setting a height in window.open does not include the browser chrome, hence our current headaches.

Rather than ask IE8 to resize the window to a height of 375, instead we (1) record the actual height, then (2) reset it to that height later.

I'm out of time for today and will spin up a bigtent instance tomorrow to test this in an IE8 VM, but wanted to get feedback on the general approach sooner than later.

It turns out there are two places where resizing happens. One of them is the error page, mentioned by @jrgm in mozilla/browserid#3476. The other is when signin completes, which is the similar error I found when trying to repro earlier today. Both cases are handled here, and I tried to be clean about namespacing the code without going nuts and cleaning up everything.

Feedback welcome.

jaredhirsch commented 11 years ago

ah, nope, this is wrong. the problem is that window.open() and window.resizeTo() do quite different things, and so the patch needs to probably resizeTo, measure the height, and then add that difference in order to resize correctly. you could even do this in successive turns via a setTimeout. I'll close + try again tomorrow.

shane-tomlinson commented 11 years ago

@6a68 - I think what you could do is use window.resizeBy. So, here is the theory, before going to Yahoo!, record the window inner dimensions. Save those to sessionStorage. When you come back from Yahoo!, check the current window inner dimensions. Calculate the difference. use window.resizeBy to do the correct resizing.

jaredhirsch commented 11 years ago

@shane-tomlinson that's a fantastic idea. I'm not thrilled about depending on sessionStorage, but it seems safe to assume we have it at this point in the flow (else we'd have errored out when the dialog first opened, right?).

resizeBy seems to be a no-op on mobile/tablet platforms, so I think we're good there.

I'm guessing (and @jedp confirms) that asking for window.foo causes all kinds of consternation in FXOS, so we'll have to figure out where to best shim in window = window || {}. I'm going to explicitly duck that here.

After doing a little testing, it turns out that this approach also handles zoom bugs gracefully, as resizeBy() doesn't overflow the user's monitor on any browser, including IE8.

I'll get a PR out shortly. Thanks for the review :-)