Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#47070 closed defect (bug) (fixed)

Recovery Mode Exit button not visible in responsive view

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

Download all attachments as: .zip

Change History (79)

#1 @afragen
5 years ago

  • Keywords servehappy added

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


5 years ago

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


5 years ago

#4 @desrosj
5 years ago

  • Milestone changed from Awaiting Review to 5.2

It would be great to fix this for 5.2.

@desrosj
5 years ago

Adding screenshot from Slack.

#5 @desrosj
5 years ago

  • Keywords has-screenshots added

#6 @TimothyBlynJacobs
5 years ago

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

@desrosj
5 years ago

@desrosj
5 years ago

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


5 years ago

#9 @afragen
5 years ago

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

Either f332 or f334

@desrosj
5 years ago

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


5 years ago

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


5 years ago

#15 @karmatosed
5 years 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
5 years ago

Updated patch based on design team feedback.

#16 @chetan200891
5 years ago

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

#17 @karmatosed
5 years 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
5 years 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
5 years 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
5 years ago

Updated patch.

#20 @chetan200891
5 years 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
5 years 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
5 years ago

Updated patch to create button instead of strip.

#22 @chetan200891
5 years ago

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

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


5 years ago

@afragen
5 years ago

#25 @afragen
5 years ago

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

#26 @chetan200891
5 years ago

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

#27 @desrosj
5 years 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
5 years ago

In my screenshot, I have Yoast SEO active.

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


5 years ago

@chetan200891
5 years ago

Updated patch to remove add new icon.

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


5 years ago

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


5 years ago

#33 @afragen
5 years 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
5 years 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 years ago

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

Patch for option-1 example

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

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

#41 @afragen
5 years ago

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

@afragen
5 years ago

remove extra code styling fixes

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


5 years ago

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

@afragen
5 years ago

don't escape translation, do escape URL

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


5 years ago

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

#52 @SergeyBiryukov
5 years ago

  • Keywords dev-reviewed added; needs-refresh removed

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

#53 @azaozz
5 years ago

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

#54 @audrasjb
5 years ago

  • Keywords commit added

Alright! Thanks for the double sign-off

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

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for the 5.2 branch.

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

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