Check and refresh post locks with heartbeat

[azaozz]

This will change the frequency we check post locks: 15 sec. when the user is active and 2 min. when inactive and add prominent warnings when somebody else is editing or when another user takes over.

[azaozz]

Initial patch:

• Show (prominent) warning when post is locked with two choices: Take over or Go back.
• When editing a post: show warning that another user has taken over with one choice: go to All Posts screen.
• Prevent trashing a post if it's locked.

To do:

• Styling for the warnings. We need a nice default for a warning dialog that can be reused and can be blocking if needed.
• Consider showing a warning when the same user opens the same post in another window or on another computer. Currently this would only work when editing from the Edit Post screen (not from Quick Press or PressThis).
• Consider limiting who can take over editing. In the initial patch all users that can edit a post can take over.
• After we have per-user autosaves, do an autosave when a user tries to save a draft or publish a locked post.

[grahamarmfield]

Seen the image in the Daily Digest.

I know it's early days still, but just wanted to add in a couple of accessibility considerations.

If we're going to pop up a panel like that please can we ensure that keyboard focus is transferred into the panel when it opens - and given to some sensible Heading that describes what's going on. The text 'Other person is currently editing' could get it (consider tabindex=0, or tabindex=-1 if this item should not become permanently part of tab order).

Then the tab order should be sensible within the panel. Consider what should happen if user tabs beyond Go Back button, or before Take over button. Cycling tab within panel is OK.

When one of the buttons is pressed, the keyboard focus must be returned to somewhere sensible. Does clicking Go Back just close the panel? Should be made clear - in an accessible way.

Is it going to be obvious to users what happens if they click Take Over - including what'll happen to the other person who currently has control?

[dh-shredder]

Previous patch from @azaozz plus generic dialog styles, modified messaging, and gravatars!

[dh-shredder]

Screenshot with "Take Over" Prompt

[dh-shredder]

Screenshot after lock has been taken

[dh-shredder]

23697-2.patch​ uses the changes from @azaozz's patch, plus:

• Generic Dialog/Notice Styles
• Default buttons displayed
• Slightly more friendly messaging
• Gravatars, so that you know whose post you're taking over (or who to go beat with Ted to give your lock back)

Screenshots of the takeover notice​ and after lock has been taken​ both attached.

[azaozz]

In 23661:

Check post locks with heartbeat and display modal notifications when a post is locked or a user takes over editing, props dh-shredder, see #23697

[nacin]

Good first pass. Based on a review:

I am not sure why we switched from AUTOSAVE_INTERVAL * 2 to 120, is this because "autosave" the concept is changing as we know it?

Please remember that get_option( 'show_avatars' ) should be checked.

Please do not use global $post_ID. [23661] should use the original $post = get_post();

Avatars may not be from gravatar.com, so the JS code should be removed. If we are going to do nasty things like parse the source URI out of get_avatar(), then we should also un-encode amp; there, not in JS.

Rather than sending back new_lock as a : separated piece of data, only to regex out a piece of it, can we send back structured data? As in, user ID + time?

Rather than <?php if ( ! $locked ) echo ' style="display:none"'; ?>> could we use a class?

wp_trash_post() or wp_delete_post() is likely invoked in other situations where it may make sense to block it if the post is locked.

The post_lock_take_over filter (which should probably be post_lock_override or override_post_lock for clarity, even if "take over" ends up being the UI) should receive $post and $user, not just the IDs.

[azaozz]

Thanks for the review.

Since locks are not tied to autosave any more, using AUTOSAVE_INTERVAL * 2 may cause some problems if set to different value. With heartbeat locks are refreshed every 15 sec. when the user is active and every 120 sec. when inactive. We were discussing "taking over" immediately (without showing a notice) if the user that has the lock is inactive. Wanted to run some more tests before adding that.

Sure, will use get_post() and get_option( 'show_avatars' ) to shorten it there.

Rather than sending back new_lock as a : separated piece of data...

Sure. This is unused at the moment. It was sent in the old autosave response and can be used for lock monitoring in JS.

Rather than <?php if ( ! $locked ) echo ' style="display:none"'; ?>> could we use a class?

That can use the hidden class. Having a style attribute there optimizes the jQuery(':hidden') check a bit for some browsers.

