WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 2 months ago

#37331 closed defect (bug) (fixed)

New site form has non-required fields that have to be filled in

Reported by: Shelob9 Owned by: rianrietveld
Milestone: 5.0 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: required-fields has-patch
Focuses: ui, accessibility, administration, multisite Cc:

Description

In multi-site's New Site form, there are several fields that are not required to submit the form (required and aria-required attributes are not set) but the form will fail server-side validation if these fields, such as admin email are not filled in.

The form really should have client-side and server-side validation for UX and accessibility reasons.

Attachments (5)

37331.diff (3.5 KB) - added by dipesh.kakadiya 19 months ago.
37331-1.diff (3.7 KB) - added by jackreichert 19 months ago.
Building upon @dipesh.kakadiya's patch added note: "Required fields are marked"
37331-2.diff (3.4 KB) - added by jackreichert 19 months ago.
37331-3.diff (3.7 KB) - added by rianrietveld 2 months ago.
adds required asterisk and required attribute
37331.2.diff (3.8 KB) - added by afercia 2 months ago.

Download all attachments as: .zip

Change History (32)

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


22 months ago

#2 @afercia
22 months ago

  • Milestone changed from Awaiting Review to Future Release

Definitely there's a bit of inconsistency across forms in the admin. Many of them should be reviewed. Some years ago an attempt was made to use the HTML5 'required' attribute but seens browsers implementation was a bit poor at that time. See for example [29030] and #22183.

At the very least, I'd say there should be a visual indication of the required fields and aria-required. Further improvements should be carefully evaluated especially thinking at mobile devices, see the discussion on the ticket mentioned above.

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


20 months ago

#5 @rianrietveld
20 months ago

  • Milestone changed from Future Release to 4.7

Discussed this in the wpa11y bug scrub: We'll have a go infixing this for 4.7. together with #16206

#6 @jeremyfelt
20 months ago

  • Keywords needs-patch added

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


20 months ago

This ticket was mentioned in Slack in #core-multisite by flixos90. View the logs.


20 months ago

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


20 months ago

#10 @rianrietveld
20 months ago

  • Owner set to rianrietveld
  • Status changed from new to assigned

#11 follow-up: @rianrietveld
20 months ago

Discussion about how to indicate the required field at: https://wordpress.slack.com/archives/accessibility/p1476119464001821

#12 @afercia
20 months ago

  • Keywords required-fields added

Related: #23870

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


19 months ago

#14 in reply to: ↑ 11 @jackreichert
19 months ago

Replying to rianrietveld:

Discussion about how to indicate the required field at: https://wordpress.slack.com/archives/accessibility/p1476119464001821

I'm not sure the discussion is conclusive, did I understand correctly that it would be best to add sprintf( ' ' . __('Required fields are marked %s'), '<span class="required">⁠⁠⁠</span>' ), or are they likely to circle back to another convention?

Last edited 19 months ago by jackreichert (previous) (diff)

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


19 months ago

#16 @dipesh.kakadiya
19 months ago

  • Keywords has-patch added; needs-patch removed

@jackreichert
19 months ago

Building upon @dipesh.kakadiya's patch added note: "Required fields are marked"

#17 follow-up: @ocean90
19 months ago

novalide="novalidate" was added on purpose. Can you verify that it doesn't reveal the previous issue? See ticket:22183:6 and [20196].

I don't see the need to make '*' translatable (it's currently not even translatable in the description). I'm curious if we can't add the * with CSS ::after.

#18 in reply to: ↑ 17 @jackreichert
19 months ago

Replying to ocean90:

novalide="novalidate" was added on purpose. Can you verify that it doesn't reveal the previous issue? See ticket:22183:6 and [20196].

Good point, I'll put it back.

I don't see the need to make '*' translatable (it's currently not even translatable in the description). I'm curious if we can't add the * with CSS ::after.

I was trying to work with the discussion linked to above, but I couldn't find other places in the admin that use *, in new user it has (required) which would make the translation more relevant. I'm happy to update the patch to either. Would you recommend css or revert to (required)?

#19 @jackreichert
19 months ago

I found that this convention is being used in https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/media.php#L1475 so I updated the patch to reflect that.

#20 @afercia
19 months ago

  • Milestone changed from 4.7 to Future Release

Not enough time to discuss and establish a convention for all the required form fields in core, punting for now.

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


3 months ago

#22 @afercia
3 months ago

  • Keywords needs-refresh added
  • Milestone changed from Future Release to 5.0

@rianrietveld
2 months ago

adds required asterisk and required attribute

#23 @rianrietveld
2 months ago

37331-3.diff​

  • adds the message Required fields are marked * just above the form
  • adds the attribute required="required" to the input field. I did not add the aria-required attribute, as this is already handled and announced by adding the HTML5 required attribute.
  • adds a aria-describedby="site-admin-email" to announce the message below the admin email with the input field.

#24 @rianrietveld
2 months ago

  • Keywords needs-refresh removed

@afercia
2 months ago

#25 @afercia
2 months ago

37331.2.diff adds a translator comment and minor coding standards.

#26 @afercia
2 months ago

Note: the patch keeps novalidate="novalidate" on the form, so the added required attributes will not trigger HTML5 browser validation. I'd tend to think using browser validation is a decision that should be made for all the forms in WordPress, and would probably need some discussion and consensus.

#27 @afercia
2 months ago

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

In 42793:

Accessibility: Networks and Sites: mark the New Site required form fields as required.

Also, adds an aria-describedby attribute to associate the Admin Email field with its description.

Props dipesh.kakadiya, jackreichert, rianrietveld.
Fixes #37331.

Note: See TracTickets for help on using tickets.