WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#42022 closed defect (bug) (fixed)

Customize: Fix date time control defects

Reported by: melchoyce Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: trunk
Component: Customize Keywords: has-patch
Focuses: ui Cc:

Description (last modified by westonruter)

See attached image. “Day” should be replaced with “Date”.

Located in "Customize" —> click the cog next to "Publish" —> click "Schedule"

  • The control should be able to be re-used for multiple settings (currently it can't).
  • Changes to the setting should sync into the control.
  • The defaultValue param should be replaced with using the value that is provided to the setting when instantiating (see #37964).
  • The time portion the control should be able to be optional.
  • The maxlength is not appropriate for number inputs.
  • “form elements label are not explicitly associated with for/id attributes as required by the accessibility standard for any new code in core.” (via @afercia)
  • “the preview link contains a few error that are easily catchable just validating the HTML, most notably: a label element can contain just one labelable element” (via @afercia)
  • “In the template, there are a couple inputs with attributes with a double quote at the end” (via @afercia)

Attachments (13)

screen_shot_2017-09-28_at_18.39.07_480.png (21.8 KB) - added by melchoyce 3 weeks ago.
42022.diff (19.5 KB) - added by afercia 3 weeks ago.
42022.2.diff (26.2 KB) - added by westonruter 3 weeks ago.
https://github.com/xwp/wordpress-develop/pull/266/files/f23454c..211219c
42022.3.diff (37.9 KB) - added by westonruter 3 weeks ago.
https://github.com/xwp/wordpress-develop/pull/266/files/0310de9..064b3c5
42022.4.diff (43.1 KB) - added by westonruter 3 weeks ago.
https://github.com/xwp/wordpress-develop/pull/266/files/064b3c5..3bd6bf3
nav-menu-name-before.png (39.3 KB) - added by westonruter 3 weeks ago.
nav-menu-name-after.png (38.6 KB) - added by westonruter 3 weeks ago.
42022.5.diff (1010 bytes) - added by westonruter 2 weeks ago.
hide-date-when-time-not-included.png (116.0 KB) - added by westonruter 2 weeks ago.
Date/time control with time_included vs not time_included
42022.6.diff (2.2 KB) - added by westonruter 2 weeks ago.
42022.7.diff (2.4 KB) - added by westonruter 2 weeks ago.
date-with-description-without-time.png (49.9 KB) - added by westonruter 2 weeks ago.
date-with-time-and-description-and-notifiations.png (66.2 KB) - added by westonruter 2 weeks ago.

Download all attachments as: .zip

Change History (41)

#1 @melchoyce
3 weeks ago

  • Description modified (diff)

#2 @melchoyce
3 weeks ago

  • Description modified (diff)

#3 @westonruter
3 weeks ago

  • Description modified (diff)
  • Keywords good-first-bug removed
  • Owner set to westonruter
  • Status changed from new to accepted
  • Summary changed from Customizer: Change "day" to "date" in scheduling to Customize: Fix date time control label
  • Type changed from enhancement to defect (bug)

#4 @westonruter
3 weeks ago

  • Summary changed from Customize: Fix date time control label to Customize: Fix date time control defects

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 weeks ago

#6 @westonruter
3 weeks ago

  • Keywords has-patch added; needs-patch removed

@afercia
3 weeks ago

#7 @afercia
3 weeks ago

Thanks for the PR. I see at the moment it doesn't address the main issue:

“form elements label are not explicitly associated with for/id attributes as required by the accessibility standard for any new code in core.”

This is a requirement of the current WordPress coding standards and should be implemented.

42022.diff far from being a real patch, serves the purpose to highlight what needs to be done.

If fixes also a few missing labels (also in other places in the Customizer) and avoids to print out empty labels. Again, please remember that any form input needs to be labeled.

Todo: find a better label text for AM / PM

<label for="date-time-ampm" class="screen-reader-text">AM / PM</label>

since it is visually hidden, it can be expanded with some more meaningful text.

There are many other pending a11y issues in the Customizer and I'm a bit surprised that, after so many recent improvements, very basic things like valid and semantic HTML are so overlooked. I haven't seen so much progress in this regard, and I'd really like to see more focus on producing at least valid HTML. Hopefully, semantic HTML.

Some examples of how the Customizer controls generate always a <label> element even when they're not meant to contain form controls...

<label for="custom_logo-button">			
	<span class="customize-control-title">Logo</span>
	<div class="customize-control-notifications-container"></div>
</label>
<label for="site_icon-button">
	<span class="customize-control-title">Site Icon</span>			
	<span class="description customize-control-description">The Site Icon is used as a browser and app icon for your site. Icons must be square, and at least <strong>512</strong> pixels wide and tall.</span>			
</label>
<label for="header_image-button">
	<span class="customize-control-title">Current header</span>
</label>
<label for="background_image-button">
	<span class="customize-control-title">Background Image</span>
</label>

This kind of HTML is, honestly, something that WordPress shouldn't, ever, output, for any reason.

Software that read the HTML to assist users in using WordPress, have big troubles when trying to report so poor HTML to users. The information conveyed is just wrong and our communication to our users just fails.

Aside:

  • colors like red shouldn't be used. WordPress has an official color palette and only colors from the palette should be used
  • please avoid using named colors e.g. white, this makes very difficult looking for occurrences of colors through the codebase
  • please avoid docblocks used as inline comments, for example this:
/**
 * Find all invalid setting less controls with notification type error.
 */

is just an inline comment but wrongly uses the double asterisk.

#8 @afercia
3 weeks ago

Note: not sure if the preview link input field should be labeled... to my understanding, it is used just to copy the link to the clipboard. If so, we should evaluate a way to make it completely invisible to assistive technologies.

#9 @westonruter
3 weeks ago

@afercia Thanks, I've merged your patch into the PR and tried to resolve conflicts: commit.

Other changes are:

  • Restore control title
  • Let each control instance's IDs be unique/random
  • Use Meridian instead of AM / PM

See PR for all changes: https://github.com/xwp/wordpress-develop/pull/266

#10 @westonruter
3 weeks ago

@sayedwp Here's a plugin that I've been using for testing the control: https://gist.github.com/westonruter/ca1a77f9309af4ab258573cee5ed9d30

Some additional thoughts:

We could eliminate the defaultValue entirely if we instead instantiate the control in JS and pass in the wp.customize.state( 'selectedChangesetDate' ) as the setting once we have #37964 implemented. The control then could be registered entirely in JS and not in PHP at all, and that would avoid us having to create a settingless control to then later link it with another Value.

In the same way, with #30738 we could also add the changeset status control with JS and have it linked to the selectedChangesetStatus value, instead of having to create a secondary Value to sync with. We may want to make an array of all of the allowed statuses for changesets, in addition to their saved/unsaved labels. Eventually I could see this as being something plugins would want to be able to filter, and it would be cleaner than having the choices for the status in one place and the translation labels in another place.

#11 @afercia
3 weeks ago

Is the value test really needed in the preview link input field?
<input readonly class="preview-control-element" data-component="input" value="test" >

#12 follow-up: @afercia
3 weeks ago

When clicking the preview link, it opens in a new tab but there's no indication it is going to open a new tab. All links that use target="_blank" or a like in this case a named browsing context keyword:
target="06abf282-e641-4862-bf60-121cac815ffe"
ned to inform users a new browser tab will be opened. The standard text for this currently used in core is:
__( '(opens in a new window)' )

not sure if in this case it needs also a rel="noopener noreferrer" attribute, see for example #36809 and #37941.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


3 weeks ago

#14 in reply to: ↑ 12 @westonruter
3 weeks ago

Replying to afercia:

not sure if in this case it needs also a rel="noopener noreferrer" attribute, see for example #36809 and #37941.

Good question, but I do not think so. What is seen here is the same as the post preview link in the edit post screen which outputs HTML like:

<a href="http://src.wordpress-develop.dev/?p=1080&amp;preview=true" target="wp-preview-1080">

#15 @westonruter
3 weeks ago

@afercia I believe 42022.3.diff addresses all of your feedback. See PR.

#16 follow-up: @afercia
3 weeks ago

@westonruter noticed add_control( new WP_Customize_Date_Time_Control ... doesn't pass a label to the template, any reason to keep it in the template?

<span class="customize-control-title">
	{{ data.label }}
</span>

Maybe in case it gets reused/extended?

The labels for
custom_logo-button
site_icon-button
header_image-button
background_image-button

are still not correct, maybe this should be handled in #33085 ?

Lastly: on IE 11 and Edge, some fields need a slightly bigger width (different system font, takes more space). Maybe should be tested also on Linux? (at least one distro?)

https://cldup.com/IlQRS41c6K.png

#17 in reply to: ↑ 16 @westonruter
3 weeks ago

Replying to afercia:

@westonruter noticed add_control( new WP_Customize_Date_Time_Control ... doesn't pass a label to the template, any reason to keep it in the template? Maybe in case it gets reused/extended?

Yes, that is why it should be kept. See this example plugin which can then make use of it: https://gist.github.com/westonruter/ca1a77f9309af4ab258573cee5ed9d30

The labels for
custom_logo-button
site_icon-button
header_image-button
background_image-button

are still not correct, maybe this should be handled in #33085 ?

Yes, let's handle them there. That's the more appropriate place.

Lastly: on IE 11 and Edge, some fields need a slightly bigger width (different system font, takes more space). Maybe should be tested also on Linux? (at least one distro?)

https://cldup.com/IlQRS41c6K.png

Good catch. I think the problem here is also that these select dropdowns should have an auto width and not a fixed width, as they can be translated after all. I've fixed that in 3bd6bf3. Look good?

#18 @westonruter
3 weeks ago

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

In 41670:

Customize: Fix WP_Customize_Date_Time_Control to be re-usable for plugins and custom settings.

  • Allow time fields to be omitted by constructing with timeIncluded as false.
  • Ensure reportValidity is only called on a control when it is in an expanded section.
  • Rename "ampm" to "meridian".
  • Improve accessibility and fix HTML validation and style issues for both the date/time control and the preview link control.
  • Fix styling of dropdowns and clean CSS.
  • Improve accessibility of nav menus component.

Props westonruter, afercia, sayedwp, melchoyce.
Amends [41626].
See #39896.
Fixes #42022.

#19 @westonruter
3 weeks ago

@afercia I just noticed a style regression in the nav menu name field after r41670. Compare two previous screenshots.

Last edited 3 weeks ago by westonruter (previous) (diff)

#20 @sayedwp
2 weeks ago

@westonruter I think if we are passing 'include_time' => false, the "Date" title becomes redundant and probably should be removed, because it's being described by the label.

https://user-images.githubusercontent.com/6297436/31076478-e688abee-a797-11e7-8936-e886c3303263.png

#21 @afercia
2 weeks ago

Note the "Date" is now a fieldset legend, and it identifies all the 3 date fields. It shouldn't be removed. Mayeb consider to remove the bold "Date Established" instead.

#22 @westonruter
2 weeks ago

@afercia the “Date Established” is the label of the field. I think that the “Date” above the month should get screen-reader-text applied to it if not include_time. Thoughts?

@westonruter
2 weeks ago

@westonruter
2 weeks ago

Date/time control with time_included vs not time_included

#23 @westonruter
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems like there is too much extra padding between the label and the fields still in hide-date-when-time-not-included.png

@westonruter
2 weeks ago

@westonruter
2 weeks ago

#24 @afercia
2 weeks ago

@westonruter

the “Date Established” is the label of the field.

Maybe in Customizer terms is a "label", not an HTML <label> and if it's rendered as a <label> it shouldn't. A <label> is associated to one form field, not 3. That should be just text or the fieldset legend.

#25 @westonruter
2 weeks ago

@afercia That's right. In the Customizer API, a control has a label property which normally gets rendered as a <label>. But in the case of this control which has compound inputs, there isn't one input to link the label to, so a <span class="customize-control-title"> is used instead. For Customizer controls, the label is really just a synonym for title.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


2 weeks ago

#27 @westonruter
2 weeks ago

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

In 41672:

Customize: Improve styling of date/time Customizer control.

  • Let date legend be screen reader text when time is not included.
  • Skip rendering containers for label (title) and description when not supplied in registered control.
  • Fix margins and padding.

Amends [41670].
Props westonruter, afercia, sayedwp.
Fixes #42022.

This ticket was mentioned in Slack in #core-customize by bpayton. View the logs.


2 weeks ago

Note: See TracTickets for help on using tickets.