WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 5 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:
PR Number:

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

Download all attachments as: .zip

Change History (79)

#1 @afragen
7 months ago

  • Keywords servehappy added

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


7 months ago

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


7 months ago

#4 @desrosj
7 months ago

  • Milestone changed from Awaiting Review to 5.2

It would be great to fix this for 5.2.

@desrosj
7 months ago

Adding screenshot from Slack.

#5 @desrosj
7 months ago

  • Keywords has-screenshots added

#6 @TimothyBlynJacobs
7 months ago

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

@desrosj
7 months ago

@desrosj
7 months ago

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


7 months ago

#9 @afragen
7 months ago

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

Either f332 or f334

@desrosj
7 months ago

#10 @desrosj
7 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
7 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
6 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.


6 months ago

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


6 months ago

#15 @karmatosed
6 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
6 months ago

Updated patch based on design team feedback.

#16 @chetan200891
6 months ago

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

#17 @karmatosed
6 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
6 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
6 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
6 months ago

Updated patch.

#20 @chetan200891
6 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
6 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
6 months ago

Updated patch to create button instead of strip.

#22 @chetan200891
6 months ago

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

#23 @karmatosed
6 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.


6 months ago

@afragen
6 months ago

#25 @afragen
6 months ago

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

#26 @chetan200891
6 months ago

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

#27 @desrosj
6 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
6 months ago

In my screenshot, I have Yoast SEO active.

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


6 months ago

@chetan200891
6 months ago

Updated patch to remove add new icon.

#30 @afercia
6 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.


6 months ago

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


6 months ago

#33 @afragen
6 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
6 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.


5 months ago

#36 @spacedmonkey
5 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
5 months ago

Patch for option-1 example

#37 @afragen
5 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.

Patch also includes some WPCS linting.

Last edited 5 months ago by afragen (previous) (diff)

#38 @spacedmonkey
5 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
5 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.


5 months ago

#41 @afragen
5 months ago

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

@afragen
5 months ago

remove extra code styling fixes

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


5 months ago

#43 @birgire
5 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
5 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
5 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
5 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.


5 months ago

@afragen
5 months ago

don't escape translation, do escape URL

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


5 months ago

#49 @audrasjb
5 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
5 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
5 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.


5 months ago

#52 @SergeyBiryukov
5 months ago

  • Keywords dev-reviewed added; needs-refresh removed

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

#53 @azaozz
5 months ago

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

#54 @audrasjb
5 months ago

  • Keywords commit added

Alright! Thanks for the double sign-off

#55 @azaozz
5 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
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.2 branch.

#57 @azaozz
5 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
5 months ago

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