WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 2 months ago

#51631 closed enhancement (fixed)

Refactor Layout of the Menu screens to avoid dual-buttons and responsive issues.

Reported by: garrett-eclipse Owned by: audrasjb
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility, css, administration Cc:

Description

This ticket is split from #49576 to further investigate improvements on the responsive layout of the Menu screens, and hopefully avoid dual primary buttons in close proximity.

Quoting specific discussion points below.

comment#16 by @afercia:

When a menu with items nested by 2-3 levels is set, this page responsive behavior is pretty different and basically breaks for viewport widths between about 950 / 782 pixels. This is a pre-existing issue unrelated to this patch. Just to note that this page has more serious, long-standing, layout issues that would require a major refactoring. In a separate ticket :)

slackComment by @helen:

Why are there two Create Menu buttons next to each other like that?

slackDiscussion from @kburgoine

This was discussed during the #core-css triage meeting and the question was asked if we needed to keep both Create Menu buttons? Or if we could remove one?

Attachments (15)

Capture d’écran 2020-11-06 à 16.43.02.png (108.8 KB) - added by audrasjb 5 months ago.
Two buttons issue / 1
Capture d’écran 2020-11-06 à 16.42.47.png (170.8 KB) - added by audrasjb 5 months ago.
Two buttons issue / 2
51631.1.diff (573 bytes) - added by maxpertici 4 months ago.
sticky-on-header
51631.1-sticky-on-header.png (149.1 KB) - added by maxpertici 4 months ago.
sticky-on-header
51631.2.diff (558 bytes) - added by maxpertici 4 months ago.
sticky-on-footer
51631.2-sticky-on-footer.png (118.6 KB) - added by maxpertici 4 months ago.
sticky-on-footer
51631-large-screens.gif (3.5 MB) - added by audrasjb 3 months ago.
51631.2.diff – on large screens
51631-med-screens.gif (3.5 MB) - added by audrasjb 3 months ago.
51631.2.diff – on medium screens
51631-small-screens.gif (1.3 MB) - added by audrasjb 3 months ago.
51631.2.diff – on small screens
expanded-menu-item-overlaps.png (13.7 KB) - added by sabernhardt 3 months ago.
z-index issue with expanded menu item
menu-sticky-covered-input.png (18.6 KB) - added by sabernhardt 3 months ago.
footer covers checkboxes (and their label) even when in focus
51631.3.diff (825 bytes) - added by maxpertici 3 months ago.
51631.3.png (164.4 KB) - added by maxpertici 3 months ago.
51631.4.diff (1.7 KB) - added by maxpertici 2 months ago.
footer sticky + js shift on focus
test-51631.4.diff.gif (1.5 MB) - added by maxpertici 2 months ago.
Test 51631.4.diff

Change History (36)

#1 @kburgoine
6 months ago

During last weeks #core-css triage, we discussed this issue briefly again in relation to #49576 and there was a question raised around whether we could do something purely with CSS that could fix this

Question for CSS gurus, is there a css way to stick the bottom one at the bottom of the container until the container bottom is out of view then have it at bottom of view. Or is JS the only route for that stuff still

https://wordpress.slack.com/archives/CQ7V4966Q/p1603398632334500

I've spent some time looking into this and can't find a way to do this purely with CSS (maybe someone else can? if so please share, I would love to know!). CSS doesn't have a way to calculate the height of a given container. You could use 100vh for the height of the viewport but, to know if the container displays entirely on the screen you would also have to know the height of the header area and the height of the container itself.

