New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding date form to date picker and improving accessibility #7621

Merged
merged 1 commit into from Oct 13, 2018

Conversation

@aldavigdis
Contributor

aldavigdis commented Jun 28, 2018

Description

Fixes #7621.

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

screen shot 2018-06-28 at 23 36 35

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:

datepicker

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

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

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.)

This comment has been minimized.

@aldavigdis

aldavigdis Jun 28, 2018

Contributor

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

This comment has been minimized.

@aldavigdis

aldavigdis Jun 28, 2018

Contributor

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.

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

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

Y10K! 😆😉

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

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

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 🤷‍♂️

This comment has been minimized.

@aldavigdis

aldavigdis Jun 29, 2018

Contributor

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.

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

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

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 😄

This comment has been minimized.

@aldavigdis

aldavigdis Jun 28, 2018

Contributor

Yeah, same here. Better nitpick sooner than later!

This comment has been minimized.

@aldavigdis

aldavigdis Jun 29, 2018

Contributor

Fix'd!

onChange( formattedDate );
}
updateMonth() {

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

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?

This comment has been minimized.

@aldavigdis

aldavigdis Jun 28, 2018

Contributor

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

This comment has been minimized.

@tofumatt

tofumatt Jun 28, 2018

Member

Yeah, definitely. Readable repetition over clever metaprogramming anyday 👍

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

This comment has been minimized.

@aldavigdis

aldavigdis Jun 29, 2018

Contributor

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.

@tofumatt tofumatt requested a review from afercia Jun 28, 2018

@aldavigdis

This comment has been minimized.

Contributor

aldavigdis commented Jun 28, 2018

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

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 29, 2018

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:

screen shot 2018-06-29 at 11 25 23

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

screen shot 2018-06-29 at 11 25 38

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

This comment has been minimized.

Contributor

jasmussen commented Jun 29, 2018

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

This comment has been minimized.

Contributor

aldavigdis commented Jun 29, 2018

@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

This comment has been minimized.

Contributor

jasmussen commented Jun 29, 2018

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

@aldavigdis

This comment has been minimized.

Contributor

aldavigdis commented Jun 29, 2018

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

@karmatosed karmatosed self-requested a review Jul 1, 2018

@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

This comment has been minimized.

Member

karmatosed commented Jul 1, 2018

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:

2018-07-01 at 13 53

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

@aldavigdis

This comment has been minimized.

Contributor

aldavigdis commented Jul 1, 2018

@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

This comment has been minimized.

Contributor

jasmussen commented Jul 2, 2018

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

This comment has been minimized.

Member

pento commented Jul 2, 2018

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.

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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)

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

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

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

typo :D

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

please use aria-label

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

please use aria-label

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

please use aria-label

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

please use aria-label

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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.

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

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

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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)

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

I agree!

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

I agree!

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

I agree.

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

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.

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

This comment has been minimized.

@afercia

afercia Jul 2, 2018

Contributor

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.

This comment has been minimized.

@aldavigdis

aldavigdis Jul 9, 2018

Contributor

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.

@gziolo gziolo added this to the 4.1 milestone Oct 10, 2018

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

This comment has been minimized.

@aduth

aduth Oct 11, 2018

Member

Is this meant to be here? (#10214)

This comment has been minimized.

@tofumatt

tofumatt Oct 11, 2018

Member

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. 😢

Show resolved Hide resolved packages/components/src/date-time/date.js Outdated
const momentDate = currentDate ? moment( currentDate ) : moment();
const momentTime = {
seconds: momentDate.seconds(), minutes: momentDate.minutes(),

This comment has been minimized.

@aduth

aduth Oct 11, 2018

Member

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

<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>

This comment has been minimized.

@aduth

aduth Oct 11, 2018

Member

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

This comment has been minimized.

@tofumatt
Show resolved Hide resolved packages/components/src/date-time/time.js
@@ -1,6 +1,6 @@
{
"name": "@wordpress/date",
"version": "2.0.1",
"version": "2.0.0",

This comment has been minimized.

@aduth

aduth Oct 11, 2018

Member

Still needs updating.

@tofumatt tofumatt requested a review from aduth Oct 12, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 12, 2018

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

@aduth

aduth approved these changes Oct 12, 2018

@tofumatt tofumatt dismissed stale reviews from karmatosed and gziolo Oct 12, 2018

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

This comment has been minimized.

Member

tofumatt commented Oct 12, 2018

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? 😓

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

This comment has been minimized.

@tofumatt

tofumatt Oct 12, 2018

Member

Hah, maybe this is it... 😓

Adding date form to date picker and improving accessibility
* 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

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

2 checks passed

codecov/project 49.53% (+0.05%) compared to b1c05b3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Oct 15, 2018

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

This comment has been minimized.

Member

tofumatt commented Oct 15, 2018

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

This comment has been minimized.

Contributor

mtias commented Oct 15, 2018

Nice work everyone driving this one to the end.

etoledom added a commit that referenced this pull request Oct 15, 2018

Adding date form to date picker and improving accessibility (#7621)
* 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
@aldavigdis

This comment has been minimized.

Contributor

aldavigdis commented Oct 18, 2018

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

This comment has been minimized.

paaljoachim commented Oct 22, 2018

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.

screen shot 2018-10-22 at 19 00 16

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

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 22, 2018

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

This comment has been minimized.

greg-raven commented Nov 27, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment