Recovery Mode Exit button not visible in responsive view

[afragen]

As @afercia noted the Recovery Mode Exit button is not visible in responsive mode. ​https://wordpress.slack.com/archives/C60K3MP2Q/p1556206594281500

Initial suggestions were to add a link to the User Account menu.

[desrosj]

It would be great to fix this for 5.2.

[desrosj]

Adding screenshot from Slack.

[TimothyBlynJacobs]

Even easier would be to perhaps add an exit button to that notice in the responsive view?

[desrosj]

For option 1, there would need to be CSS hiding that link if the button in the admin bar is visible. The wording is a bit repetitive, but it will not introduce a new string.

Option 2 seems like the best option. But, there is a point where the button does not fit and will bump down (less than 353 pixels wide).

[afragen]

@schlessera had a thought about using one of the 2 shield dashicons when in responsive mode.

Either f332 or f334

[desrosj]

Played with this a bit more today. 47070.diff​ is a first pass.

The dropdown menu needs to not show on desktop, and needs to be full width (similar to the Posts dropdown) on mobile.

Unless someone is available to provide some design feedback, my gut says to punt this and tackle in 5.2.1, just to make sure it receives the right level of attention.

[desrosj]

This is not quite ready. Let's address in 5.2.1.

[desrosj]

I'm moving this to 5.2.2. This still needs design feedback (this was my fault for assigning the old needs-design keyword to the ticket and not the new needs-design-feedback keyword).

[karmatosed]

This was discussed in today's design triage. It would be great to avoid having to add an icon such as a shield or hide the action under a drop down. The ideal would be to remove icons on smaller screens, for example, is that needed in recovery mode? Maybe even remove the avatar? It certainly doesn't need added icons from plugins showing. Even just the menu would work.

Failing that, an alternative that's not great but an option if removing icons isn't possible, that's to have a strip under the admin bar. This is better than adding in a new icon for this.

[chetan200891]

Updated patch based on design team feedback.

[chetan200891]

@karmatosed I have updated patch 47070.2.diff based on your suggestion. I have attached screenshot too.

[karmatosed]

Thanks for this @chetan200891, is there any reason why we can't just remove icons and not have the strip? To be clear the strip was suggested as a fallback, sorry if it seemed like it was the ideal.

[chetan200891]

There is no desirable room on admin bar. This is clean install but many plugins add icons there and some of them are important to have. So I have created it as strip.

[karmatosed]

Thanks for the context @chetan200891. It's a tight space that's for sure. I wonder though if we need to show all those icons? What if we didn't? Do people need to see those in recovery mode?

[chetan200891]

Updated patch.

[chetan200891]

@karmatosed I understand fixing site is more important than other stuff while in recovery mode. So I have updated patch and added Exit Recovery Mode strip on admin bar. Dashboard and profile icon will be still there. I have attached screenshot.

[karmatosed]

Thank you so much for the changes, I appreciate you taking this time @chetan200891. One little note I think would perhaps help this would not to have it full width. Could it just go back to being a button width like on desktop? Apologies for asking for that change, but I think it would be the balance here.

[chetan200891]

Updated patch to create button instead of strip.

[chetan200891]

@karmatosed I have updated patch to create as button instead of full width strip. I have attached screenshot.

[karmatosed]

@chetan200891 thank you so very much for working through this and all your hardwork. From a design perspective going to sign this off and remove the feedback label.

[afragen]

Seems like there's an issue when using an older iPhone5/SE

[chetan200891]

@afragen Thanks for checking. Can you tell me which kind of issue are you getting?

[desrosj]

In addition to the issues @afragen is seeing with the + icon being covered, there are also issues with any plugin that adds a menu icon on newer screens. I don't necessarily think Core can account for this, but wanted to mention it.

Thinking through the flow a bit. If a user is logged in while the site is in recovery mode, it means they have clicked a link that there is an issue with their site in an email. So the user is most likely (or should be) trying to investigate and fix the issue. With that in mind, I think the plus icon is also not necessary when in recovery mode. Users probably should not be creating content while in recovery mode.

I think if we also hide the + icon, then this is good to go. Plugins should handle their menus however they want.

[desrosj]

In my screenshot, I have Yoast SEO active.

[chetan200891]

Updated patch to remove add new icon.

[afercia]

