4
\$\begingroup\$

I have a function that makes sure the user wants to actually leave the page. I am not a jQuery expert. Is the code below the most efficient way to do this function? Or can it be simplified much more to run smoother? Any advice is greatly appreciated.

$(document).ready(function() {
  formmodified=0;
    $('form *').change(function(){
        formmodified=1;
    });
  window.onbeforeunload = confirmExit;
    function confirmExit() {
      if (formmodified == 1) {
          return "Are you sure you want to leave this page? This will abandon any progress on changes to document preferences";
      }
  }
  $("input[name='ModifyRecord']").click(function() {
        formmodified = 0;
    });
    $("input[name='DeleteRecord']").click(function() {
        formmodified = 0;
    });
    $("input[name='DeleteChecked']").click(function() {
        formmodified = 0;
    });
});
\$\endgroup\$
5
\$\begingroup\$

If you don't need the confirmExit function anywhere else, you can just assign it to to window.beforeunload.

window.onbeforeunload = function() {
    if (formmodified == 1) {
        return "Are you sure you want to leave this page? This will abandon any progress on changes to document preferences";
    }
}

Make sure your indentation is consistent, it's confusing to read when brackets don't match up.

Last but not least, it might be helpful (saves you the trouble of writing the same thing multiple times) and more performant to use a class for elements that should have a click handler to reset the "modified" state of the form:

$("input.formreset").click(function() {
    formmodified = 0;
});
\$\endgroup\$
  • \$\begingroup\$ Can you show me an example of the exact way you would write it? \$\endgroup\$ – David Brierton Sep 26 '17 at 12:45
  • 2
    \$\begingroup\$ It is preferable that you use named functions rather than anon functions even if you use the function only once as it makes tracing a lot easier. Also there is no need to use window.onbeforeunload when onbeforeunload will do, I bet you don't use window.setTimeout, window.document or for many other global scoped properties, so why use window here? \$\endgroup\$ – Blindman67 Sep 26 '17 at 13:05
  • \$\begingroup\$ You're absolutely right, using a named function is not an issue at all and can increase readability. Using an anonymous function is, however, more concise, which seemed favorable to me as this is such a small script. The idea was to make this simple script as simple as possible. I guess, at the end of the day, it's a matter of overall complexity of the program you're dealing with as well as personal preference. \$\endgroup\$ – ced Sep 26 '17 at 19:15
5
\$\begingroup\$

You have defined the variable formmodified as a global variable. This means that it can be modified from anywhere using the code window.formmodified = "yes". If you don't want that, you have to declare a local variable at the beginning of your code:

$(document).ready(function() {
    var formmodified = 0;

The var makes the difference between the global and the local variable.

Since the variable takes only two values, these values should be true and false instead of 1 and 0. This also makes checking this condition simpler:

if (formmodified) { ... }

That reads much more natural than your current code.

Instead of defining the same action on three jQuery selectors, you can combine them into one:

$("input[name='ModifyRecord'], input[name='DeleteRecord'], input[name='DeleteChecked']").click(function() {
    formmodified = false;
});

But having a CSS class for these input fields, as suggested in ced's answer, is even better.

\$\endgroup\$
  • \$\begingroup\$ Can you show me an example of the exact way you would write it? \$\endgroup\$ – David Brierton Sep 26 '17 at 12:45
2
\$\begingroup\$

Is this the most efficient way to do this function?

For the sake of efficiency, what you have may be close. As others have pointed out, the selector for the fields to reset the value of formmodified could be combined. For example, the single selector offered in the answer by Roland Illig:

$("input[name='ModifyRecord'], input[name='DeleteRecord'], input[name='DeleteChecked']").click(function() {
    formmodified = false;
});

This could be simplified using an arrow function (a feature of - note the browser support of those).

$("input[name='ModifyRecord'], input[name='DeleteRecord'], input[name='DeleteChecked']")
    .click(_ => formmodified = false);

Another approach is to use event delegation by adding a click handler to an element containing those inputs - e.g. the document, form, etc. Then check the target property of the event argument to see if it:

  1. has a value in the tagName attribute of input, and
  2. has a value in the name attribute matching one of the three inputs targeted (i.e. 'ModifyRecord', 'DeleteRecord', 'DeleteChecked')

For #1, we can access the tagName property. For #2, we can create an array - e.g. var resetNames = ['ModifyRecord', 'DeleteRecord', 'DeleteChecked']; and if the name attribute is in that array:

$(document).click(function(clickEvent) {
    var targ = clickEvent.target;
    if (targ.tagName.toLowerCase() == 'input' && resetNames.includes(targ.name)) {
        formmodified = 0; 
    }
}

Like ced mentioned, adding a class attribute to the inputs to target could simplify the logic. Then the logic to check if the name of the input is in the array could be replaced by a simple call to targ.hasClass().

Or can it be simplified much more to run smoother?

I am not sure about smoother, but I do know that the MDN documentation for onbeforeunload states:

You can and should handle this event through window.addEventListener() and the beforeunload event. More documentation is available there.1

For a single page application, there may not be a need to have more than one function called during the beforeunload event, but as applications grow larger, there may be a need to have multiple. That approach (with window.addEventListener('beforeunload', function() {...}) supports multiple callback functions to be run.

Also note that the documentation for beforeunload states:

If a string is assigned to the returnValue Event property, a dialog appears asking the user for confirmation to leave the page... WebKit-based browsers don't follow the spec for the dialog box. An almost cross-browser working example would be close to the following:

window.addEventListener("beforeunload", function (e) {
      var confirmationMessage = "\o/";
      e.returnValue = confirmationMessage;     // Gecko, Trident, Chrome 34+
      return confirmationMessage;              // Gecko, WebKit, Chrome <34
});

2

So for both cases, set the returnValue property on the event argument (i.e. e in the example above) and then return that value.

And in agreement with Roland, add var (I would say let but maybe you are supporting browsers that don't fully support that and other features like IE 10 and earlier) before initializing formmodified to limit the scope to that of the DOM-loaded callback.

Additionally, the format for .ready() can be simplified to "the recommended syntax"3:

$(function() { ... })

$(function() { //DOM ready callback
  var formmodified = 0;
  var resetNames = ['ModifyRecord', 'DeleteRecord', 'DeleteChecked'];
  $('form *').change(function() {
    formmodified = 1;
    console.log('set formmodified to 1');
  });
  window.addEventListener('beforeunload', confirmExit);

  function confirmExit(e) {
    if (formmodified === 1) {
      var confirmationMessage = "Are you sure you want to leave this page? This will abandon any progress on changes to document preferences";
      e.returnValue = confirmationMessage
      return e.returnValue;
    }
  }
  $(document).click(function(clickEvent) {
    var targ = clickEvent.target;
    if (targ.tagName.toLowerCase() == 'input' && resetNames.includes(targ.name)) {
      formmodified = 0; 
      console.log('set formmodified to 0');
    }
  });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.10.0/jquery.min.js"></script>
<form>
  Some text
  <input type="Number" name="numberField" />
  <input name="CreateRecord" type="button" value="Create record" />
  <input name="ModifyRecord" type="button" value="Modify record" />
  <input name="DeleteRecord" type="button" value="Delete record" />
  <input name="DeleteChecked" type="button" value="Delete checked" />
</form>


1https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload#Notes

2https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload

3http://api.jquery.com/ready/

\$\endgroup\$
  • \$\begingroup\$ jQuery has built-in event delegation support, and I would recommend using it instead of rolling your own: $(document).on('click', 'input[name=ModifyRecord], input[name=DeleteRecord], input[name=DeleteChecked]', function () { formmodified = 0 }) \$\endgroup\$ – Ilmari Karonen May 30 at 16:30
1
\$\begingroup\$

According to MDN and the HTML Living Standard, the most reliable way to write a beforeunload event handler is something like this:

window.onbeforeunload = function (event) {
    if (formmodified) {
       event.preventDefault();  // standard method to trigger prompt
       return event.returnValue = "Are you sure you want to leave this page?";  // compat
    }
};

or, using addEventListener():

window.addEventListener('beforeunload', function (event) {
    if (formmodified) {
       event.preventDefault();  // standard method to trigger prompt
       return event.returnValue = "Are you sure you want to leave this page?";  // compat
    }
});

(Note that, for security reasons, basically all modern browsers will ignore the returned prompt text, and will show a fixed generic prompt instead. Still, providing a meaningful prompt may be useful, just in case any tries to use your page on Internet Explorer or some other old browser that still shows the custom prompt.)

Also, as noted here on MDN, having a beforeunload event handler active can reduce performance by forcing browsers to actually unload and reload the page instead of just suspending it and storing its state in a cache. Because of this, it's a good idea to avoid setting such handlers unless necessary, and remove them once they're no longer needed.

In particular, instead of having a global formmodified flag that tells your event handler whether to actually do something or not, I'd recommend just installing the event handler when the user modifies the form, and uninstalling it once the form is submitted:

function confirmUnload (event) {
   event.preventDefault();  // standard method to trigger prompt
   return event.returnValue = "Are you sure you want to leave this page?";  // compat
};

const submitButtonSelector = 'input[name=ModifyRecord], input[name=DeleteRecord], input[name=DeleteChecked]'; 

$('form').on('change', function () {
    window.addEventListener('beforeunload', confirmUnload);
}).on('click', submitButtonSelector, function () {
    window.removeEventListener('beforeunload', confirmUnload);
});

BTW, assuming that you're actually sending the form data to a server via Ajax, you might also consider only removing the beforeunload event listener when the call actually returns successfully. That way, if the user clicks the submit button and then tries to immediately leave the page before the data has actually been sent (e.g. because their WiFi connection just happened to drop for a moment), they'll still get prompted.

(Also, note that the code above assumes that you only have one form on the page. If you might have several, you'd have to keep track of which of them contain unsaved changes, and add or remove the beforeunload event handler accordingly.)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.