Opened 4 months ago
Last modified 6 days ago
#23295 new task (blessed)
Improved login expiration warning
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Priority: | normal | Milestone: | 3.6 |
| Component: | Autosave | Version: | |
| Severity: | normal | Keywords: | autosave-redo has-patch ui-feedback |
| Cc: | dh-shredder, mike@…, aaroncampbell, sirzooro, nashwan.doaqan@…, DeanMarkTaylor |
Description
The goal here is to improve the user experience when your login has / will expire, as discussed in WordPress 3.6: Autosave and Post Locking and the autosave and post locking team meeting in IRC.
The suggested tactic is to integrate the PMC Post Savior plugin into core. That part should be pretty straightforward.
I think 3 valuable enhancements that are on the plugin's roadmap, but not yet complete would be:
- Alert properly on failed requests and lost connectivity. If my internet connection has gone down and I'm still working, I should be alerted that I'm "Working offline" and my changes aren't being automatically saved. I don't think this should be a blocking UI, but it should be obvious.
- Block publish/save until login has been verified. Polling duration can be decreased (or maybe even done away with entirely) if we block the Publish/Save/etc actions until we've verified the user's cookie.
- Pre-emptive notification. When approaching the user's login cookie expiration time, say 1 hour before, display a message and allow the user to extend their login. For example, how banking sites notify you when you've been inactive too long and they're about to log you out.
Attachments (15)
Change History (72)
comment:1
mintindeed — 4 months ago
- Type changed from defect (bug) to enhancement
comment:2
dh-shredder — 4 months ago
- Cc dh-shredder added
- Milestone changed from Awaiting Review to 3.6
- Type changed from enhancement to task (blessed)
- Version trunk deleted
comment:5
aaroncampbell — 4 months ago
- Cc aaroncampbell added
comment:7
SergeyBiryukov — 4 months ago
Related: #23216
comment:8
SergeyBiryukov — 4 months ago
- Component changed from Warnings/Notices to Autosave
comment:10
follow-up:
↓ 11
mintindeed — 4 months ago
Right now this login warning only appears on the Edit Post page (and works on custom post types, pages, etc) because that's the most critical point where you really, really don't want to get logged out unexpectedly. But I'm thinking if it's a core feature, maybe it's appropriate for it to work on every admin page?
comment:11
in reply to:
↑ 10
nacin — 4 months ago
Replying to mintindeed:
Right now this login warning only appears on the Edit Post page (and works on custom post types, pages, etc) because that's the most critical point where you really, really don't want to get logged out unexpectedly. But I'm thinking if it's a core feature, maybe it's appropriate for it to work on every admin page?
Given that this can build off the heartbeat API, it can realistically work anywhere. If it is not too much, I would architect this in such a way that it is standalone so it can be used on more than just post.php and post-new.php, even if only those pages will use it for the time being. If 3.6 only supports post.php and post-new.php, that's certainly hitting our goal, though.
comment:12
mintindeed — 4 months ago
It was simpler just to load it everywhere in the admin, so I did that. It would be pretty easy to further extend it to show it on non-admin pages if the user is logged in.
I wrapped it in an object so that it's as self-contained as possible, and to make it easy to override & extend as needed. I'm including the class via wp-settings.php, then adding a "loader" helper function in functions.php, and adding an action to default-filters.php to actually load it. The action runs in "init" so that plugins, themes, etc can unhook it if desired. (Not "admin_init"; the WP_Auth_Check class is responsible for determining that it only wants to run in the admin, this also allows plugins to change when/how it's loaded by overriding WP_Auth_Check::_init().)
Block publish/save until login has been verified. Polling duration can be decreased (or maybe even done away with entirely) if we block the Publish/Save/etc actions until we've verified the user's cookie.
The JS will disable the save/publish/move to trash buttons (the plugin had this behaviour). That's probably fancy enough for now?
Pre-emptive notification. When approaching the user's login cookie expiration time, say 1 hour before, display a message and allow the user to extend their login. For example, how banking sites notify you when you've been inactive too long and they're about to log you out.
We can use the new Heartbeat API from JS to request time left to cookies expiration, or on the PHP side, send notification to the browser one hour before.
The plugin polled every 15 seconds, which is the same duration as the heartbeat, so I'm hooking into the heartbeat API to run the login check. It would be pretty simple to add a message this way; I'll take a stab at that after I make any necessary revisions to the attached patch.
comment:13
alex-ye — 4 months ago
- Cc nashwan.doaqan@… added
mintindeed — 3 months ago
comment:14
mintindeed — 3 months ago
- Keywords has-patch ui-feedback added
Updated patch to address items discussed in #wordpress-dev
The major items are:
- Removed reliance on Thickbox, allows simplification of the JS and looks a little nicer
- Heartbeat response now returns the HTML for the modal dialogue
I've gone back and forth a little bit on how to display the modal dialogue and whether to use an overlay. Second opinions here are welcome. Here's my rationale for what I've done in the patch:
- Disabling critical buttons (save, publish, trash, etc) instead of using an overlay, because one of the more common use-cases is: this dialogue pops up while typing in a post. Not using an overlay seems like it's less jarring in this context, doesn't interrupt typing, and still presents the nice big notice to the user.
- I made the modal dialogue bigger, so it no longer needs to resize to fit the interim login iframe. The "growing" animation is kinda sexy, but a larger dialogue window is a lot more noticeable.
comment:15
azaozz — 3 months ago
In 23295-2.patch:
- Move most html, css and js to the response sent through heartbeat, only few lines of js are loaded initially.
- Move the Close button outside of the wp-login iframe.
- Take out button disabling: not needed with that big message. If we have to do that, logically it would have to be everywhere, not just on the Edit Post screen, and it would be better to add a "blocking" background (so all buttons and links are blocked).
- Tweak opening and closing of the warning: when there are multiple tabs open to the same admin or if the user is superadmin on a network, logging in at one tab would auto-close the warnings everywhere.
comment:16
azaozz — 3 months ago
We've talked with @dh-shredder earlier about how much UI is needed for this. Were wondering if we need the two steps. Currently first we show a warning:
then after clicking the button we show:
How about we skip the warning and show directly a smaller version of the login form's iframe:
That way it's one click only and the text "You will not leave this screen" is not needed (I know this will need some beautification / UI attention).
The login form can look completely different too. There is a body class interim-login that can be used to make it horizontal or show only the username/password fields.
comment:17
mbijon — 3 months ago
+1 for the single, simpler login form.
Updating the message is the only other thing I can think of. The default session expired message seems a bit abstracted from what users are concerned about. Maybe something like "You should not lose any data or changes" would reassure basic users better.
comment:18
helen — 3 months ago
Agree on a single simple form.
comment:19
follow-up:
↓ 21
azaozz — 3 months ago
In 23504:
comment:20
azaozz — 3 months ago
In 23505:
comment:21
in reply to:
↑ 19
;
follow-up:
↓ 22
Archetyped — 3 months ago
Replying to azaozz:
In 23504:
FYI, get_called_class() supported on PHP 5.3+. Fatal error on PHP 5.2.4 (as per WordPress' requirements).
comment:22
in reply to:
↑ 21
azaozz — 3 months ago
FYI, get_called_class() requires PHP 5.3+.
I knew something didn't look right there...
Thanks @nacin for the patch. You're right, no need for a class just to wrap couple of functions. However this will have to work from the front-end for logged in users (things like P2), so it would probably need another PHP function or two.
For now thinking to make is a static class to fix the error, then revisit in the next few days.
comment:23
azaozz — 3 months ago
Actually, nevermind. We only have to split the JS that shows the iframe from heartbeat.js so they can be added separately on the front end if needed. Will revisit in few days in any case.
comment:24
azaozz — 3 months ago
In 23543:
comment:25
azaozz — 3 months ago
In 23295-3.patch:
- Move the initial JS into heartbeat.js.
- Pass is_user_logged_in() through localize_script.
- Tweak the css so it doesn't cut the login form's drop shadow.
comment:26
markoheijnen — 3 months ago
#23655 was marked as a duplicate.
comment:27
markoheijnen — 3 months ago
#23656 was marked as a duplicate.
comment:28
markjaquith — 3 months ago
In 23625:
comment:29
follow-up:
↓ 32
helen — 2 months ago
comment:30
azaozz — 2 months ago
In 23691:
comment:31
azaozz — 2 months ago
In 23692:
comment:32
in reply to:
↑ 29
azaozz — 2 months ago
comment:33
follow-up:
↓ 36
esmi — 2 months ago
Sorry to sounds like Cassandara here but we really need to check that any new solution is accessible to disabled users. The a11y group have some concerns about whether the context changes, on session expiry, will be announced to screen reader users.
comment:34
azaozz — 2 months ago
In 23699:
comment:35
DeanMarkTaylor — 2 months ago
- Cc DeanMarkTaylor added
comment:36
in reply to:
↑ 33
azaozz — 2 months ago
Replying to esmi:
Currently when the dialog is shown it is focused (the <div> has tabindex="0"). Not sure if this is enough, suggestions welcome.
comment:37
azaozz — 2 months ago
Been thinking how to organize this code better. This dialog is shown very rarely, it's not warranted loading the html, css and js on all screens all the time. Currently most of it is sent through heartbeat and injected into the DOM. This is acceptable for testing but not a good idea for production.
Another big concern is that there may be "frame busting" JS on the Log In screen added by a plugin as defence against click-jacking. These JS snippets were quite popular couple of years ago, still see them around although they are not very effective. If that happens, the Log In screen will open instead of the iframe and the user will loose any unsaved changes (and our "You will not move away from this screen" looks really silly).
To work around that we can use an XHR instead of having an iframe (XHRs can be used to set cookies). Another technique is to create an iframe then submit a <form> with target="iframe-name". In this case the main page is not reloaded and cookies are set as usual. The only problem with these approaches is that if there is branding on the Log In screen, it will have to be applied to the "local" log in form, i.e. some themes and/or plugins would need to be updated.
comment:38
azaozz — 2 months ago
In 23295-4.patch:
- Move the JS and CSS into separate files.
- Output the HTML in the footer of each page.
- Add onbeforeunload warning to counter JS redirects.
- Let the user open new tab with the login screen when cross-domain.
comment:39
azaozz — 8 weeks ago
In 23295-5.patch:
- Properly load on the front-end when the user is logged in. This also loads heartbeat and jQuery. Will need a review if we drop front-end jQuery loading for audio/video.
- Focus the iframe or the first line of the text on load. The iframe has a title which would be announced by screen readers.
comment:40
azaozz — 8 weeks ago
In 23805:
comment:41
azaozz — 8 weeks ago
In 23881:
comment:42
ocean90 — 7 weeks ago
23295-docs.patch updates the inline doc for the change in [23881].
comment:43
azaozz — 7 weeks ago
In 23922:
comment:44
azaozz — 4 weeks ago
The current login expiration popup works well but seems unwarranted to check a few times every minute for something that's usually 48 hours away and sometimes up to 336 hours (two weeks) away. In addition it abruptly interrupts the users.
A much better user experience would be to not reach the login expiration time at all. This is not possible at the moment. The next best thing would be to prompt the user to log in in advance on page load, before she/he makes any changes and attempts to save them.
23295-6.patch introduces wp_logout_time() that returns the time left until login expiration and _login_again_early() that redirects to the login screen 30 minutes before expiration.
comment:45
dh-shredder — 3 weeks ago
If we're running heartbeat for other reasons anyway (like updating post locks), then it makes sense to additionally check the login and offer an easy re-login as well, since it's very little additional effort, resource-wise, and is still a better experience than a hard redirection and loss of place in the panel.
Are you arguing we shouldn't use a login popup at all, or just under circumstances where heartbeat wouldn't otherwise be running?
comment:46
azaozz — 3 weeks ago
In 23295-7.patch:
- Don't remove login error messages coming from wp_signon().
- When the login form is shown in iframe, open all links in a new tab/window.
- Add filter for the login form error message.
comment:47
ryan — 2 weeks ago
23295-7.patch looks good to me. I no longer see the administrator message when the cookie expires.
comment:48
azaozz — 2 weeks ago
In 24179:
comment:49
nacin — 2 weeks ago
The commit message for [24179] doesn't mention why the sandbox attribute was removed?
comment:50
azaozz — 2 weeks ago
When set to prevent parent window access, the sandbox attribute also prevents links with target="_blank" (or any other target) from working. They stop functioning completely, behave as normal text.
comment:51
azaozz — 2 weeks ago
In 24208:
comment:52
azaozz — 2 weeks ago
In 24209:
comment:53
azaozz — 2 weeks ago
Uh, that should be #23697.
comment:54
azaozz — 12 days ago
In 23295-9.patch: Fix same domain comparison in wp_auth_check_html() when FORCE_SSL_LOGIN && ! FORCE_SSL_ADMIN.
comment:55
azaozz — 7 days ago
In 24266:
comment:56
azaozz — 7 days ago
In 24271:
comment:57
azaozz — 6 days ago
In 24273:





We can use the new Heartbeat API from JS to request time left to cookies expiration, or on the PHP side, send notification to the browser one hour before.