Dropdown arrow key support #1075

kmcfaul · 2018-12-17T16:56:11Z

What: Dropdown menus should be navigable via up and down arrow keys.

Refers to issue: #1002

kmcfaul · 2018-12-17T16:58:40Z

I removed the mapping for the key handler when the children elements are passed in, and not DropdownItems, for the moment, as they were causing invariant errors and React warnings because the keyHandler prop was being read as a DOM element.

patternfly-build · 2018-12-17T17:13:07Z

PatternFly-React preview: https://1075-pr-patternfly-react-patternfly.surge.sh

coveralls · 2018-12-17T17:13:45Z

Pull Request Test Coverage Report for Build 3934

30 of 72 (41.67%) changed or added relevant lines in 7 files are covered.
4 unchanged lines in 3 files lost coverage.
Overall coverage decreased (-0.9%) to 80.179%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.js	3	5	60.0%
packages/patternfly-4/react-core/src/components/Dropdown/Item.js	0	2	0.0%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js	4	12	33.33%
packages/patternfly-4/react-core/src/components/Dropdown/Toggle.js	2	10	20.0%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownMenu.js	18	40	45.0%

Files with Coverage Reduction	New Missed Lines	%
packages/patternfly-4/react-core/src/components/Modal/ModalContent.js	1	82.35%
packages/patternfly-4/react-core/src/components/Nav/NavItem.js	1	75.0%
packages/patternfly-4/react-core/src/components/Dropdown/Toggle.js	2	47.22%

Totals
Change from base Build 3899:	-0.9%
Covered Lines:	4390
Relevant Lines:	5138

💛 - Coveralls

dmiller9911 · 2018-12-18T03:36:47Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

+class DropdownItem extends React.Component {
+  constructor(props) {
+    super(props);
+    this.ref = React.createRef();


No need for this to be in the constructor. The class properties plugin is being used. This will match other components.

I was able to remove the onKeyDown binding but I'm not sure where the ref should be created if outside of the constructor. Removing this breaks the ref.

there is a babel plugin that pretty much abstracts away any use of the constructor & adds any definition within the class as a class property instead of having to explicitly do so in the constructor.

Ah I see, cool. I removed the constructor.

dmiller9911 · 2018-12-18T03:37:35Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

+  constructor(props) {
+    super(props);
+    this.ref = React.createRef();
+    this.onKeyDown = this.onKeyDown.bind(this);


Same with this. If it is declared as a class property using the arrow notation there is no need to bind here.

dmiller9911 · 2018-12-18T03:38:18Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

+          {...additionalProps}
+          className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)}
+          onKeyDown={this.onKeyDown}
+          ref={ref => (this.ref = ref)}


Since you are using createRef you can simply pass this.ref to it. Then you can access the current ref with this.ref.current

dmiller9911 · 2018-12-18T03:40:27Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownMenu.js

+  }
+
+  keyHandler(index, position, custom = false) {
+    const kids = this.props.children;


This could break if consumers put items in a fragment. It might be better to make use of context to pass things around.

I'm not sure what you mean by using context? I am seeing that having a child that is a React.Fragment will throw an error but should we just handle that when the children get extended (wrapping them in a div) or is there a better method?

The context API https://reactjs.org/docs/context.html

I can move keyHandler and sendRef to a Context but they only get passed down one layer so I'm not sure it saves much. I'm also confused about the case of putting the items in a Fragment because the top level Dropdown component should be expecting an array.

jgiardino · 2019-01-03T21:42:41Z

When I test this component using just my keyboard, it works as expected. When I test in JAWS however, I don't see the behavior where hitting the Down arrow key on the last item shifts focus to the first item.

jgiardino · 2019-01-07T21:58:14Z

Something else I missed in my previous review, when using just the keyboard without screen reader to navigate this component, the user should be able to hit the Tab key while the menu is open to close the menu and shift focus to the next element on the page.

You can see this behavior in the wai-aria example

This behavior is expected for the variations that display a menu, but not for the variation that's labeled "Dropdown Panel" in our preview. In that case, Tab key is now the user would navigate with the contents within the panel.

dlabaj · 2019-01-08T20:25:45Z

packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.test.js

@@ -81,7 +81,7 @@ describe('dropdown', () => {
  test('basic', () => {
    const view = mount(


There are application launcher tests that run similar tests. Will most likely need some updates.

Updated the tests.

Also removed mapping for custom children

Also add focus trap to dropdown panel

Handle tab and enter behavior

update tests, remove duplicate select, set toggle as focus when toggled

jgiardino · 2019-01-11T18:54:21Z

Great job on this!!! 🎉

I am able to test in Chrome, both with VO and normal keyboard interaction, and it works as I'd expect for both cases.

When I try to test in FF or Safari, the Dropdown doesn't even open, but I have been having issues with the pf-react previews (e.g. sometimes they're cached and they don't behave as expected), so I'm not sure if that's an issue with the Dropdown, or with these browsers properly loading these previews. I'm hoping it's the latter :-)

When I test on my phone (Safari or Chrome), I'm able to touch the toggle to close the menu, but touching outside the toggle or menu does nothing. This was something that was fixed in #1152. But I also wouldn't be surprised if this is more of a preview issue than a component issue. I'm trying to test the latest version of the workspace, and the Dropdown there doesn't work on my phone at all.

jgiardino · 2019-01-11T18:55:29Z

UPDATE - I was able to get the preview to load in Firefox. It appears to work the same as in Chrome with one exception... When I click the toggle to display the menu, I should be able to hit the Esc key to dismiss the menu. That works in Chrome, but not in FF.

I'll keep trying to get the other previews to work, but wanted to pass that along.

beanh66 · 2019-01-11T18:57:54Z

Thanks @kmcfaul, this looks great! Worked well when I tried it out in Chrome.

jgiardino · 2019-01-11T21:01:59Z

I was able to get it working in Safari, too. The issue I noted above is also the case in Safari:

When I click the toggle to display the menu, I should be able to hit the Esc key to dismiss the menu.

But then I found this, which might explain why:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus

Most browsers place focus on the button on click, but Safari and Firefox on a mac don't. 😕 Let me know your thoughts on this one. It is an edge case, and the user can still dismiss the menu with other ways, so I'm not sure how much we should try to handle that interaction.

dmiller9911 · 2019-01-13T15:59:18Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

 };

 class DropdownItem extends React.Component {
+  componentWillMount() {


willMount is being deprecated. You can write this like below

class DropdownItem extends React.Component { ref = React.createRef() ...

dmiller9911 · 2019-01-13T16:00:13Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

@@ -70,6 +105,8 @@ class DropdownItem extends React.Component {
              <Component
                {...additionalProps}
                className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)}
+                ref={ref => (this.ref = ref)}


since tyou are already using createRef() this can be written as ref={this.ref}

dmiller9911 · 2019-01-13T16:01:24Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js

+    } else if (event.key === 'ArrowDown') {
+      this.props.context.keyHandler(this.props.index, 'down');
+    } else if (event.key === 'Enter') {
+      this.ref.click();


This, i beleive should be this.ref.current.click(). Also since it is possible to pass a component in you will need to make sure that click exists.

dmiller9911 · 2019-01-13T16:02:57Z

packages/patternfly-4/react-core/src/components/Dropdown/DropdownMenu.js

-    menu = (
-      <DropdownContext.Consumer>
-        {onSelect => (
+class DropdownMenu extends React.Component {


to keep consistent with the repo modify this to be

class DropdownMenu extends React.Component { refsCollection = {}; keyHandler = (index, position, custom = false) => { ... } }

jgiardino

This looks great!
I was able to test this on my phone and verify that it works there.
I think the other observations I noted with FF and Safari can be addressed later if those come up as issues.

tlabaj

LGTM

kmcfaul added PF4 React issues for the PF4 core effort accessibility labels Dec 17, 2018

dmiller9911 reviewed Dec 18, 2018

View reviewed changes

tlabaj requested a review from jgiardino December 19, 2018 17:15

kmcfaul force-pushed the dropdown-arrow-key-support branch from f98ce68 to 11d1c53 Compare January 7, 2019 14:57

dlabaj requested changes Jan 8, 2019

View reviewed changes

kmcfaul force-pushed the dropdown-arrow-key-support branch 2 times, most recently from f93d345 to dd33618 Compare January 9, 2019 20:50

kmcfaul and others added 12 commits January 10, 2019 15:37

feat(Dropdown): add arrow key navigation (unfinished)

6f42f10

arrows

8265405

feat(Dropdown): remove console logs, update tests

16fb213

Also removed mapping for custom children

feat(Dropdown): add arrow key logic to custom children elements

f6b4bd3

fix(Dropdown): remove unneeded binding

0fc9bcc

feat(Dropdown): move create ref out of constructor

a241db0

feat(Dropdown): use Context to pass arrow key eventing

48c6ffc

feat(Dropdown): add accessibility roles for arrow key nav

f72f1a2

Also add focus trap to dropdown panel

test(ApplicationLauncher.test): update Dropdown test snapshots

e6da66f

fix(DropdownItem): fix missing merge change

23e81c6

feat(Dropdown): add keyboard navigation to dropdown toggle

66d26f9

Handle tab and enter behavior

feat(Dropdown): add enter interaction for item

4fabab7

update tests, remove duplicate select, set toggle as focus when toggled

kmcfaul force-pushed the dropdown-arrow-key-support branch from afc6e8a to 4fabab7 Compare January 10, 2019 21:28

dlabaj previously approved these changes Jan 11, 2019

View reviewed changes

dmiller9911 reviewed Jan 13, 2019

View reviewed changes

feat(Dropdown): clean up declarations

e7d42dc

kmcfaul dismissed dlabaj’s stale review via e7d42dc January 14, 2019 16:10

jschuler approved these changes Jan 14, 2019

View reviewed changes

jgiardino approved these changes Jan 14, 2019

View reviewed changes

tlabaj approved these changes Jan 14, 2019

View reviewed changes

tlabaj merged commit 39950dc into patternfly:master Jan 14, 2019

jgiardino mentioned this pull request Jan 15, 2019

Dropdown arrow key interaction #1002

Closed

jgiardino mentioned this pull request Jul 23, 2019

Dropdown: The first item in the dropdown is always selected #2569

Closed

jgiardino mentioned this pull request Jun 24, 2020

unable to use keyboard navigation on dropdown opened via mouse click #3955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dropdown arrow key support #1075

Dropdown arrow key support #1075

kmcfaul commented Dec 17, 2018

kmcfaul commented Dec 17, 2018

patternfly-build commented Dec 17, 2018

coveralls commented Dec 17, 2018 •

edited

Loading

dmiller9911 Dec 18, 2018

kmcfaul Dec 18, 2018

ibolton336 Dec 19, 2018

kmcfaul Jan 7, 2019

dmiller9911 Dec 18, 2018

dmiller9911 Dec 18, 2018

dmiller9911 Dec 18, 2018

kmcfaul Dec 18, 2018

ibolton336 Dec 19, 2018

kmcfaul Jan 7, 2019

jgiardino commented Jan 3, 2019

jgiardino commented Jan 7, 2019

dlabaj Jan 8, 2019

kmcfaul Jan 8, 2019

jgiardino commented Jan 11, 2019

jgiardino commented Jan 11, 2019

beanh66 commented Jan 11, 2019

jgiardino commented Jan 11, 2019

dmiller9911 Jan 13, 2019

dmiller9911 Jan 13, 2019

dmiller9911 Jan 13, 2019

dmiller9911 Jan 13, 2019

jgiardino left a comment

tlabaj left a comment

		@@ -81,7 +81,7 @@ describe('dropdown', () => {
		test('basic', () => {
		const view = mount(

Dropdown arrow key support #1075

Dropdown arrow key support #1075

Conversation

kmcfaul commented Dec 17, 2018

kmcfaul commented Dec 17, 2018

patternfly-build commented Dec 17, 2018

coveralls commented Dec 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 3934

💛 - Coveralls

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

jgiardino commented Jan 3, 2019

jgiardino commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgiardino commented Jan 11, 2019

jgiardino commented Jan 11, 2019

beanh66 commented Jan 11, 2019

jgiardino commented Jan 11, 2019

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

jgiardino left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

coveralls commented Dec 17, 2018 •

edited

Loading