WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 13 months ago

Last modified 13 months ago

#47070 closed defect (bug) (fixed)

Recovery Mode Exit button not visible in responsive view

Reported by: afragen Owned by: afragen
Milestone: 5.2.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-screenshots has-patch dev-reviewed commit
Focuses: ui, administration Cc:

Description

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.

Attachments (21)

Screenshot 2019-04-25 at 17.26.48.png (86.9 KB) - added by desrosj 15 months ago.
Adding screenshot from Slack.
option-1.png (64.0 KB) - added by desrosj 15 months ago.
option-2.png (57.9 KB) - added by desrosj 15 months ago.
47070.diff (2.0 KB) - added by desrosj 15 months ago.
Screen Shot 2019-05-02 at 8.20.29 AM.png (123.7 KB) - added by desrosj 15 months ago.
Screen Shot 2019-05-02 at 8.20.21 AM.png (113.6 KB) - added by desrosj 15 months ago.
2019-05-20 at 17.46.png (88.9 KB) - added by karmatosed 14 months ago.
47070.2.diff (2.0 KB) - added by chetan200891 14 months ago.
Updated patch based on design team feedback.
Recovery-Mode-Mobile.png (74.2 KB) - added by chetan200891 14 months ago.
47070.3.diff (1.8 KB) - added by chetan200891 14 months ago.
Updated patch.
Recovery-Mode-Mobile-Updated.png (74.4 KB) - added by chetan200891 14 months ago.
47070.4.diff (1.8 KB) - added by chetan200891 14 months ago.
Updated patch to create button instead of strip.
Recovery-Mode-Mobile-Updated-Button.png (74.5 KB) - added by chetan200891 14 months ago.
iPhone5-SE.png (59.3 KB) - added by afragen 14 months ago.
Screen Shot 2019-05-28 at 9.53.00 AM.png (82.5 KB) - added by desrosj 14 months ago.
47070.5.diff (1.9 KB) - added by chetan200891 14 months ago.
Updated patch to remove add new icon.
01 Screenshot 2019-05-29 at 08.43.39.png (73.8 KB) - added by afercia 14 months ago.
47070-option-1.diff (2.0 KB) - added by afragen 13 months ago.
Patch for option-1 example
47070-option-1.2.diff (966 bytes) - added by afragen 13 months ago.
remove extra code styling fixes
47070-option-1.3.diff (950 bytes) - added by afragen 13 months ago.
don't escape translation, do escape URL
47070-option-1.4.diff (952 bytes) - added by garrett-eclipse 13 months ago.
Refresh to move translator comment into multi-line printf statement

Download all attachments as: .zip

Change History (79)

#1 @afragen
15 months ago

  • Keywords servehappy added

This ticket was mentioned in Slack in #core-php by afragen. View the logs.


15 months ago

This ticket was mentioned in Slack in #core by desrosj. View the logs.


15 months ago

#4 @desrosj
15 months ago

  • Milestone changed from Awaiting Review to 5.2

It would be great to fix this for 5.2.

@desrosj
15 months ago

Adding screenshot from Slack.

#5 @desrosj
15 months ago

  • Keywords has-screenshots added

#6 @TimothyBlynJacobs
15 months ago

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

@desrosj
15 months ago

@desrosj
15 months ago

#7 @desrosj
15 months ago

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).

This ticket was mentioned in Slack in #core by chanthaboune. View the logs.


15 months ago

#9 @afragen
15 months ago

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

Either f332 or f334

@desrosj
15 months ago

#10 @desrosj
15 months ago

  • Keywords needs-design added

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.

#11 @desrosj
15 months ago

  • Keywords has-patch needs-refresh added; needs-patch removed
  • Milestone changed from 5.2 to 5.2.1

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

#12 @desrosj
14 months ago

  • Keywords needs-design-feedback added; needs-design removed
  • Milestone changed from 5.2.1 to 5.2.2

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).

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


14 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


14 months ago

#15 @karmatosed
14 months ago

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
14 months ago

Updated patch based on design team feedback.

#16 @chetan200891
14 months ago

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

#17 @karmatosed
14 months ago

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.

#18 @chetan200891
14 months ago

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.

#19 @karmatosed
14 months ago

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
14 months ago

Updated patch.

#20 @chetan200891
14 months ago

  • Focuses ui administration added

@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.

#21 @karmatosed
14 months ago

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
14 months ago

Updated patch to create button instead of strip.

#22 @chetan200891
14 months ago

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

#23 @karmatosed
14 months ago

  • Keywords needs-design-feedback removed

@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.

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


14 months ago

@afragen
14 months ago

#25 @afragen
14 months ago

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

#26 @chetan200891
14 months ago

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

#27 @desrosj
14 months ago

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.

#28 @desrosj
14 months ago

In my screenshot, I have Yoast SEO active.

This ticket was mentioned in Slack in #accessibility by chetan200891. View the logs.


14 months ago

@chetan200891
14 months ago

Updated patch to remove add new icon.

#30 @afercia
14 months ago

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


14 months ago

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


14 months ago

#33 @afragen
14 months ago

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?

#34 @afragen
14 months ago

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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


14 months ago

#36 @spacedmonkey
14 months ago

  • Owner set to afragen
  • Status changed from new to assigned
  • Version set to 5.2

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
13 months ago

Patch for option-1 example

#37 @afragen
13 months ago

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

https://core.trac.wordpress.org/raw-attachment/ticket/47070/option-1.png

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

Version 0, edited 13 months ago by afragen (next)

#38 @spacedmonkey
13 months ago

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)".

#39 @afragen
13 months ago

@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?

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


13 months ago

#41 @afragen
13 months ago

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

@afragen
13 months ago

remove extra code styling fixes

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


13 months ago

#43 @birgire
13 months ago

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.

#44 follow-up: @afragen
13 months ago

@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.

#45 in reply to: ↑ 44 @birgire
13 months ago

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.

#46 @afragen
13 months ago

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

This ticket was mentioned in Slack in #core-php by spacedmonkey. View the logs.


13 months ago

@afragen
13 months ago

don't escape translation, do escape URL

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


13 months ago

#49 @audrasjb
13 months ago

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

#50 @afragen
13 months ago

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
13 months ago

Refresh to move translator comment into multi-line printf statement

This ticket was mentioned in Slack in #core by justinahinon. View the logs.


13 months ago

#52 @SergeyBiryukov
13 months ago

  • Keywords dev-reviewed added; needs-refresh removed

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

#53 @azaozz
13 months ago

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

#54 @audrasjb
13 months ago

  • Keywords commit added

Alright! Thanks for the double sign-off

#55 @azaozz
13 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#56 @azaozz
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.2 branch.

#57 @azaozz
13 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#58 @spacedmonkey
13 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.