search

jamesallardice / Placeholders.js

A JavaScript polyfill for the HTML5 placeholder attribute
http://jamesallardice.github.io/Placeholders.js
948 stars 232 forks source link

Only add beforeunload event listener when needed #64

Closed kevinoid closed 8 years ago

kevinoid commented 10 years ago

Currently Placeholders.js unconditionally adds a listener to the beforeunload event. Unfortunately, this prevents "Back-Forward Cache"/"Page Cache"/"Fast History Navigation" on Firefox/Safari/Opera respectively.

Since Placeholders.disable is a no-op when browsers have native support for placeholders, the beforeunload event is only required on browsers lacking native support. This commit only adds it on those browsers.

Note: It's not clear to me why disabling the placeholders when unloading the page is useful, so it may be worth removing altogether based on your analysis of the tradeoffs. However, since I am unsure, this commit still registers it when placeholders are not natively supported.

Signed-off-by: Kevin Locke klocke@quantpost.com

mattparlane commented 10 years ago

Hi all, just thought I'd add my 2c.

I'm currently running pretty much this exact same patch in production and it works fine, and it also removes a pesky console error when the page is unloaded, and also solves a few issues with my JS form validation code that I haven't been able to solve any other way.

So that's a +1 from me.

kevinoid commented 8 years ago

I just came across this pull request and did some checking. It looks like the issue was fixed for browsers with native support in d68fe45.

It might still be nice to provide users who are more concerned about the caching than the flash of unstyled placeholders a way to disable it, but I think it's probably outside the scope of this PR and not something I'm personally interested in. If you have other ideas or reasons for keeping this PR open, feel free to reopen it.

Thanks for fixing this issue and for the continued updates!