Fix a couple of nits as a result of the a11y PR #429

majapw · 2017-04-07T20:47:04Z

blur the CalendarDay is month navigation was done with the mouse
prevent home/end/pagedown/pageup from scrolling the page
fixed some weirdness with the aria-label text on Calendar Day components when not using the DRP explicitly

to: @airbnb/webinfra @moonboots

coveralls · 2017-04-07T21:03:44Z

Coverage decreased (-0.2%) to 83.554% when pulling 7f44e43 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

lencioni · 2017-04-07T21:12:59Z

src/components/DayPicker.jsx

+    }, () => {
+      // we don't want to focus on the relevant calendar day after a month transition
+      // if the user is navigating around using a mouse
+      if (this.state.withMouseInteractions && document.activeElement) {


Should you check that document.activeElement is not the body?

Would it cause a problem if it was?

Should this also check for document?

document.activeElement is the body by default. So when you blur, it gets set to the body. So this will end up blurring on the body every time. Probably not a big deal, but it is possible that this could kick off event listeners or other side-effects, depending on what has been registered on the page. Something like this might work? (not sure about browser support of HTMLBodyElement off the top of my head though)

!(document.activeElement instanceof HTMLBodyElement)

or I guess more simply

document.activeElement !== document.body

I'll add the check, although this is somewhat of an extreme edge case given the way the datepicker is managing focus.

Basically, the setState that this is a callback of triggers a series of events that always ends in a CalendarDay being focused. So while it is possible that the user could blur between the componentDidUpdate call that focuses the CalendarDay in that leaf node and this setState callback 4 levels up in the tree, it's highly unlikely.

coveralls · 2017-04-07T21:16:13Z

Coverage decreased (-0.2%) to 83.554% when pulling 7f44e43 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

ljharb · 2017-04-07T21:32:32Z

src/components/DayPicker.jsx

@@ -301,10 +305,12 @@ export default class DayPicker extends React.Component {
        didTransitionMonth = this.maybeTransitionPrevMonth(newFocusedDate);
        break;
      case 'Home':
+        if (e) e.preventDefault();


I'm curious when there will not be an e object in an event handler?

(it's probably best to keep this as-is, ofc, for consistency with the rest of the branches; and removing the if in the other branches would be semver-major, but i'd like to see us do that in the future)

Mostly for testing purposes, which like, could be done better. I can remove these in another PR.

Awesome, let's do that - we can pass { preventDefault() {} } in tests if needed.

ljharb · 2017-04-07T21:33:06Z

src/components/DayPicker.jsx

+    }, () => {
+      // we don't want to focus on the relevant calendar day after a month transition
+      // if the user is navigating around using a mouse
+      if (this.state.withMouseInteractions && document.activeElement) {


Should this also check for document?

majapw · 2017-04-07T21:53:51Z

I also fixed some weirdness that I had not fully thought through with the phrases when it came to non-drp usage! PTAL @lencioni @ljharb

coveralls · 2017-04-07T22:10:11Z

Coverage decreased (-0.08%) to 83.662% when pulling 2e6f4a0 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

coveralls · 2017-04-07T22:17:23Z

Coverage decreased (-0.08%) to 83.662% when pulling 615d4c0 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

coveralls · 2017-04-07T22:27:39Z

Coverage decreased (-0.08%) to 83.662% when pulling 615d4c0 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

ljharb

LGTM, but it'd be nice not to drop test coverage ಠ_ಠ

ljharb · 2017-04-07T22:34:18Z

src/components/CalendarDay.jsx

@@ -102,8 +102,7 @@ export default class CalendarDay extends React.Component {
    const formattedDate = `${day.format('dddd')}, ${day.format('LL')}`;

    let ariaLabel = getPhrase(chooseAvailableDate, {
-      checkin_date: formattedDate,
-      checkout_date: formattedDate,
+      date: formattedDate,


is this a bugfix, or semver-major?

bugfix i'll say.

ljharb · 2017-04-07T22:35:21Z

src/utils/getPhrase.jsx

@@ -1,6 +1,8 @@
 import React from 'react';

 export default function getPhrase(phrase, args) {
+  if (!phrase) return '';


tests for this?

I should
probably write some tests
sigh

- blur the CalendarDay is month navigation was done with the mouse - prevent home/end/pagedown/pageup from scrolling the page - fixed some weirdness with the aria-label text on Calendar Day components when not using the DRP explicitly

coveralls · 2017-04-10T17:17:06Z

Coverage decreased (-0.03%) to 83.716% when pulling 311fec3 on maja-fix-tweaks-to-a11y-pr into 289dcc3 on master.

majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Apr 7, 2017

majapw force-pushed the maja-fix-tweaks-to-a11y-pr branch from 2f956c1 to 7f44e43 Compare April 7, 2017 21:02

lencioni reviewed Apr 7, 2017

View reviewed changes

ljharb reviewed Apr 7, 2017

View reviewed changes

majapw force-pushed the maja-fix-tweaks-to-a11y-pr branch 2 times, most recently from ef54e2b to 2e6f4a0 Compare April 7, 2017 21:53

majapw force-pushed the maja-fix-tweaks-to-a11y-pr branch from 2e6f4a0 to 615d4c0 Compare April 7, 2017 22:13

ljharb approved these changes Apr 7, 2017

View reviewed changes

Fix a couple of nits as a result of the a11y PR

311fec3

- blur the CalendarDay is month navigation was done with the mouse - prevent home/end/pagedown/pageup from scrolling the page - fixed some weirdness with the aria-label text on Calendar Day components when not using the DRP explicitly

majapw force-pushed the maja-fix-tweaks-to-a11y-pr branch from 615d4c0 to 311fec3 Compare April 10, 2017 17:03

majapw merged commit 2c71984 into master Apr 10, 2017

majapw deleted the maja-fix-tweaks-to-a11y-pr branch April 10, 2017 17:25

allilevine mentioned this pull request Sep 15, 2020

DateTimePicker: Click on date causes popover to close automatically WordPress/gutenberg#24206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a couple of nits as a result of the a11y PR #429

Fix a couple of nits as a result of the a11y PR #429

majapw commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

lencioni Apr 7, 2017

majapw Apr 7, 2017

ljharb Apr 7, 2017

lencioni Apr 7, 2017

lencioni Apr 7, 2017

majapw Apr 7, 2017

coveralls commented Apr 7, 2017 •

edited

Loading

ljharb Apr 7, 2017

majapw Apr 7, 2017

ljharb Apr 7, 2017

ljharb Apr 7, 2017

majapw commented Apr 7, 2017

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

ljharb left a comment

ljharb Apr 7, 2017

majapw Apr 7, 2017

ljharb Apr 7, 2017

majapw Apr 7, 2017

coveralls commented Apr 10, 2017 •

edited

Loading

Fix a couple of nits as a result of the a11y PR #429

Fix a couple of nits as a result of the a11y PR #429

Conversation

majapw commented Apr 7, 2017 • edited Loading

coveralls commented Apr 7, 2017 • edited Loading

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

coveralls commented Apr 7, 2017 • edited Loading

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

majapw commented Apr 7, 2017

coveralls commented Apr 7, 2017 • edited Loading

coveralls commented Apr 7, 2017 • edited Loading

coveralls commented Apr 7, 2017 • edited Loading

ljharb 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

coveralls commented Apr 10, 2017 • edited Loading

majapw commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 7, 2017 •

edited

Loading

coveralls commented Apr 10, 2017 •

edited

Loading