WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 6 days ago

#50686 accepted defect (bug)

UI issue on nav-menus.php

Reported by: dilipbheda Owned by: audrasjb
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch early
Focuses: ui, accessibility, css, administration Cc:

Description

button border color issue, if users click on create menu button without enter the menu name.

STEP:
1) Go to Appearance -> Menus
2) Click on create menu button without enter the menu name.

I have attached an image for better understanding.

Attachments (9)

ui-issue.png (77.0 KB) - added by dilipbheda 3 months ago.
Before apply patch
after-apply-patch.png (46.5 KB) - added by dilipbheda 3 months ago.
After apply patch
50686.patch (450 bytes) - added by dilipbheda 3 months ago.
empty-custom-link-before-patch.jpg (14.2 KB) - added by sabernhardt 2 months ago.
Custom link inputs with error border, before patch
empty-custom-link-after-patch.jpg (13.9 KB) - added by sabernhardt 2 months ago.
Custom link inputs with error border, after patch
incorrect-datetime-post-classic-editor.jpg (20.5 KB) - added by sabernhardt 2 months ago.
the OK buttons for post and comment dates correctly do not have the red border when the date/time is invalid (with or without the patch)
50686 new user.png (50.2 KB) - added by afercia 8 weeks ago.
Add new user: invalid input fields styling
50686 term.php page.png (37.0 KB) - added by afercia 8 weeks ago.
Edit terms: broken styling
50686.1.patch (1.5 KB) - added by sabernhardt 7 weeks ago.
using more exclusive CSS selector .form-invalid input:not(.button):not(#custom-menu-item-name) and adding required attribute

Download all attachments as: .zip

Change History (19)

@dilipbheda
3 months ago

Before apply patch

@dilipbheda
3 months ago

After apply patch

@dilipbheda
3 months ago

@sabernhardt
2 months ago

Custom link inputs with error border, before patch

@sabernhardt
2 months ago

Custom link inputs with error border, after patch

@sabernhardt
2 months ago

the OK buttons for post and comment dates correctly do not have the red border when the date/time is invalid (with or without the patch)

#1 @sabernhardt
2 months ago

  • Focuses accessibility added
  • Version trunk deleted

@dilipbheda Thanks for the patch!

50686.patch successfully removes the extra red border from the "Create Menu" submit button when the menu name is empty. Likewise, that border is no longer on the "Add to Menu" button when a Custom Link's URL field is left empty. The date/time selection fields for posts and comments can also be inside the container with a form-invalid class, but the OK buttons are not within the container (so those do not need fixing).

However, the border is still on the Custom Link's "Link Text" input though the error only relates to the empty URL field. Technically, the selectors could have another :not() to make .form-invalid input:not(.button):not(#custom-menu-item-name), yet there probably is a better option.

I added the accessibility focus so the team could consider more substantial feedback for these errors, appearing in close proximity to the inputs. If that's outside the scope of this ticket, it could be considered on another.

I also noticed that the extra border occurred in a version earlier than trunk (5.4.2).

#2 @afercia
8 weeks ago

Related: [33109] / #32490 where a "\f534" dashicon was added to the styling of the invalid input fields. See for example the Add New User page. Seems that change missed various input fields in the admin that use the same styling. Also, in some pages the icon is completely off place, see for example the term.php page in the screenshot below.

The validateForm() function and similar implementations in core that set the form-invalid CSS class and only add a visual feedback are very old bits of code and show their age.

I'm not sure it is worth spending time to make validateForm() more accessible. A solid, comprehensive, user input validation API (see #47505) would require many more things than a border and some ARIA attributes. First of all, it would require a standardized ways to generate the input fields, see the Fields API project, which is in a dormant state. It should be used for the Settings API first and then made available to plugins and themes. These are big features that historically WordPress misses and would need some big effort. Hopefully they will get prioritized, at some point.

@afercia
8 weeks ago

Add new user: invalid input fields styling

@afercia
8 weeks ago

Edit terms: broken styling

@sabernhardt
7 weeks ago

using more exclusive CSS selector .form-invalid input:not(.button):not(#custom-menu-item-name) and adding required attribute

#3 @sabernhardt
7 weeks ago

Thanks @afercia for looking into this. So it's probably not worth a big fix just for the menu pages.

Then I propose these smaller changes here:

  1. editing the CSS selector to be stricter
  2. adding the required attribute to the Menu name input (so at least the browser can give its usual notification)

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


7 weeks ago

#5 @audrasjb
7 weeks ago

  • Milestone changed from Awaiting Review to 5.6

Let's move it to milestone 5.6

#6 @audrasjb
7 weeks ago

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

#7 @dschalk
5 weeks ago

[I have tested this patch and can confirm it works as expected.
The problem where buttons incorrectly got the red form-invalid styling seems solved.

After patch:
https://www.dropbox.com/s/zi7w2o748igvzoq/Screenshot%202020-08-28%20at%2012.10.08.png
https://www.dropbox.com/s/he4tv2ycv7486c9/Screenshot%202020-08-28%20at%2012.09.53.png

I have performed a general smoke test to see if I could find any other forms with buttons that had issues but have not found any.

The styling issues mentioned by @afercia are not fixed by this patch, I would propose to create a new issue for these.

#8 @audrasjb
11 days ago

  • Keywords commit early added

Indeed, let's commit the current patch and open a new one to handle styling issue.
I'll open a new ticket as soon as this one is fixed, and I'd like to milestone it for 5.6, so I'm also adding early keyword.

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


6 days ago

#10 @helen
6 days ago

  • Keywords commit removed

I am famous for doing this but I am also very sorry in advance for it :) I think unfortunately in this case we do need to go at least a little bit wider for a fix that doesn't put us into a difficult place later. I don't think we need to go all the way into fixing validateForm from an accessibility point of view for the purposes of fixing this bug, but I think the correct approach here would not to be add a bunch of specific :not selectors to the CSS, but rather to add .form-required to the appropriate form inputs and use that existing naming.

That said, because it's existing naming and affects both CSS and JS, going that route will need a lot of testing. I think the initial approach for the menus screen would be to add .form-required to the specific inputs that should be highlighted (not on a parent element) and add .form-invalid .form-required { border } to forms.css. From there, you would need to test that validateForm doesn't interfere with menus because of the way it works, and that the new CSS doesn't adversely affect other elements that already use .form-required.

Does that make sense? I want to be clear that the patch isn't *bad*, it's just that I see this style of change as a bit of a red flag especially when it comes to trying to understand why it's this way in the future.

Note: See TracTickets for help on using tickets.