wp_trash_post() or wp_delete_post() is likely invoked in other situations where it may make sense to block it if the post is locked.

Yes, at least when bulk-trashing posts. This should have been in a separate ticket, will open one tomorrow.

Yeah, the names can be better :) Will switch to using $post and passing it to the filters.

[azaozz]

Replying to nacin:

Avatars may not be from gravatar.com, so the JS code should be removed. If we are going to do nasty things like parse the source URI out of get_avatar(), then we should also un-encode amp; there, not in JS.

Yes, that's needed for displaying avatars in the posts list table too. Was thinking to separate get_avatar() into two functions, one to get the src and another to return the img tag. Then we can specify the encoding of &, etc. easily.

The other part is harder. In JS it's not safe to pass the URL directly. Will look for another way.

[SergeyBiryukov]

Replying to azaozz:

Was thinking to separate get_avatar() into two functions, one to get the src and another to return the img tag. Then we can specify the encoding of &, etc. easily.

Related: #21195

[azaozz]

In [23725]:

Post locks: add Preview button when post is locked, fix the suggested places.

[grahamarmfield]

Replying to azaozz:

In [23725]:

Post locks: add Preview button when post is locked, fix the suggested places.

@azaozz Please could you confirm that you've included the keyboard focus considerations that I mention above in Comment 2?

[azaozz]

In 23733:

Post locks: when a dialog is shown move focus to the text, see #23697

[azaozz]

Replying to grahamarmfield:

Added in [23733]. Is there anything else we should be doing to make these dialogs better?

[grahamarmfield]

Replying to azaozz:

Replying to grahamarmfield:

Added in [23733]. Is there anything else we should be doing to make these dialogs better?

My take on accessibility best practices for these dialogs (and all popup panels) would be:

• When the dialog opens, keyboard focus must be transferred into a meaningful object within the dialog - suggest a heading that explains purpose of dialog. This item can be given tabindex=0 to allow it to receive focus both by scripting and tabbing.
• The purpose of the dialog should always be visible.
• Whilst the dialog is open, functionality will be needed to tidily cope with users tabbing beyond the controls/buttons in the dialog, or reverse tabbing before the entry point. Tabbing can either cycle within dialog if it's important that user responds, or that tabbing outside confines of dialog closes the dialog.
• Whenever dialog is closed, keyboard focus should be returned to somewhere sensible - maybe the link/button that opened dialog in the first place, or to top of a new page if one is opened.

Hope that helps.

[markjaquith]

[23733] introduced odd visual focus glitches:

Chrome:

Firefox:

This item should get focus without a visible outline.

[grahamarmfield]

Replying to markjaquith:

[23733] introduced odd visual focus glitches:

This item should get focus without a visible outline.

I'm afraid I disagree with that. In my view from an accessibility perspective, focus should always be visible wherever possible. By all means tone down the Chrome effect, but the Firefox formatting is fine.

[helen]

Replying to grahamarmfield:

focus should always be visible wherever possible

Even for a plain old paragraph of text? In this instance, that line of text artificially receives focus so it's getting the focus styling when it appears, which is visually ugly and distracting.

[azaozz]

[23733] introduced odd visual focus glitches... This item should get focus without a visible outline.

Wasn't sure what is the proper way here, so left out the outline: 0 for this commit.

As far as I understand focus should always be visible for normally focusable elements, i.e. form elements and links. For elements that normally don't receive focus, the outline visibility depends on the function: if the element is used as a link or a control (i.e. has onclick="some_action", etc.) the focus should be visible. If not, there is no need for the focus to be visible.

