Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DateTimePicker: fix onChange callback check so that it also works inside iframes #54669

Merged
merged 2 commits into from Sep 21, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Sep 20, 2023

What?

Fixes #54378
Inspired by #54643

Improve internal check in DateTimePicker so that the onChange callback fires even when the picker is rendered inside an iframe.

Why?

Making the component more robust by not assuming (in the instanceof check) that it always going to be rendered in the root window.

How?

By grabbing the HTMLInputElement value from the event target's owner window.

Testing Instructions

  1. Apply the following diff:
Click to expand
diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js
index 0c1892c6e4..4dbb6fcda3 100644
--- a/packages/block-library/src/button/edit.js
+++ b/packages/block-library/src/button/edit.js
@@ -15,6 +15,7 @@ import {
 	TextControl,
 	ToolbarButton,
 	Popover,
+	TimePicker,
 } from '@wordpress/components';
 import {
 	AlignmentControl,
@@ -170,6 +171,10 @@ function ButtonEdit( props ) {
 
 	return (
 		<>
+			<TimePicker
+				currentDate={ new Date() }
+				onChange={ ( date ) => console.log( 'logging here: ', date ) }
+			/>
 			<div
 				{ ...blockProps }
 				className={ classnames( blockProps.className, {
  1. In the editor, add a "Buttons" block
  2. Interact with the datetime picker fields, and make sure that the console logs are printed when changing every input field in the component
  3. As a comparison, apply the same diff on trunk and check that only the month dropdown causes the console log to fite
  4. Additionally, check that the callback fires as expected also in Storybook (this can be seen by checking the "actions" tab)

@ciampo ciampo self-assigned this Sep 20, 2023
@ciampo ciampo added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) labels Sep 20, 2023
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Sep 20, 2023
Copy link
Contributor

@bangank36 bangank36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well! ownerDocument.defaultView is good use
All input changes fire correct data on both Storybook and Editor

Editor Storybook
Kapture.2023-09-20.at.22.48.24.mp4
Kapture.2023-09-20.at.22.51.34.mp4

@github-actions
Copy link

Flaky tests detected in b551a18.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6250831673
📝 Reported issues:

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for me 👍

✅ Could replicate original issue on trunk
✅ This PR correctly fires onChange callback for all inputs when inside an iframe
✅ Tested well in the editor and the Storybook

Nice work @ciampo!

@ciampo ciampo merged commit 47e9e06 into trunk Sep 21, 2023
50 checks passed
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Sep 21, 2023
@ciampo ciampo deleted the fix/date-time-picker-instance-of-iframe branch September 21, 2023 08:10
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 21, 2023
mikachan pushed a commit that referenced this pull request Sep 22, 2023
…ide iframes (#54669)

* DateTimePicker: fix onChange callback check so that it also works inside iframes

* CHANGELOG
@mikachan
Copy link
Member

I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: e38d246

@mikachan mikachan removed the Backport to Gutenberg RC Pull request that needs to be backported to a Gutenberg release candidate (RC) label Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

TimePicker onChange event only fires on month change
4 participants