Adding date form to date picker and improving accessibility #7621

aldavigdis · 2018-06-28T21:41:56Z

Description

Fixes issues related to issue #1311. The discussion on the ticket is extensive, but this should cover most of the complaints.

Date form has been added to the date-time component, as a primary input method
Locale-dependent field order, with en-US being month-day, and the default day-month
Keyboard focus goes to the month selector first, as dates are validated based on the month (and year if leap-year)
The post-schedule sidebar widget is now more accessible, with button designated as a live object, so changes should be announced to date pickers
React-Datepicker widget has been removed from the keyboard navigation and screenreader context, as the new form fields are not the primary input method.

How has this been tested?

Unit tests added for text input fields
Manual tests for locale-depended field order

Screenshots

Types of changes

Accessibility and UI changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

tofumatt

This is cool! I left some little bits of code/style feedback.

I didn't try keyboard navigation yet; I'll leave that for @afercia since he filed the original issue and is an expert. 😄

Thoughts on the design: it'd be nice if days in a month that is not the current one/month selected were faded a bit to draw attention to the days in the selected month. So in your screenshot it'd look like:

tofumatt · 2018-06-28T22:24:57Z

components/date-time/style.scss

+
+		input[name=day] {
+			width: 40px;
+			-moz-appearance:textfield;


I would have thought our build tools would do browser-prefixes as-needed.

Are these here as browser-specific styles (I recall appearance being different in Firefox from my days working on the Add-ons website, but I forget now). If they're browser-specific that's fine, but a comment explaining what they're for (and why they're -moz-only) would be handy 😄