This sounds good except when the focus is "invisible" there is one more "tabbing stop" that is undetectable for most users (thinking screen readers would read the focused element's text). In our case pressing tab would go to the first button, pressing shift+tab would go back to the <p> and if there is no outline, sighted users that don't use a mouse will be left wondering where the focus is.

This can be avoided if we use tabindex="-1" (the element is focusable only with JS) but then screen reader users would not be able to go back to the text...

The solution as I see it is to use tabindex="-1" and focus it while containing the tabbing inside the dialog:

• When the dialog is opened, focus goes to the <p>.
• Tabbing moves through the buttons.
• When the next tab stop is outside the dialog, we move the focus to the <p> again.

[azaozz]

In 23763:

Post locks: contain focus inside the dialog when tabbing, remove outline when focus is on the text, see #23697

[azaozz]

In 23697-3.patch​: don't show "post locked" dialogs to superadmins that are not members of the blog or to the users when such superadmin has locked a post.

Needed when a superadmin troubleshoots issues on the Edit Post screen and the user opens the same post.

[azaozz]

In 23981:

Post locks: make sure we never overwrite a draft when it's locked, clean up wp_ajax_autosave() and make wp-refresh-post-lock a bit more robust, see #23697

[azaozz]

In 23697-4.patch:

• Show "Saving revision..." while autosaving after a post lock has been taken over. Change to "Revision saved" when autosave completes.
• Make sure a user exists before using $user->display_name.

[nacin]

I'm not convinced that 23697-3.patch is good default behavior. Rather, there should be a filter here to enable a user to silently look in.

But secondly, why can't read-only mode work?

[azaozz]

Yeah, we were discussing how to best to this. For superadmins that are not users on the blog best seems to have a button to "ignore" the lock. That will put them in a read-only mode. We still would need to show the "Post locked" or "Lock taken over" warnings so they are aware the user has the lock.

Also talked about "incognito" mode (something like 23697-3.patch) but that has the disadvantage of not knowing whether the user has opened the post and/or who has the lock. Will make a new patch soon.

[mattfelten]

On 23697-4.patch​:
What about "Saved as new revision" and bringing it down to the right of the button? It could be set in a smaller font size too to take up less space. That way it's clear what happened to the changes being made.

[azaozz]

What about "Saved as new revision" and bringing it down to the right of the button?

Was thinking the same but in some translations both the button and the text would be quite longer, and the line will wrap under the button making it look pretty bad.

[azaozz]

In [24042]

• Show 'Saving revision...' while autosaving after a post has been taken over. Change to 'Your latest changes were saved as a revision.' when autosave completes.
• Make sure a user exists before using $user->display_name.
• Add 'post_lock_text' action for extending the message text.

[azaozz]

In 23697-5.patch:

• Move nonces refreshing from autosave to lock checking.
• Do autosave only when there is something to save.

[azaozz]

In [24209]:
Post locks and autosave:

• Move nonces refreshing from autosave to lock checking.
• Do autosave only when there is something to save.

[azaozz]

In 24273:

Separate the nonces update from checking the post lock. Fix scheduling the logged out check. See #23697, see #23295.

[azaozz]

It doesn't make sense to check post locks for single installs with only one user. Seems we should do that for multi-site installs too. Superadmins are able to edit or post to all blogs without being a user on the specific blog but that is an exception.

In 23697-6.patch​:

• Change from IDs to classes for the notification-dialog divs so they can be reused.
• Do not check post locks if the dialog's html is not present.
• Do not check post locks if there is only one blog user (for both single and multi-site installs).

[azaozz]

In 24299:

Post locks: do not check locks on the Posts screen if the list table is empty, see #23697

[azaozz]

23697-7.patch is same as 6.patch except it always checks post locks on multisite.

[kirasong]

23697-7.patch​ looks good. This way we still have functioning lock notifications for sites with single user blogs + superadmins.

[azaozz]

In 24304:

Post locks:

• Change from IDs to classes for the notification-dialog divs so they can be reused.
• Do not check post locks if the dialog's html is not present.
• Do not check post locks if there is only one user on a single site install.

See #23697.

[azaozz]

In 23697-8.patch:

• When a post is locked, ensure the 'Go back' button doesn't reload the same screen. If the referrer is the same or there is no referrer, change the button from 'Go back' to 'Go to All Posts'/'Go to All Pages' etc. and load the proper post type screen.
• Rename 'wp-check-locked' to 'wp-check-locked-posts', it's clearer.

[azaozz]

In 24408:

Post locks:

• When a post is locked, ensure the 'Go back' button doesn't reload the same screen. If no referrer, change the button from 'Go back' to 'Go to All Posts'/'Go to All Pages' etc.
• Remove restriction on checking locks only for posts.

See #23697.

[johnbillion]

Related: #24551

[azaozz]

In 24543:

Post locks: load the post locked dialog html in post-new.php too, see #23697.

[nacin]

Per azaozz in IRC, calling this fixed. New tickets for new issues, please.