For example, if you knew the exact height of the page header area (in this case, shows the edit menu's and menu location tabs as well as the select box to choose a menu to edit) was 35vh and you also knew that the specific container for the menu items was 75vh then you could use something along the lines of a SASS function to add those two heights together and if the total is greater than 100, set a specific behaviour.

However, we can't determine any of those values because the header area height will change responsively and could have a larger height on smaller screens, where items have to stack. The size of the container also depends on the number of menu items added to it. We would need Javascript to determine those values for us, and once you start using javascript there is probably a much more elegant solution.

We could add some javascript here, it wouldn't be too difficult, but is there then a question of over-engineering?

In my mind, the simplest solution is to remove the bottom button when it is a create menu button and keep it there when it is a save menu button.

It's not a perfect solution because when you have first created your menu until you start adding menu items there are still two buttons in close proximity, but as you add menu items it does at least make sense as to why the bottom button is also there. Thoughts?

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


5 months ago

@audrasjb
5 months ago

Two buttons issue / 1

@audrasjb
5 months ago

Two buttons issue / 2

#3 @audrasjb
5 months ago

  • Keywords needs-patch has-screenshots added
  • Milestone changed from Awaiting Review to 5.7

As per today's accessibility bug scrub, the second "Create menu" button doesn't looks necessary and can be removed.
But the redundant "Save menu" button can be useful when there is a lot of menu items.

Moving to milestone 5.7.

#4 @garrett-eclipse
5 months ago

  • Keywords needs-design added

IMO I'd suggest keeping the button at the bottom as it provides a more logical form flow (actions after inputs). For the create screen definitely drop the top one.

For the Save screen am wondering if we could use JS to fix the bottom actions to always be visible but keep them in their container if you scroll past. This would allow us to also be consistent between the two views, removing the top button.

If JS isn't an option maybe we suppress the top button when there's less than X menu items. So suppressed on create and on save when say only 3 menu items available.

Thoughts? Added needs-design to get input from design on the approach.

This ticket was mentioned in Slack in #core-css by kirstyburgoine. View the logs.


5 months ago

@maxpertici
4 months ago

sticky-on-header

@maxpertici
4 months ago

sticky-on-header

@maxpertici
4 months ago

sticky-on-footer

@maxpertici
4 months ago

sticky-on-footer

#6 follow-up: @maxpertici
4 months ago

Hello,

From my point of view, it is possible with just a little bit of css to keep the buttons visible despite the page height. I offer two patches for example with sticky positions on the header menu (51631.1.diff) or the footer menu (51631.2.diff).
These two patches use @supports to test the use of the css rule and also hide the unwanted button.

I understand that keeping the footer button is logical but I find that the sticky footer menu "hides" the rest of the menu and the rest of the page. It is less obvious that settings are at the bottom like the menu locations.

#7 @audrasjb
4 months ago

  • Owner set to audrasjb
  • Status changed from new to reviewing

Self assigning for 5.7 consideration

#8 @hedgefield
3 months ago

  • Keywords needs-design removed

Thanks for the designs @maxpertici, those look like good options! I don't have an outspoken favorite, but I think the sticky bottom makes most sense as it also has the Delete option, which otherwise is kind of hard to discover.

@audrasjb
3 months ago

51631.2.diff – on large screens

@audrasjb
3 months ago

51631.2.diff – on medium screens

@audrasjb
3 months ago

51631.2.diff – on small screens

#9 @audrasjb
3 months ago

  • Keywords has-patch commit added; needs-patch removed
  • Status changed from reviewing to accepted

I widely tested 51631.2.diff and it looks like a great enhancement 👍

I'd lean towards committing this proposal asap in the alpha cycle, to make sure to give some time to be widely tested. On my side the proposal looks really good (see screenshots above). Plus, it was validated by the design team.

Marking this for commit.

#10 @poena
3 months ago

I have tested 51631.2.diff and it works well for me.

@sabernhardt
3 months ago

z-index issue with expanded menu item

@sabernhardt
3 months ago

footer covers checkboxes (and their label) even when in focus

#11 in reply to: ↑ 6 @sabernhardt
3 months ago

  • Keywords commit removed

Replying to maxpertici:

I understand that keeping the footer button is logical but I find that the sticky footer menu "hides" the rest of the menu and the rest of the page. It is less obvious that settings are at the bottom like the menu locations.

I might like having a sticky footer, but I've also used this screen often and know my options well. It's probably not so friendly to new users.

Is making the footer sticky enough of a benefit? If it is sticky, the z-index needs to be more than 10 to stay above the expanded menu items.

When navigating with the keyboard, the footer can cover the checkboxes, even when they are in focus. While the location options can be hidden in at least Chrome and Firefox, Chrome tends to scroll down to show them more often than Firefox.

Also, we avoided introducing a breakpoint of 783 pixels in wp-admin last year (ticket:49576#comment:26 and ticket:48780#comment:19). On the other hand, the block editor uses that breakpoint now.

I definitely prefer hiding/removing the top button and keeping the bottom one, especially in the case of a large menu combined with slow hosting and/or connection speed. (Saving a menu before all items load removes the unloaded items without a way to revert changes.)

@maxpertici
3 months ago

@maxpertici
3 months ago

#12 @maxpertici
3 months ago

I propose a new patch (51631.3.diff) to take into account the remarks of @sabernhardt : the z-index and keyboard navigation.

I also added a shadow to make the footer stand out when it is above the other elements.

After thinking about new users, I tell myself that if the menu is new or short, all options will be visible.

I have no opinion to share.

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


3 months ago

#14 @ryokuhi
3 months ago

  • Keywords needs-testing added

#15 @ryokuhi
3 months ago

This ticket was discussed today during the weekly accessibility bug scrub.
The new patch needs a few more testing and possibily some testing using assistive technologies, but if everything is fine it can be committed.

Last edited 3 months ago by ryokuhi (previous) (diff)

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


3 months ago

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


3 months ago

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


2 months ago

#19 @maxpertici
2 months ago

After a few tries, scroll-margin-bottom introduced in patch 51631.3.diff is not a solution.
I don't know if it's possible to do without javascript to avoid keyboard navigation problems.
I am sending you a new patch where I added a little javascript to force a shift if the element with the focus is at the bottom of the page. 51631.4.diff

@maxpertici
2 months ago

footer sticky + js shift on focus

@maxpertici
2 months ago

Test 51631.4.diff

#20 @joedolson
2 months ago

  • Keywords commit added; needs-testing removed

Thanks, @maxpertici!

#21 @joedolson
2 months ago

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

In 50115:

Menus: Add sticky footer to avoid duplicate save buttons.

Make the navigation menu footer sticky so a Save Menu button is always available in the viewport. Improves usability and effectiveness of the interface when in a responsive view.

Props garrett-eclipse, audrasjb, maxpertici, sabernhardt, kburgoine, poena
Fixes #51631

Note: See TracTickets for help on using tickets.