(Also there should be a space after the colon but that's just a style nitpick.)

Looks like some junk code from when I was working with number inputs. Well spotted!

And regarding the design of the date picker widget itself, I feel like it's outside of the scope of the ticket, but I'll be happy to make a separate PR for that.

tofumatt · 2018-06-28T22:26:17Z

components/date-time/time.js

+		const { onChange } = this.props;
+		const { year, date } = this.state;
+		const value = parseInt( year, 10 );
+		if ( ! isInteger( value ) || value < 1970 || value > 9999 ) {


Y10K! 😆😉

tofumatt · 2018-06-28T22:28:54Z

components/date-time/time.js

+		}
+		const newDate = date.clone().year( value );
+		this.setState( { date: newDate } );
+		const formattedDate = newDate.format( TIMEZONELESS_FORMAT );


Setting these as constants makes the intent clearer because they get useful names, but this variable is assigned the line before it's used, once.

It might be nicer to just comment anything you want to add clarification to and use the result of things like newDate.format( TIMEZONELESS_FORMAT ) inline, eg:

this.setState( { date: newDate } ); onChange( newDate.format( TIMEZONELESS_FORMAT ) )

Just a thought 🤷‍♂️

True. Let me have a look really quickly. I've always found the classic JS coding style to be very heavy on assigning everything as a variable though.

tofumatt · 2018-06-28T22:30:29Z

components/date-time/time.js

+							name="month"
+							value={ month }
+							onChange={ this.onChangeMonth }
+							onBlur={ this.updateMonth }>


Just a nitpick but should the > go on a newline? It might improve readability; if that violates our eslint my bad. I'm still getting used to Gutenberg's style 😄

Yeah, same here. Better nitpick sooner than later!

tofumatt · 2018-06-28T22:34:39Z

components/date-time/time.js

+		onChange( formattedDate );
+	}
+
+	updateMonth() {


The update[Day/Month/Year] methods are all very similar to each other, save the newDate and value checks. Could they be DRY'd a bit so they duplicate less code?

True. I felt I was following some convention by doing it this way. Sometimes too much metaprogramming results in less readability too.

Yeah, definitely. Readable repetition over clever metaprogramming anyday 👍

But if it could be tightened up that'd be cool. 😉

I decided not trying to consolidate this more and instead follow what has already been done. If we initiate a redesign, then we would probably remove those functions as well anyway.

aldavigdis · 2018-06-28T22:40:11Z

~~Update: just spotted an i18n issue. Will update the PR by tomorrow~~

jasmussen · 2018-06-29T09:29:56Z

Nice work. Like I said in some past feedback, I think the ingredients are there for something solid, and to elaborate on that, it seems like the combination of allowing you to input the date in classic form widgets for accessible reasons and having the calendar for mouse users, this could work for us.

I think the specific implementation could be tweaked a little bit. What if instead of dropdowns, we had an input field where you could both type in DD/MM/YYYY or in other formats, and we interpreted the input to a date format?

What if also, instead of those inputs being inside the popout, they were directly shown inline? Then the popout in its entirety could be aria-hidden.

Here are rough mockups:

Focus or click one of the fields to get the popout:

Don't pay too much attention to the specific visual styling of the input field, these would be normal input fields that would simply invoke the popout when focused.

We'd still have the same ingredients, just rearrange them a bit.

jasmussen · 2018-06-29T09:31:16Z

A sidenote I forgot to mention right above — my role in this is mainly as a designer, and the goal with the above mockups were to ensure accessibility, while ensuring the convenience of a date picker for mouse users. I will always defer to Tammie for final design thoughts on the overall picture, and I know she has many thoughts on date pickers. CC: @karmatosed

aldavigdis · 2018-06-29T14:14:27Z

@jasmussen — this is the approach I took from the start. Unfortunatly, it would have required the following:

Replacing React-Datepicker with ReactJS Day Picker — this is something that I've been expressing interest in doing, but I decided it was outside the scope of the ticket.
Rework both the date-time and post-schedule components from ground-up, even replacing the date-time one, unelss it's used somewhere else.

The popover managed to break the sidebar layout as well, so I decided to cut off the fat and concentrate on the actual matter.

So I decided to concentrate on tackling the keyboard navigation and accessibility issues first and foremost, without deviating too much from the current asthetics.

I am very happy to see the redesign mockup, but I suggest we create a new issue ticket called "Implement datepciker redesign" or something along those lines and I'll be happy work on it until @ehg tells me to stop. 🤓

jasmussen · 2018-06-29T14:46:20Z

Definitely seems fine to me to implement a stopgap solution on the path to greater things!

aldavigdis · 2018-06-29T14:53:10Z

i18n issue has been fixed. Squashing and pushing the changes.

karmatosed

Whilst I do think it's good to have a more accessible date picker in and thanks for all the great work here, if we also have a design let's do that at the same time. I would prefer we didn't have a half way state. Let's get the design work done by @jasmussen into this PR. I understand the urgency but there are some usability issues raised by such an intense UI that can be eased in the design.

karmatosed · 2018-07-01T12:55:01Z

I really like the work @jasmussen has done here and added a comment above to get that worked on as I think that's important. For input the date picker I am adoring right now is https://github.com/airbnb/react-dates:

What is being suggested I think is a solid design iteration, solid enough it should be done before we commit here.

aldavigdis · 2018-07-01T20:51:44Z

@karmatosed Note that I have suggested initiating a new issue ticket to implement a redesign as has been discussed here, which is something I do support wholeheartedly as a longer–term vision.

Nevertheless, issue #1311 managed to have its first anniversary a little more than a week ago. Leaving it open and unresolved instead of implementing this PR, even if it is just a stop–gap fix is something that I feel disregards the needs of those who require the use of assistive technologies such as screen readers to use Gutenberg and Calypso.

Implementing said redesign (and the coordination work required) at a later date on one hand and the changes brought in with this PR on the other is not mutually exclusive, with the changes brought in here having the potential of being a step toward the redesign, and being very important in regards of accessibility.

I am looking forward to see @afercia's take on this ticket.

jasmussen · 2018-07-02T06:01:02Z

Thanks so much for your work, this is a powerful proof of concept, and as far as I'm concerned it has helped unlock a path forward both implementationwise and design wise.

We are in beta right now, barrelling towards feature complete. The ticket this means to solve is in the merge-proposal milestone, which means we won't ship a v1 without this addressed.

Although I could see an intermediate step, Tammie's comment also reminded me of past instances where we fix and issue partially and then immedately forget about the other aspects we promised ourselves that we'd come back to fix. As such, I can completely understand the desire to keep this one a bit longer in the oven, and I'd always defer to her on the larger vision.

Given all the work that's been done already, I can only imagine phase 2 of this PR will be much easier than phase 1 has been, due to all the hard work you've already put into it. If you are out of time to contribute that's completely and totally fine, we will be able to take the work you've done and address the phase 2 feedback. Thank you ✨

pento · 2018-07-02T06:37:07Z

Thank you for the PR, @aldavigdis!

I don't have any comment on the redesign question, I'll leave that to @karmatosed as the Gutenberg design lead.

I have some feedback on the functionality and implementation, however!

Why are there value checks in updateDay(), updateMonth(), and updateYear()? The day check is inaccurate depending on the month and year (eg, Feb 29 is valid sometimes, Feb 30 is never valid). The month is coming from a dropdown, if a person is changing the value being sent, they can probably remove the check, too. The publication year can be before 1970, or after 9999.

Selecting an invalid date will cause the form value to jump around. Eg, if you select March 30, then change it to Feb 30 (say, if you wanted to change a scheduled post from sometime in March to sometime in Feb, and you decided to change the month first), the form value will automatically change to Feb 28. If you do it the other way, starting with Feb 28, then changing to Feb 30 (intending to change the month to March next), the form will auto-correct to March 02.

I like the idea of providing feedback on invalid dates, but it seems like it would be better to passively warn the person entering the date, rather than trying to correct whatever they've done wrong, before they've had a chance to correct it themselves.

When ordering the form elements, I highly recommend checking the behaviour of Core's touch_time() function. In particular, the __( '%1$s %2$s, %3$s @ %4$s:%5$s' ) translated string will give you the correct ordering for the form elements in every locale, rather than just en_US and everyone else. You could add this format to the wp.date.setSettings() setup call.

The date package provides a list of the months, rather than listing each month <option>, you could import them via getSettings() from @wordpress/date.

afercia · 2018-07-02T07:20:24Z

components/date-time/time.js

+					<div key="month-field" className="input-container month">
+						<select
+							id="date-time-form--month-field"
+							aria-labelledby={ __( 'Month' ) }


aria-labelledby accepts only a reference to an existing ID to target another element, in this case aria-label should be used instead (same applies to the other fields)

I stand corrected. This may also have happened during a "find-and-replace" operation so thanks for spotting this.

afercia · 2018-07-02T07:20:54Z

components/date-time/time.js

+							<option value="08">{ __( 'August' ) }</option>
+							<option value="09">{ __( 'September' ) }</option>
+							<option value="10">{ __( 'October' ) }</option>
+							<option value="11">{ __( 'Nobember' ) }</option>


afercia · 2018-07-02T07:21:24Z

components/date-time/time.js

+						<input
+							id="date-time-form--day-field"
+							className="components-time-picker__input day"
+							aria-labelledby={ __( 'Day' ) }


please use aria-label

afercia · 2018-07-02T07:21:43Z

components/date-time/time.js

+						<input
+							id="date-time-form--year-field"
+							className="components-time-picker__input"
+							aria-labelledby={ __( 'Year' ) }


please use aria-label

afercia · 2018-07-02T07:21:56Z

components/date-time/time.js

+					<input
+						id="date-time-form--hour-field"
+						className="components-time-picker__input"
+						aria-labelledby={ __( 'Hours' ) }


please use aria-label

afercia · 2018-07-02T07:22:22Z

components/date-time/time.js

+					<input
+						id="date-time-form--minute-field"
+						className="components-time-picker__input"
+						aria-labelledby={ __( 'Minutes' ) }


please use aria-label

edit-post/components/sidebar/post-schedule/index.js

afercia · 2018-07-02T07:29:42Z

edit-post/components/sidebar/post-schedule/index.js

 							type="button"
 							className="edit-post-post-schedule__toggle"
 							onClick={ onToggle }
 							aria-expanded={ isOpen }
+							aria-live="polite"
+							aria-label={ __( 'Date and time' ) }


Not sure about this, as using a label that's completely different from the displayed text won't work for some users, for example speech recognition software users. There's also #470 about the visibility and date buttons.

So it's better to introduce a label tag instead?

afercia · 2018-07-02T07:59:52Z

components/date-time/time.js

+						onChange={ this.onChangeMinutes }
+						onBlur={ this.updateMinutes }
+					/>
+					{ is12Hour && <fieldset aria-labelledby={ __( 'AM or PM' ) }>


fieldset elements shouldn't use aria-labelledby; instead they should use a <legend> element to give the group a name. In this case though I'm not sure I'd use a fieldset in the first place. I'd say the two buttons "AM" and "PM" are clear enough. (also, they're nested within the "time" fieldset)

afercia · 2018-07-02T08:01:07Z

components/date-time/time.js

-						{ __( 'PM' ) }
-					</Button>
-				</div> }
+				<fieldset key="date-time-form" className="date-time-form">


fieldset should use a <legend> element to give the group a name

afercia · 2018-07-02T08:01:33Z

components/date-time/time.js

+						/>
+					</div>
+				</fieldset>
+				<fieldset className="date-time-form">


fieldset should use a <legend> element to give the group a name

afercia · 2018-07-02T08:03:46Z

components/date-time/style.scss

+
+/* Makes the month appear before the day for those in the USA */
+html[lang="en-US"] .date-time-form .input-container.month {
+	order: 0;


we can't use the flex order property, as for a11y visual order should always match DOM order when it affects meaning and operability

In this case, the date validation (which is done on-the-fly) is based on the number of days in the month, so this is the best thing I could come up with.

This means the user will always be able to tab into the month, update it and then enter the day of the month.

afercia · 2018-07-02T08:44:27Z

components/date-time/date.js

-		{ ...args }
-	/>;
+	return (
+		<div className="datepicker-container" tabIndex="-1" aria-hidden="true">


Not sure tabIndex="-1" will help so much, as it works just on the element it's applied to. This div is already non-focusable. Instead, I'm still able to tab to the Prev and Next buttons in the date picker.

I'm looking into this right now. But alas, this also depends on the date picker component itself. I just sent in a PR for React-Datepicker that would introduce text value to the buttons as that seems to be the only way to get this to work without forking the date picker (not a good idea) or using the onFocus hook to disable those buttons.

afercia

Left come comments, please see also comment in the thread.

aduth · 2018-10-11T15:50:17Z

packages/api-fetch/src/index.js

-				body: body || JSON.stringify( data ),
-				headers,
-			}
+		const responsePromise = Promise.resolve(


Is this meant to be here? (#10214)

Probably not. Sorry, this branch is epic and has required a lot of rebases and at this point I'm not sure what is what anymore. 😢

packages/components/src/date-time/date.js

aduth · 2018-10-11T15:57:19Z

packages/components/src/date-time/date.js

+
+		const momentDate = currentDate ? moment( currentDate ) : moment();
+		const momentTime = {
+			seconds: momentDate.seconds(), minutes: momentDate.minutes(),


We should be consistent about one property per line, or all properties on one line.

aduth · 2018-10-11T15:58:01Z

packages/components/src/date-time/date.js

+						<li>{ __( 'Click the right and left arrows to select other months in the past or the future.' ) }</li>
+						<li>{ __( 'Click the desired day to select it.' ) }</li>
+					</ul>
+					<strong>{ __( 'Navigating with a keyboard' ) }</strong>


Semantically, wouldn't these be better represented by a heading tag?

packages/components/src/date-time/time.js

aduth · 2018-10-11T16:01:15Z

packages/date/package.json

@@ -1,6 +1,6 @@
 {
 	"name": "@wordpress/date",
-	"version": "2.0.1",
+	"version": "2.0.0",


Still needs updating.

tofumatt · 2018-10-12T02:42:21Z

All comments addressed, should be set for another look. Thanks a bunch for catching everything 👍

In order to meet the accessibility milestones we need to iterate on this; as it's an improvement over what we have I'm going to ship it. We can iterate on it later :-)

tofumatt · 2018-10-12T22:13:53Z

I'm a bit confused on why these totally unrelated E2E tests are failing: https://travis-ci.org/WordPress/gutenberg/jobs/440799548#L1441

I didn't touch editor alignment and they're failing for me locally too, even after rebasing onto the very latest master. Any ideas? 😓

tofumatt · 2018-10-12T22:28:57Z

packages/components/src/button/index.js

@@ -43,6 +43,7 @@ export function Button( props, ref ) {
 	return createElement( tag, {
 		...tagProps,
 		...additionalProps,
+		'aria-pressed': isToggled,


Hah, maybe this is it... 😓

* Date form has been added to the date-time component, as a primary input method * Locale-dependent field order, with en-US being month-day, and the default day-month * Keyboard focus goes to the month selector first, as dates are validated based on the month (and year if leap-year) * The post-schedule sidebar widget is now more accessible, with button designated as a live object, so changes should be announced to date pickers * React-Datepicker widget has been removed from the keyboard navigation and screenreader context, as the new form fields are not the primary input method. * Tiny bit of code cleanup

danielbachhuber · 2018-10-15T11:51:19Z

I didn't touch editor alignment and they're failing for me locally too, even after rebasing onto the very latest master. Any ideas?

@tofumatt What was the reason?

tofumatt · 2018-10-15T11:53:30Z

Oh, I tried to be clever in the branch and automate the aria-pressed prop for the Button component. Turns out it didn't behave like I wanted and the tests caught that 👍

So I just left it alone and manually set the aria-pressed prop (as this PR did originally anyhow).

mtias · 2018-10-15T13:27:14Z

Nice work everyone driving this one to the end.

aldavigdis · 2018-10-18T14:56:34Z

Just wanted to say that I'm stoked that things managed to get to the finish line here!

Thanks everyone! This has been a ride!

paaljoachim · 2018-10-22T17:02:31Z

Setting a post into the future. Doing a few previews and then later setting it to todays/or earlier date still leaves a trace of the future date. It still thinks that today (yesterday) is the future.

I had to switch to draft and then back again to publish for it to notice that I had changed dates.

tofumatt · 2018-10-22T22:30:48Z

Hi @paaljoachim, if you could please refrain from adding comments to closed issues and instead file new issues with the reports that would be easier for us to track. Thanks so much! 😄

greg-raven · 2018-11-27T13:52:17Z

Well, this is better than it was initially, but I just tried to post an article from November 30, 1950, and the Gutenberg date picker would not accept 1950 as the year. As soon as the cursor left the year field, the year reverted back to 2018.

aldavigdis mentioned this pull request Jun 28, 2018

react-datepicker is not accessible #1311

Closed

karmatosed added the Needs Design Feedback Needs general design feedback. label Jun 28, 2018

tofumatt reviewed Jun 28, 2018

View reviewed changes

tofumatt requested a review from afercia June 28, 2018 22:39

karmatosed self-requested a review July 1, 2018 12:52

karmatosed previously requested changes Jul 1, 2018

View reviewed changes

afercia reviewed Jul 2, 2018

View reviewed changes

edit-post/components/sidebar/post-schedule/index.js Outdated Show resolved Hide resolved

afercia reviewed Jul 2, 2018

View reviewed changes

gziolo added the [Priority] High Used to indicate top priority items that need quick attention label Oct 10, 2018

aduth requested changes Oct 11, 2018

View reviewed changes

tofumatt requested a review from aduth October 12, 2018 02:41

aduth approved these changes Oct 12, 2018

View reviewed changes

tofumatt reviewed Oct 12, 2018

View reviewed changes

tofumatt merged commit 00b9063 into WordPress:master Oct 13, 2018

postphotos mentioned this pull request Oct 14, 2018

Fix: z-index: -1 needed for publish footer due to accordions #10589

Closed

4 tasks

Soean mentioned this pull request Oct 14, 2018

In date selection, please make a select to choose the year of the entry #10600

Closed

tofumatt mentioned this pull request Oct 15, 2018

Combine "Calendar Help" and "Close" buttons in Datepicker #10621

Closed

ocean90 mentioned this pull request Oct 21, 2018

strip html from string to be translated #10841

Closed

paaljoachim mentioned this pull request Oct 23, 2018

Schedule post to future date and then change to past date doesn't publish inmediately #10930

Closed

This was referenced Oct 23, 2018

Gutenberg: update npm packages to latest versions Automattic/wp-calypso#28000

Merged

DateTimePicker component: Use color function for defining the background #10943

Merged

aduth mentioned this pull request Jan 30, 2019

Fix: Allow years lower than 1970 in DateTime component. #13602

Merged

youknowriad mentioned this pull request Nov 21, 2019

Fix hidden post schedule popover on mobile #18658

Merged

gziolo mentioned this pull request Apr 26, 2020

Improvement suggestion: replace react-dates with smaller library #21820

Closed

retrofox mentioned this pull request Sep 22, 2020

DateTimePicker: re-implement component with react-datepicker component #25428

Closed

9 tasks

Adding date form to date picker and improving accessibility #7621

Adding date form to date picker and improving accessibility #7621

Conversation

aldavigdis commented Jun 28, 2018 • edited by tofumatt Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldavigdis commented Jun 28, 2018 • edited Loading

jasmussen commented Jun 29, 2018

jasmussen commented Jun 29, 2018

aldavigdis commented Jun 29, 2018 • edited Loading

jasmussen commented Jun 29, 2018

aldavigdis commented Jun 29, 2018

karmatosed left a comment • edited Loading

Choose a reason for hiding this comment

karmatosed commented Jul 1, 2018 • edited Loading

aldavigdis commented Jul 1, 2018 • edited Loading

jasmussen commented Jul 2, 2018

pento commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

aldavigdis Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldavigdis Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldavigdis Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldavigdis Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Oct 12, 2018

tofumatt commented Oct 12, 2018

Choose a reason for hiding this comment

danielbachhuber commented Oct 15, 2018

tofumatt commented Oct 15, 2018

mtias commented Oct 15, 2018

aldavigdis commented Oct 18, 2018

paaljoachim commented Oct 22, 2018 • edited Loading

tofumatt commented Oct 22, 2018

greg-raven commented Nov 27, 2018

aldavigdis commented Jun 28, 2018 •

edited by tofumatt

Loading

aldavigdis commented Jun 28, 2018 •

edited

Loading

aldavigdis commented Jun 29, 2018 •

edited

Loading

karmatosed left a comment •

edited

Loading

karmatosed commented Jul 1, 2018 •

edited

Loading

aldavigdis commented Jul 1, 2018 •

edited

Loading

afercia Jul 2, 2018 •

edited

Loading

aldavigdis Jul 9, 2018 •

edited

Loading

aldavigdis Jul 9, 2018 •

edited

Loading

aldavigdis Jul 9, 2018 •

edited

Loading

aldavigdis Jul 9, 2018 •

edited

Loading

paaljoachim commented Oct 22, 2018 •

edited

Loading