Only add beforeunload event listener when needed #64

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@kevinoid

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

@kevinoid kevinoid Only add beforeunload event listener when needed
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>
1d0b67b
@mattparlane

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.

@mattparlane mattparlane added a commit to wbgs/Placeholders.js that referenced this pull request Feb 20, 2014
@mattparlane mattparlane Only add beforeunload event listener when needed
This is only needed until jamesallardice#64 gets merged.
62a152e
@kevinoid

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!

@kevinoid kevinoid closed this Jan 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment