fix(MenuToggle): fix indeterminate checkbox error, toggle click behavior

[kmcfaul]

What: Closes #10034 and #10035

For 10034 - checked needed to use the result of calculateChecked instead of just the isChecked prop (because null needs to be re-resolved to false to prevent the issue).

For 10035 - This one is a little tricky, because the new toggle structure was not set up to wrap or pass the toggle click handler to the splitButtonItems. I've opted to put the onClick on the outer div wrapper, and prevent propagation in the splitButtonItems (on the action button and checkbox input specifically), but we may want to consider which behavior we want. We can use the changes in the PR currently (the whole div becomes clickable), or make the checkbox label text click trigger the checkbox input change handler instead (keeping the toggle button as the only toggle click handler), or let a user pass the onClick manually to the MenuToggleCheckbox.

[patternfly-build]

Preview: https://patternfly-react-pr-10066.surge.sh

A11y report: https://patternfly-react-pr-10066-a11y.surge.sh

[thatblindgeye]

Currently clicking the down caret icon calls the onClick twice (clicking the "X selected" text calls it only once). Looks like it just needs another stopPropagation on the button in the split button wrapper.

I think this may be partially an issue with the markup. The original issue for the onClick portion has trhe expectation that clicking the "10 selected" text would trigger the onClick of the toggle (i.e. expand it). The markup, however, has this text as part of the label element, which I would expect clicking that text to toggle the checkbox state/call the onChange callback.

I wouldn't be opposed to this as a temporary solution, but as a more permanent solution I don't think we should place an onClick on the div wrapper (even though right now it doesn't seem like there's anything preventing keyboard interaction as you'd expect; the down caret icon receives focus for the MenuToggle onClick, and the checkbox receives focus for its onChange). Instead we should go with what you proposed above (having the text trigger the onChange of the checkbox) or consider moving that split button text inside the button element instead of the label to then trigger the MenuToggle's onClick (and then link that text to the label with for/id attributes).

[kmcfaul]

@mattnolting

[kmcfaul]

Per working session conversation, I'm adjusting the click changes to not use a click event on the outer div (which doesn't really make sense and would cause some accessibility issues). Now, clicking the text should check/uncheck the checkbox, as is the usual behavior for checkboxes with labels. Toggling can only be done with the toggle button for splitButtonAction toggles because the behavior is divided between two interactive elements.

[mattnolting]

Looks great, thanks!

[thatblindgeye]

Looks good! I left a comment in patternfly/patternfly#6317 just to mention the convo from here, more of a followup to look into the markup of the split button toggle with text.

[patternfly-build]

Your changes have been released in:

• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]

Thanks for your contribution! 🎉