[DateRangeCalendar] Ensure date dragging triggers regardless of trigger element#21868
[DateRangeCalendar] Ensure date dragging triggers regardless of trigger element#21868michelengelen wants to merge 6 commits intomui:masterfrom
Conversation
Signed-off-by: michel <jsnerdic@gmail.com>
michelengelen
added
type: bug
|
Deploy preview: https://deploy-preview-21868--material-ui-x.netlify.app/ Bundle size report
|
flaviendelangle
left a comment
flaviendelangle
left a comment
There was a problem hiding this comment.
Code Review: [DateRangeCalendar] Fix dragging start or end date
PR: #21868 by @michelengelen
Summary: Fixes drag-and-drop of start/end dates in a selected date range by making element lookups more resilient when drag events target child elements rather than the button itself.
What it does
The core issue is that drag events can target child elements inside <button> elements (e.g., a <span> inside the button), meaning event.target may not have the expected data-timestamp or data-position attributes. The fix adds .closest() fallback lookups to find the nearest ancestor with the expected dataset attribute.
Issues & Suggestions
1. Duplicated pattern — extract a helper
The "find element with dataset attribute" pattern is repeated 4 times with slight variations:
const element = target.dataset.timestamp
? target
: target.closest<HTMLElement>('[data-timestamp]');This should be extracted into a small helper like:
const closestWithData = (el: HTMLElement, attr: string) =>
el.dataset[attr] != null ? el : el.closest<HTMLElement>(`[data-${attr}]`);This would reduce duplication and make the intent clearer across all 4 call sites.
2. as any cast
return adapter.date(timestamp as any, timezone);This as any is suspicious. The old code passed an ISO string (new Date(timestamp).toISOString()), the new code passes a raw number. If the adapter's date() method doesn't accept a number, this will fail at runtime while silencing the type checker. The PR should either:
- Keep the ISO string conversion if the adapter expects a string, or
- Fix the type signature if
numberis actually valid input
Either way, as any should not be the answer here.
3. resolveDateFromTarget — instanceof HTMLElement check
if (!target || !(target instanceof HTMLElement)) {
return null;
}This is good defensive coding. However, instanceof HTMLElement can fail across iframes/realms. If the date picker is ever rendered inside an iframe, this check would incorrectly return null. The codebase guidelines mention using owner utilities for realm-sensitive checks — worth verifying this isn't a concern here.
4. Inconsistent element resolution between resolveDateFromTarget and isSameAsDraggingDate
In resolveDateFromTarget, the function first checks instanceof HTMLElement and handles null gracefully. In isSameAsDraggingDate, there's a raw event.target as HTMLElement cast with no null/type guard. These should be consistent — if one needs the guard, both likely do.
5. onDragStart uses event.currentTarget while others use event.target
In the onDragStart handler, the code uses event.currentTarget, but in isSameAsDraggingDate it uses event.target. This inconsistency could lead to different elements being resolved depending on event bubbling. The choice between target and currentTarget should be deliberate and consistent, or at least documented with a comment explaining why they differ.
6. Missing test coverage
The PR description says "fixes dragging of start or end dates" but includes no tests. A test that simulates a drag event targeting a child element of the date button would validate the fix and prevent regression.
Verdict
The fix addresses a real bug (drag events hitting child elements), but the implementation has some rough edges: duplicated lookup patterns, an as any cast that hides a potential type mismatch, and no test coverage. Suggest addressing at least items 1, 2, and 6 before merging.
The as any is a deal breaker here, there is no way to assure that all date libraries accept a number on adapter.date.
LukasTy
changed the title
PR Review: [DateRangeCalendar] Ensure date dragging triggers regardless of trigger elementPR: #21868 ProblemDrag events on SolutionThe fix introduces a What Changed
Flavien's Review Feedback — All Addressed
AnalysisStrengths
Remaining Nits
Neither of these blocks merging. VerdictThe fix correctly addresses the root cause of intermittent drag failures. All of Flavien's review feedback has been incorporated. New |
| * @param event The event object. | ||
| * @returns The target element of the event. | ||
| */ | ||
| export function getTarget(event: Event) { |
There was a problem hiding this comment.
This is pretty much a straight copy from floating ui dom utils, and the below isHTMLElement is inspired by it.
4 participants
This fixes the dragging of the start or end dates of a selected range.