DatePicker Calendar is not shown when clicking on the Icon when using MediaInput as a trigger

[abdallahzakzouk]

Expectations

Given a usage of Media Input inside a Date Picker as a trigger input, and having an icon as End, Start, or both,
When I click on the Icon, I am expecting the input to be focused and the Calendar to be shown

Reality

On the previous scenario the Input is focused correctly, but the calendar is NOT shown

Steps to Reproduce

1. Use Media Input as a trigger inside Date Picker
2. Add as Icon as End Icon for the media Input
3. View the result, Click on the Icon

Codesandbox Example,

Fine Print

• Component: Date Picker, Media Input
• Version: 8.37.1
• Browsers: all

[abdallahzakzouk]

This can be helpful, from my investigation, I see that in file https://github.com/zendeskgarden/react-components/blob/main/packages/datepickers/src/elements/Datepicker/Datepicker.tsx - Line 202

...
/** Ensure click/focus events from associated labels are not triggered */
if (isInputMouseDownRef.current && !state.isOpen) {
...

This ref and the condition for it are showing the Calendar only on clicking on the Input element itself, to not open the Calendar when clicking on the Wrapper that also include the Label, this may be changed to include any click in the field wrapper including the icons and ignoring only the outer content like Label

[mtomcal]

Hi @abdallahzakzouk. I removed the isInputMouseDownRef.current conditional in a test branch and the Calendar icon remains un-clickable unfortunately. Here is the branch 8914498. If you have time to investigate, perhaps pull down this branch and experiment with different methods to trigger the open event.

And of course, if things come up and let us know. We can add this one to our team's backlog. 👍

[mtomcal]

After looking into this, we added this issue into our backlog for Garden attention. Thanks for reporting this! 👏

[abdallahzakzouk]

I double checked and yes you are right, I found that it should be handled from MediaInput component, not from DatePicker

Thank you Michael for your consideration :)

[jzempel]

The only child provided to Datepicker is cloned with events and props attached directly to it. The (incorrect Garden) assumption is that this child has always been an <input />. In the case of a MediaInput clone, the only child is a div with child input and svg(s) siblings. In this scenario, a click on the svg does not bubble to the input. So the datepicker needs a little help to know how to configure the clone. That's where the refKey prop comes into play in order to point at the outer MediaInput div wrapper. Since "wrapperRef" is the prop of interest on the MediaInput API, we want something like this:

<Field>
  <Label>{Datepicker.displayName}</Label>
  <Datepicker {...args} formatDate={formatDate} isCompact={isCompact} refKey="wrapperRef">
    <MediaInput
        end={<Icon />}
        isCompact={isCompact}
        wrapperProps={{ style: { width: isCompact ? 256 : 320 } }}
    />
  </Datepicker>
</Field>

But this only gets us part-way there. The Garden Datepicker needs to be fixed so that events of interest are attached to the wrapperRef.current rather than to the default. My hypothesis is that we get there by moving the onMouseDown, onMouseUp, and onClick logic into this block, connected to a valid refValue:

react-components/packages/datepickers/src/elements/Datepicker/Datepicker.tsx

Lines 189 to 192 in 1d5b1df

	[refKey!]: (refValue: HTMLElement) => {
	(ref as any)(refValue);
	(inputRef as any).current = refValue;
	},

Local testing bears out this solution. But more would need to be done to thoroughly unit test and also check DatepickerRange for similar defects/fixes.

[jzempel]

If we determine that double-dipping on refKey is undesirable (I can imagine some use cases where tying a click to that ref would be wrong), then another option would be to add a new prop, perhaps triggerPropsKey, that could be used to drop the click events into the proper object. If the prop is undefined, then leave the code as-is.

[abdallahzakzouk]

Thank you @jzempel for your investigation, I think having an extra prop to be used when needed is a good approach if it's not possible to force the behaviour for all cases, please let me if know if something is needed from my side to help doing this change :)