Change month nav buttons to be divs with role="button" #1305
Conversation
majapw
added
the
semver-patch: fixes/refactors/etc
majapw
force-pushed
the
maja-address-unclickable-custom-nav-in-FF
branch
from
a1e4c7c
to
23d0d58
Compare
| @@ -283,11 +293,13 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({ | |||
| height: 19, | |||
| width: 19, | |||
| fill: color.core.grayLight, | |||
| display: 'block', | |||
There was a problem hiding this comment.
Should this be inline-block to match the default for button?
There was a problem hiding this comment.
ditto what garrett said
There was a problem hiding this comment.
This is on the svg itself, not on the button and ... this is the display value that is needed for the height to remain the same in the before/after 🤷♀️ Otherwise there is some weird padding below the arrows.
There was a problem hiding this comment.
Confirmed, inline-block does not address the issue. I think it has to do with the svg itself?
There was a problem hiding this comment.
ah ok didn't realized this was applied to the svg - this seems fine
| aria-label={phrases.jumpToPrevMonth} | ||
| onClick={onPrevMonthClick} | ||
| onKeyDown={(e) => { |
There was a problem hiding this comment.
probably better to use onKeyUp here instead
| @@ -283,11 +293,13 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({ | |||
| height: 19, | |||
| width: 19, | |||
| fill: color.core.grayLight, | |||
| display: 'block', | |||
There was a problem hiding this comment.
ditto what garrett said
| @@ -115,7 +115,9 @@ function DayPickerNavigation({ | |||
| )} | |||
| > | |||
| {!isVerticalScrollable && ( | |||
| <button | |||
| <div | |||
There was a problem hiding this comment.
I'm not sure how much CSS you want to remove (also not sure if you ever think you'll turn this back to a button in the future), but now that this is a div, there's a lot of CSS that's no longer necessary
There was a problem hiding this comment.
i'm very confused why a "div that you're marking as being a button" is better than "a semantically correct button element" :-/
There was a problem hiding this comment.
@ljharb Did you read my description? The whole point is that with the way the custom button code is currently set up (the navPrev/navNext props), custom month navigation DOES NOT WORK (literally you cannot navigate months) in Firefox. This is specifically because of the way Firefox handles button elements. I don't know that we'll want to keep this forever but this div with a button role addresses the issue and still satisfies accessibility criteria. This is worth revisiting (and as I mentioned there still exists an issue with there being no visible outline) but we need to fix this right now.
There was a problem hiding this comment.
@backwardok I think I actually do want to keep the css
cursor: 'pointer',
userSelect: 'none',
border: 0,
padding: 0,
margin: 0,
is all there is and I think we still want those things.
There was a problem hiding this comment.
border, padding, and margin don't need to be set on divs because they don't have those things set by default
Custom month nav buttons are not currently clickable in Firefox. Changing them to divs addresses the problem. This is due to the difference in treatment of button elements by Firefox (absolutely positioned children of <button> elements that are rendered outside of the <button> visually are not interactive). Separately, this work highlighted the fact that custom month navigation does not currently have a focus ring. That is something not addressed by this commit but which will be investigated in the near future.
majapw
force-pushed
the
maja-address-unclickable-custom-nav-in-FF
branch
from
23d0d58
to
d35a736
Compare
|
@backwardok @ljharb @garrettberg I've changed the |
|
Hello. In the WordPress Gutenberg editor project we're trying to use |
|
Hi @afercia, the issue was specifically when the navNext/navPrev props were
used. The default case continue to work as intended out of the box.
…On Sat, Sep 29, 2018, 10:25 AM Andrea Fercia ***@***.***> wrote:
Hello. In the WordPress Gutenberg editor project we're trying to use
DayPickerSingleDateController (see WordPress/gutenberg#7621
<WordPress/gutenberg#7621>) and at the moment
it's still using 17.1.1. In fact, in the Gutenberg implementation these
buttons are still <button> elements. I'm not able to reproduce the
original bug, for me the buttons are perfectly actionable with both mouse
and keyboard, at least in Firefox 62 on mac. Out of curiosity, was the bug
triggered under some special condition or configuration or anything else I
may be missing? Thanks! /Cc @majapw <https://github.com/majapw>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1305 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUdtWEeHyNpKJVSg3OFZpTU3b2sL3Pzks5uf60agaJpZM4V7j8u>
.
|
Custom month nav buttons are not currently clickable in Firefox. Changing them to divs addresses the problem. This is due to the difference in treatment of button elements by Firefox (absolutely positioned children of elements that are rendered outside of the visually are not interactive).
Separately, this work highlighted the fact that custom month navigation does not currently have a focus ring. That is something not addressed by this commit but which will be investigated in the near future. I have opened an issue here: #1304.
Fixes #1303
to: @ha1ogen @noratarano @ljharb @backwardok @garrettberg