Disabling the Save and Publish buttons when heartbeat connection is in error state

[azaozz]

Currently we disable the Save/Publish buttons and show a "connection with the server fail" error message on the Edit Post screen when heartbeat connection times out or the server response is invalid/error.

This may be too aggressive. In some cases the XHR may fail but the form can still be submitted as discussed in ​#wordpres-dev.

[azaozz]

In 24695.patch​: only disable the Save/Publish buttons and show an error message when heartbeat times out (the most common case). We could show an error message in the other cases, but not sure what action to offer to the users. Showing an error without a suggested action is pretty bad UX.

[nacin]

Just thinking out loud here, maybe there should be a way to say 'ignore'?

[azaozz]

We can add 'Ignore' button in that warning but would most users understand what they are ignoring? Maybe we can skip 'parsererror', meaning the response wasn't a well formed json (perhaps there were PHP notices/warning messages, etc.), but still disable the buttons in all other cases?

[wpdavis]

A really poor mockup, but what about something like this?

[nacin]

In 24691:

For an intermittent connection, only disable save and publish buttons when the request times out. An error still means a we have a connection.

props azaozz.
see #24695.

[nacin]

I find [24691] to be both sufficient and safe for 3.6. Leaving open for feedback.

[aaroncampbell]

I think [24691] is good for 3.6. If we see other errors that we can handle well, we can add support for them in future versions.

[nacin]

I have an idea. Let's only deactivate the buttons if we confirmed that heartbeat worked at least once.

Let's say we load post-new.php. Some kind of server configuration or plugin (possibly always) prevents heartbeat from properly returning data, and the whole thing continuously times out. We suddenly disable their Publish/Save buttons.

A better approach would be to only disable Publish/Save *after* at least one successful heartbeat. This should mitigate any potential weird edge cases that will inevitably end up in the forums.

This should be really easy to do — set a flag on .success() to check on .fail()?

[nacin]

carldanley is looking into this, with azaozz backing things up.

[carldanley]

The patch I just uploaded takes care of ensuring that we only pulse these triggered events if we've connected to the server on this page load at least once before. I tested this by throwing a sleep(40) into wp_ajax_heartbeat(). If you want to test what it did before, just set heartbeatHasRunSuccessfully to true by default (instead of false).

Let me know if anything needs to be changed.

[nacin]

24695.diff​ (some stuff as carldanley's patch, just pared down a bit to match existing dev patterns) is tested and works.

Two things:

1. I feel like a "Try again" link is still going to be useful for people who want immediate gratification. A spinner could be used — and in fact, it may not be a bad idea for a spinner to always be spinning here, since we want them to know we're actively trying to regain connection.
2. We should consider telling them (assuming window.sessionStorage) that we're trying to log their data to the browser, in case there's an issue.

[nacin]

Marking as ready for review. The two points can be discussed today.

[carldanley]

Replying to nacin:

24695.diff​ (some stuff as carldanley's patch, just pared down a bit to match existing dev patterns) is tested and works.

I'm ok with the code being pared down to what you think makes more sense. However, for what it's worth, please consider the following:

I'm following the brace coding styles from jQuery (​http://contribute.jquery.org/style-guide/js/#spacing) which happen to be found in WP as well (​http://make.wordpress.org/core/handbook/coding-standards/javascript/#ifelse). Douglas Crockford also specifies this requirement here: ​http://javascript.crockford.com/code.html#compound%20statements. Also, ​http://net.tutsplus.com/tutorials/javascript-ajax/24-javascript-best-practices-for-beginners/. It's important to use braces in JS so things aren't left up to the compiler to decide where it's placed and results in more secure code.

Also, function declarations are much faster than named function expressions (see ​http://jsperf.com/function-declaration-vs-function-expression). That being said, my advice is to make sure that we move the anonymous function out of .success() and back into it's own function.

Two things:

1. I feel like a "Try again" link is still going to be useful for people who want immediate gratification. A spinner could be used — and in fact, it may not be a bad idea for a spinner to always be spinning here, since we want them to know we're actively trying to regain connection.

I agree with this. Not sure where the placement of said loader would be.

2. We should consider telling them (assuming window.sessionStorage) that we're trying to log their data to the browser, in case there's an issue.

Not a bad idea. We need to flesh these details out I think.

[nacin]

Sorry, removing brackets from single-line if statements is a habit.

For what it's worth, I don't know if our own JS standards are "codified". They've had influence from koopersmith and azaozz but I am not sure if they've been fully vetted; I think there have been some changes by others. I agree with following jQuery and Crockford where possible.

Regarding NFE, I'm just trying to follow, for now, a fairly standard and existing pattern we use in this file and elsewhere. By "much faster" we're talking literally immeasurable differences for single operations. This isn't something being done in a quickly repeating loop.

I'm fine with removing things out of expressions but then we should do that for all deferred callbacks for that XHR call.

[carldanley]

Makes total sense. Also, I can see that this is really adding a function signature for a single line of code BUT it's important going forward. Pure speed benefits aside, this anonymous function will probably end up getting patched in early 3.7 to remove anonymity AND allow for a clear indication of what's going on. It removes the extremely long "inline" function calls and makes code easier to read. For this patch, I kept it simple to do what you wanted going forward. My patch for the heartbeat API (​http://core.trac.wordpress.org/attachment/ticket/23216/23216-heartbeat-cleanup.5.diff [most recent iteration as of the time I'm posting this comment]) fixes all of these anonymous functions by moving them to proper private declarative functions. Just my two cents though, I'm ok with going whatever flow you think is necessary for this patch. If you think it's a good idea, I can open a new ticket to discuss this practice of code styling in JS.

Also, a lesser point, can you imagine trying to add documentation for this anonymous function when it's inline like that?

I can take point on supplying a new patch with the fixes for the other deferred's in that XHR if you need, just give me the word.

[azaozz]

I feel like a "Try again" link is still going to be useful for people who want immediate gratification.

The heartbeat connection times out after 30 sec. On the Edit Post screen heartbeat runs with 15 sec. interval (it doesn't in case there is only one registered user, but it switches to 15 sec. interval after a connection error). In practice as soon as one connection attempt times out another is fired, so a "Try again" link wouldn't do anything to speed this up.

We should consider telling them (assuming window.sessionStorage) that we're trying to log their data to the browser, in case there's an issue.

All supported browsers ​except IE7 have sessionStorage, perhaps we can mention this in the message (btw do we still care about IE7?).

Sorry, removing brackets from single-line if statements is a habit.

Same habit here :) Not sure when the JS coding standard was changed. Part of the reason we have that standard is to keep JS and PHP as similar as possible.

[carldanley]

Replying to azaozz:

(btw do we still care about IE7?).

Excited to know this answer as well =P

[azaozz]

24695.3.diff works well. 24695.4.diff​ is almost identical to 24695.3.diff​, only fixes a typo in using the cached jQuery notice instead of selecting it again.

+1 to commit

[nacin]

In 24743:

Better 'Connection lost' notice that includes an indication of activity. fixes #24695.

[nacin]

props carldanley for [24743]!