Dropdown interaction issues #1014

kmcfaul · 2018-12-05T16:30:23Z

Issue reference: #984

What: Fix focus trap disabling the dropdown toggle function. Pipe onSelect event to DropdownItem, allowing its disabled property to control if the event is fired. In our example docs, this means that clicking a disabled link will not close the dropdown.

patternfly-build · 2018-12-05T16:45:51Z

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

mcarrano

Looks great. Thanks for making these changes @kmcfaul !

jgiardino · 2018-12-06T23:30:29Z

Thanks for making these updates! I tested across different devices.

Testing this with a mouse, it works as expected.
Testing this with the keyboard, it works as expected.
Testing on an iOS device, I can touch the toggle and collapse the menu. But touching anywhere outside the dropdown toggle or menu does not collapse the menu. Whereas with a mouse, I am able to click anywhere outside the toggle and menu to collapse the menu.
- Since we plan to use the solution implemented for modal in feat(PF4-Modal): Adds keyboard and screen reader focus trapping #1011 with the dropdown panel variation, it might be worth checking if this solution provides any similar interaction. Although I'm assuming we would only implement this for the variation we're calling Dropdown Panel (i.e. custom contents in the popup), and this might not help us with the menu solution.
Testing this with JAWS, NVDA and VO, I am able to place focus on the disabled elements, which is expected (as noted in the APG for Menu—scroll down to the green info box). When I hit the Enter key while a disabled item has focus, the menu dismisses. Unfortunately, I can't find any suggestion on what should happen if the Enter key is pressed at this time. My suggestion would be to ignore Enter key presses when the menu is open and a disabled item has screen reader focus. I don't think this is a major issue if that's not possible.

michaelkro · 2018-12-07T15:01:49Z

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

      <Component
        {...additionalProps}
        className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)}
+        onClick={onSelect}


There is a bit of an issue with always setting onClick to onSelect here. If the dropdown has multiple button items, this will set all of their onClick handlers to onSelect (so all buttons will fire the same action).

One possible solution would be to add a handleOnClick handler to DropdownItem and do something like the following

// ... return ( <li> <Component {...additionalProps} className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)} onClick={event => handleOnClick && handleOnClick(event, onSelect)} > {children} </Component> </li> );

Then the consuming app could do something like

<DropdownItem // ... component="button" handleOnClick={(event, onSelect) => { // fire off the particular action for this button onSelect(); }} > // ... </DropdownItem>

But this would mean that all DropdownItems (even the "link" ones) will require an explicit handleOnClick handler to ensure that onSelect is fired.

michaelkro · 2018-12-07T15:05:38Z

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

 };

-const DropdownItem = ({ className, children, isHovered, component: Component, isDisabled, ...props }) => {
+const DropdownItem = ({ className, children, isHovered, onSelect, component: Component, isDisabled, ...props }) => {


whoops! onSelect needs to be added to the propTypes

coveralls · 2018-12-08T04:20:10Z

Pull Request Test Coverage Report for Build 3880

14 of 16 (87.5%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.07%) to 81.129%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownItem.js	11	12	91.67%
packages/patternfly-4/react-core/src/components/Dropdown/DropdownMenu.js	1	2	50.0%

Totals
Change from base Build 3866:	-0.07%
Covered Lines:	4371
Relevant Lines:	5079

💛 - Coveralls

kmcfaul · 2018-12-11T21:17:57Z

Added onSelect to prop types, updated the onClick to also call the button's onClick method in addition to the onSelect, updated snapshots

michaelkro · 2018-12-12T12:23:03Z

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

        {...additionalProps}
        className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)}
