WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 15 months ago

Last modified 15 months ago

#50686 closed defect (bug) (fixed)

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 needs-testing
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 (12)

ui-issue.png (77.0 KB) - added by dilipbheda 18 months ago.
Before apply patch
after-apply-patch.png (46.5 KB) - added by dilipbheda 18 months ago.
After apply patch
50686.patch (450 bytes) - added by dilipbheda 18 months ago.
empty-custom-link-before-patch.jpg (14.2 KB) - added by sabernhardt 18 months ago.
Custom link inputs with error border, before patch
empty-custom-link-after-patch.jpg (13.9 KB) - added by sabernhardt 18 months ago.
Custom link inputs with error border, after patch
incorrect-datetime-post-classic-editor.jpg (20.5 KB) - added by sabernhardt 18 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 18 months ago.
Add new user: invalid input fields styling
50686 term.php page.png (37.0 KB) - added by afercia 18 months ago.
Edit terms: broken styling
50686.1.patch (1.5 KB) - added by sabernhardt 18 months ago.
using more exclusive CSS selector .form-invalid input:not(.button):not(#custom-menu-item-name) and adding required attribute
50686.diff (3.3 KB) - added by helen 15 months ago.
post-publish-date-invalid-classic.jpg (22.2 KB) - added by sabernhardt 15 months ago.
patch does not apply border to invalid publish time in Classic editor posts/pages
50686.1.diff (5.7 KB) - added by sabernhardt 15 months ago.
removing "form-required" class from custom menu link text input but adding it to 5 date-time fields (for publishing posts/pages/comments)

Download all attachments as: .zip

Change History (35)

@dilipbheda
18 months ago

Before apply patch

@dilipbheda
18 months ago

After apply patch

@dilipbheda
18 months ago

@sabernhardt
18 months ago

Custom link inputs with error border, before patch

@sabernhardt
18 months ago

Custom link inputs with error border, after patch

@sabernhardt
18 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
18 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
18 months 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
18 months ago

Add new user: invalid input fields styling

@afercia
18 months ago

Edit terms: broken styling

@sabernhardt
18 months ago

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

#3 @sabernhardt
18 months 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.


18 months ago

#5 @audrasjb
18 months ago

  • Milestone changed from Awaiting Review to 5.6

Let's move it to milestone 5.6

#6 @audrasjb
18 months ago

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

#7 @dschalk
17 months 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
16 months 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.


16 months ago

#10 @helen
16 months 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.

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


16 months ago

#12 @sabernhardt
16 months ago

  • Focuses javascript added
  • Keywords needs-patch added; has-patch early removed

After discussion during today's #accessibility bug scrub, we decided to wait on punting for another week, in case someone wants to try the JavaScript approach in time for 5.6.

@helen
15 months ago

#13 @helen
15 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

50686.diff is what I think a solution could be given what's currently in place. I tested in a few spots and didn't notice any new breakage. The basic idea here is that when .form-invalid and .form-required are on the same element, which is the generic case currently in places like Edit Category and Add New User, the border continues to be applied to any input and elements contained within that element. The new addition is nesting .form-required inside .form-invalid, which this patch adds to the nav menu screen, and then applying the border only to the .form-required element itself.

The issue on the Edit Category screen where the alert icon is out of place turns out to be due to the p.description underneath, and the fact that we are applying the icon as an ::after on the td instead of the input. I think that's probably due to not previously being styled as inside the input and that you could potentially have multiple inputs/selects inside the td but it might be more correct to attach that icon to the input/select at this point. That would be better addressed in a new ticket, however, since it's not actually a component of this specific issue.

Would like a little more testing for any forms with required elements that I may have missed before committing this.

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


15 months ago

@sabernhardt
15 months ago

patch does not apply border to invalid publish time in Classic editor posts/pages

#15 @sabernhardt
15 months ago

  • Focuses javascript removed
  • Keywords needs-refresh added; needs-testing removed

@helen Thanks for trying the other approach.

With the patch, I noticed that these did not receive the border when one is invalid:

  • post schedule (classic editor) - date/time fields
  • page schedule (classic editor) - date/time fields

These fields do show the border when left empty:

  • new user - both username and email address inputs
  • new menu - name
  • menu custom link - both URL and link text
  • new category - name
  • new tag - name
  • edit category - name
  • edit tag - name

I don't mind that the latest patch applies the red border around the custom link text input, though technically it is not required (that's why the previous patch had the second :not()). If a URL is submitted with empty custom text, the "Menu Item" fallback link text is not very helpful anyway.

Due to the time, this probably should be punted. However, if one :not() is acceptable, I could create a quick patch to add the required attribute to the first patch and let the custom link text receive the border when the URL is empty.

#16 @helen
15 months ago

@sabernhardt I didn't test the classic editor so the classes can be changed there as needed. As for the menu item name, I just had the patch match what was currently there but if it doesn't need to be highlighted because of the default name insertion it's a matter of not adding the .form-required class to it. Not sure if somebody else wants to pick up this patch, otherwise I'll come back to it later.

@sabernhardt
15 months ago

removing "form-required" class from custom menu link text input but adding it to 5 date-time fields (for publishing posts/pages/comments)

#17 @sabernhardt
15 months ago

  • Keywords needs-testing added; needs-refresh removed

I checked the publish date fields with Comments for this patch, in addition to the fields I checked last time.

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


15 months ago

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


15 months ago

#20 @sarahricker
15 months ago

Request for more testing from @sabernhardt

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


15 months ago

#22 @helen
15 months ago

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

In 49283:

Administration: Better targeting for required form field highlighting.

Previously, any input or select inside of a .form-invalid wrapper would get the red border highlighting, including submit buttons which was not visually correct. This now only applies to form elements with a class of .form-required inside of the .form-invalid wrapper. It also continues to apply the border to elements with both classes (.form-invalid.form-required) as that is how some of the admin markup is structured.

Plugin authors may need to do the same sort of class application seen in this commit, i.e. add .form-required to certain form elements.

Props sabernhardt, dilipbheda, helen.
Fixes #50686.

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


15 months ago

Note: See TracTickets for help on using tickets.