WordPress.org

Make WordPress Core

Opened 7 weeks ago

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

Download all attachments as: .zip

Change History (78)

#1 @afragen
7 weeks ago

  • Keywords servehappy added

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


7 weeks ago

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


7 weeks ago

#4 @desrosj
7 weeks ago

  • Milestone changed from Awaiting Review to 5.2

It would be great to fix this for 5.2.

@desrosj
7 weeks ago

Adding screenshot from Slack.

#5 @desrosj
7 weeks ago

  • Keywords has-screenshots added

#6 @TimothyBlynJacobs
7 weeks ago

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

@desrosj
7 weeks ago

@desrosj
7 weeks ago

#7 @desrosj
7 weeks 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 weeks ago

#9 @afragen
7 weeks ago

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

Either f332 or f334

@desrosj
7 weeks ago

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


4 weeks ago

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


4 weeks ago

#15 @karmatosed
4 weeks 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
4 weeks ago

Updated patch based on design team feedback.

#16 @chetan200891
4 weeks ago

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

#17 @karmatosed
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

Updated patch.

#20 @chetan200891
4 weeks 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
4 weeks 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
4 weeks ago

Updated patch to create button instead of strip.

#22 @chetan200891
4 weeks ago

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

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


3 weeks ago

@afragen
3 weeks ago

#25 @afragen
3 weeks ago

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

#26 @chetan200891
3 weeks ago

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

#27 @desrosj
3 weeks 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
3 weeks ago

In my screenshot, I have Yoast SEO active.

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


3 weeks ago

@chetan200891
3 weeks ago

Updated patch to remove add new icon.

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


3 weeks ago

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


3 weeks ago

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


2 weeks ago

#36 @spacedmonkey
2 weeks 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
2 weeks ago

Patch for option-1 example

#37 @afragen
2 weeks 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 2 weeks ago by afragen (previous) (diff)

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


2 weeks ago

#41 @afragen
13 days ago

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

@afragen
13 days ago

remove extra code styling fixes

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


12 days ago

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


10 days ago

@afragen
8 days ago

don't escape translation, do escape URL

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


8 days ago

#49 @audrasjb
8 days 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
8 days 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
8 days ago

Refresh to move translator comment into multi-line printf statement

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


7 days ago

#52 @SergeyBiryukov
7 days ago

  • Keywords dev-reviewed added; needs-refresh removed

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

#53 @azaozz
7 days ago

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

#54 @audrasjb
7 days ago

  • Keywords commit added

Alright! Thanks for the double sign-off

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.2 branch.

#57 @azaozz
7 days 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.

Note: See TracTickets for help on using tickets.