+        onClick={() => {
+          Component === 'button' ? (props.onClick && props.onClick(), onSelect()) : onSelect();


Nice, def cleaner than my suggestion! Just a couple more things :)

Now that we're explicitly setting onClick handler using a prop, onClick should be added to the propTypes, and can be destructured along with the other props that we exclude from ...props

We should probably pass the onClick event object to the callbacks, just in case the consumers need it

Seeing an eslint error here

[eslint] Expected an assignment or function call and instead saw an expression. [no-unused-expressions]

We can do something like the following to get rid of it

onClick={event => { if (Component === 'button') { onClick && onClick(event); onSelect(event); } else { onSelect(event); } }}

That being said, I'm not sure why Travis isn't running prettier, or maybe this particular rule has been removed. Are we not standardizing around prettier and eslint? @jschuler

michaelkro · 2018-12-14T12:25:24Z

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

            position={position}
            aria-labelledby={id}
-            onClick={event => onSelect && onSelect(event)}
+            onSelect={event => onSelect && onSelect(event)}


One last thing, I'd like to discuss the semantics of using onSelect as a handler for click events.

The native select event is fired when some text is being selected, whereas, the native change event is fired for select elements when a change to the element's value is committed by the user.

Since this is a custom implementation, and we're actually hooking into the click event, technically, it does not matter what we call our event handler. In fact, just to make sure, I went through and changed all the onSelects to onSomeEvent, and things seem to work just fine.

However, since this is going to become part of <Dropdown />'s API, I figured it was worth bringing up

Actually, now that I think about it again, I realize that <Dropdown /> isn't actually a select... it's a menu. So onChange doesn't necessarily make sense either.

This is sort of a weird use case here... What do you think we should do?

This may be more of a case of bad nomenclature, as this onSelect isn't a property of the DropdownMenu but a prop that gets passed down to the DropdownItem itself where it is set to the onClick function.

Suggested change

onSelect={event => onSelect && onSelect(event)}

onSelect={onSelect}

jschuler · 2018-12-19T18:51:46Z

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

        className={css(isDisabled && styles.modifiers.disabled, isHovered && styles.modifiers.hover, className)}
+        onClick={event => {
+          if (Component === 'button') {
+            onClick && onClick(event);


Why both onClick and onSelect?

I see, onSelect is added in the Menu. Maybe remove from propTypes so it is only documented that user can add onClick to the item?

Actually, nevermind, better would be to use context i think

I think it would be best to create a context in Dropdown, and then pass the onSelect through that instead of props. Additionally, when you return the onSelect to the user, apart from the event maybe also return the index

michaelkro · 2019-01-08T20:13:33Z

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

-  } else if (Component === 'button') {
-    additionalProps.disabled = isDisabled;
-  }
+class DropdownItem extends React.Component {


Out of curiosity, what is the reasoning behind switching to a class based component? ~~In any case, if we decide to keep this as a class, we can switch the context stuff over to the contextType syntax~~

EDIT: hmm, maybe not on the contextType. Trying to get it working locally and it's being weird.

michaelkro · 2019-01-08T20:21:04Z

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

+                    if (Component === 'button') onClick && onClick(event);
+                    onSelect && onSelect(event);
+                  } else {
+                    Function.prototype;


Getting an eslint error here

Expected an assignment or function call and instead saw an expression.eslint(no-unused-expressions)

I think, if we want the handler to do nothing in case isDisabled is true, we can just omit this else block

Yeah I think you are right, I will remove the block

michaelkro · 2019-01-08T20:23:19Z

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

  isDisabled: false,
-  href: '#'
+  href: '#',
+  onSelect: Function.prototype,


Missing from propTypes :)

Oops, I'm wrong, we need to do the opposite. I think we need to remove onSelect from defaultProps

Must've missed removing it when I took out the proptypes, will remove

Select event should not fire when disabled actions in a menu are selected.

…p toggle

disabled items may be enabled by application code, so should still have the onSelect attached.

Ensures that if the component is a button, the unique onClick is still fired in addition to the onSelect

n

update tests, rebase and resolve conflicts

tlabaj

LGTM

jgiardino

Same here, since the issue I noted about touch is captured in a separate issue.

jschuler · 2019-01-09T21:35:41Z

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

-  '': PropTypes.any
+  '': PropTypes.any,
+  /** Callback for click event */
+  onClick: PropTypes.func


Also need to update the props in the DropdownItem.d.ts file?

I will add the onClick to the ts file

tlabaj requested review from jgiardino, mcarrano and michaelkro December 6, 2018 15:46

tlabaj added PF4 A11y labels Dec 6, 2018

mcarrano previously approved these changes Dec 6, 2018

View reviewed changes

michaelkro reviewed Dec 7, 2018

View reviewed changes

kmcfaul dismissed mcarrano’s stale review via 6644e80 December 11, 2018 21:16

kmcfaul force-pushed the dropdown-interaction-issues branch from 15046f2 to 6644e80 Compare December 11, 2018 21:16

michaelkro reviewed Dec 12, 2018

View reviewed changes

michaelkro reviewed Dec 14, 2018

View reviewed changes

jschuler reviewed Dec 19, 2018

View reviewed changes

jgiardino mentioned this pull request Jan 3, 2019

fix(dropdown): add focus trap option that allows close on click #1126

Merged

kmcfaul force-pushed the dropdown-interaction-issues branch 2 times, most recently from cd03a63 to fd5f478 Compare January 7, 2019 14:41

michaelkro reviewed Jan 8, 2019

View reviewed changes

mturley mentioned this pull request Jan 8, 2019

Dropdown component does not close on touch event outside of the focused area #1131

Closed

kmcfaul force-pushed the dropdown-interaction-issues branch from fd5f478 to 1857284 Compare January 9, 2019 14:32

kmcfaul added 4 commits January 9, 2019 15:34

feat(Dropdown): disable selection of disabled menu items

a94385b

Select event should not fire when disabled actions in a menu are selected.

feat(Dropdown): improved select event piping, added fix for focus tra…

8fd199f

…p toggle

feat(Dropdown): assign onClick to disabled items, update tests

8a451e9

disabled items may be enabled by application code, so should still have the onSelect attached.

feat(Dropdown): handle both button onclick and list onselect

32937c8

Ensures that if the component is a button, the unique onClick is still fired in addition to the onSelect

kmcfaul added 4 commits January 9, 2019 15:36

refactor(add onClick to proptypes, pass event to functions):

bc772a2

n

feat(Dropdown): use Context to pass onSelect handlers

7dd2683

feat(Dropdown): remove onSelect default

2641669

update tests, rebase and resolve conflicts

test(Dropdown): update tests from rebase

8640789

kmcfaul force-pushed the dropdown-interaction-issues branch from 1857284 to 8640789 Compare January 9, 2019 20:42

tlabaj previously approved these changes Jan 9, 2019

View reviewed changes

jgiardino previously approved these changes Jan 9, 2019

View reviewed changes

jschuler reviewed Jan 9, 2019

View reviewed changes

feat(DropdownItem): add new prop to ts file

ccd4cf3

kmcfaul dismissed stale reviews from jgiardino and tlabaj via ccd4cf3 January 9, 2019 21:52

tlabaj approved these changes Jan 10, 2019

View reviewed changes

jschuler approved these changes Jan 10, 2019

View reviewed changes

jschuler merged commit a72d4e6 into patternfly:master Jan 10, 2019

tlabaj mentioned this pull request Jan 15, 2019

Dropdown Component Audit #956

Closed

	onSelect={event => onSelect && onSelect(event)}
	onSelect={onSelect}

Dropdown interaction issues #1014

Dropdown interaction issues #1014

Uh oh!

Conversation

kmcfaul commented Dec 5, 2018

Uh oh!

patternfly-build commented Dec 5, 2018

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Dec 6, 2018

Uh oh!

michaelkro Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3880

💛 - Coveralls

Uh oh!

kmcfaul commented Dec 11, 2018

Uh oh!

michaelkro Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschuler Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelkro Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

jgiardino left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

michaelkro Dec 7, 2018 •

edited

Loading

coveralls commented Dec 8, 2018 •

edited

Loading

michaelkro Dec 12, 2018 •

edited

Loading

jschuler Dec 19, 2018 •

edited

Loading

michaelkro Jan 8, 2019 •

edited

Loading