Quick screenshot (see below), to remind that plugins can add their menus also on the right of the sidebar. Tested with the "View Admin As" plugin, and I guess there are other plugins that do the same.

Placing the "Recovery Mode Exit" on the right on small screens would likely break the admin bar layout.

WordPress never established strict rules for what plugins can add in the admin menus so basically there's nothing to prevent plugins to do what they want other than a few CSS rules that are very easy to override. The related hooks can't be disabled because they're used by WordPress itself. Relying on CSS doesn't seem a good idea as well.

I'd tend to think we should explore a new, reliable, way to conditionally render menus in the admin bar.

Not sure if it's possible (just brainstorming and would appreciate any feedback), what about to:

• introduce a new hook(s) exclusively meant to print out the Exit Recovery Mode link and the other WordPress menus that should be available in Recovery Mode
• when in Recovery Mode, disable all the hooks like admin_bar_menu

[afragen]

On mobile, it might be worthwhile to use remove_all_actions( 'admin_bar_menu' ) when in recovery mode so no other admin bar items are present.

Thoughts?

[afragen]

The above really doesn't solve the issue either as some plugins insert admin menus by other means, I'm looking at Query Monitor.

[spacedmonkey]

After this week's dev chat, it was decided to work on adding exit recovery mode to the notice. This is the cleanest way, as modifying the admin bar with css, is not workable in the long term.

It is unlikely that this will make it's way to into 5.2.2, but I am hoping 5.2.3.

Assigning to afragen to own.

[afragen]

Patch for option-1 example

[afragen]

47070-option-1.diff​ adds the Exit Recovery Mode link as in

It does not remove the Exit Recovery Mode button from the admin bar.

Patch also includes some WPCS linting.

[spacedmonkey]

Why not just change the wording.
"To exit recovery mode, log out (link to logout) or click this link to exit recovery mode. (link to exit recovery mode)".

[afragen]

@spacedmonkey I thought the discussion from last week was to simply add the link text back as in the @desrosj’s original mockup. Also, won’t rephrasing the notice cause translation issues with new strings?

[afragen]

I’ll update the patch tomorrow and remove the extra WPCS code styling.

[afragen]

remove extra code styling fixes

[birgire]

In 47070-option-1.2.diff​ I would suggest using esc_url( $url ) in printf(). There's the esc_html() on the output within wp_nonce_url() but I think we should use the correct url context here. There are also some existing cases in core with esc_url( wp_nonce_url( ... ) ).

I don't recall seeing wp_kses_post() used on translation, even though it contains HTML.

[afragen]

@birgire I was trying to determine what to use to escape the translation. Much of core doesn’t escape translations, but I’m not certain that’s a best practice. I’m curious to see what others have to say.

[birgire]

Replying to afragen:

@birgire I was trying to determine what to use to escape the translation. Much of core doesn’t escape translations, but I’m not certain that’s a best practice. I’m curious to see what others have to say.

I understand your concerns.

Here @SergeyBiryukov shed some light on the matter recently:

https://core.trac.wordpress.org/ticket/47385#comment:1

with some references to previous discussions and noting that core translations are considered trusted.

[afragen]

I can update the patch accordingly, but won’t be able to until Tuesday or so.

[afragen]

don't escape translation, do escape URL

[audrasjb]

Hi there!

Do you think we could have a consensus (and a final patch) for this ticket before 5.2.2 RC2 (scheduled tomorrow around 18:00 UTC)? If not, we'll punt it to the next release :-)

Cheers,
Jb

[afragen]

It looks like I need to move the translator comment. I’ll try to get to this or it can be fixed on commit.

[garrett-eclipse]

Refresh to move translator comment into multi-line printf statement

[SergeyBiryukov]

47070-option-1.4.diff​ looks good to me.

[azaozz]

47070-option-1.4.diff looks good here too.

[audrasjb]

Alright! Thanks for the double sign-off

[azaozz]

In 45529:

Add a Recovery Mode Exit button to the notice. This fixes it in responsive view.

Props desrosj, karmatosed, chetan200891, afercia, afragen, garrett-eclipse.
Fixes #47070.

[azaozz]

Reopen for the 5.2 branch.

[azaozz]

In 45530:

Add a Recovery Mode Exit button to the notice. This fixes it in responsive view.

Props desrosj, karmatosed, chetan200891, afercia, afragen, garrett-eclipse.
Merges [45529] to the 5.2 branch.
Fixes #47070.