Enhance: Added Dialog Wrapper for `EntitiesSavedStatesExtensible` #67352

Mayank-Tripathi32 · 2024-11-27T15:16:14Z

Attempt to resolve #67313, Reference PR: #67327

Alternate PR to consider: #67351

What?

This PR removes the renderDialog prop from EntitiesSavedStates and EntitiesSavedStatesExtensible, and moves the dialog functionality into a new wrapper function, EntitieDialogWrapper.

Why?

The issue occurs when clicking on the 'padding area' of the Entity saved modal dialog, causing the dialog to close unexpectedly.

Animated GIF to illustrate:

How?

This PR introduces a new wrapper function, EntitieDialogWrapper, to handle the dialog functionality inside the editor's "save-publish-panel". Additionally, the renderDialog prop has been removed from EntitiesSavedStates and EntitiesSavedStatesExtensible, as it is no longer necessary. This ensures that EntitiesSavedStatesExtensible can be used with Modal in site-editor without any dialog logic.

Testing Instructions

Go to the Site editor.
Make a simple change to a global style, e.g. change a color.
Reopen the Navigation panel by clicking 'Open Navigation' at the top left of the screen.
Click any item in the navigation other than 'Styles' e.g. click 'Pages' or 'Templates'.
At this point, a blue button at the bottom of the Navigation panel appears, labeled 'Review 1 change...'

Click the button: the modal dialog to save entities opens.
Click the padding area of the modal dialog.
Observe the modal dialog closes.
Alternatively preview a block theme from the WP admin > Appearance > any block theme > Live Preview.
Click the blue button labeled 'Activate {theme name}' at the bottom of hte Navigation panel: the modal dialog to save entities opens.
Click the padding area of the modal dialog.
Observe the modal dialog closes.

github-actions · 2024-11-27T15:16:31Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Mayank-Tripathi32 <mayanktripathi32@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-11-27T15:17:00Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Mayank-Tripathi32! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

Mayank-Tripathi32 · 2024-11-27T16:17:24Z

@youknowriad Please have a look when you get time 🤞 Thanks!

youknowriad · 2024-11-27T19:01:19Z

Thanks for the PR this is close to what I had in mind. There's some consideration to check about backwards compatibility of the component ... but important thing for now is to get some reviews for folks.

afercia · 2024-11-28T14:32:14Z

A few considerations:

Please have a look at a simpler approach in #67351. You will notice a couple things:

There are more places where the modal behavior and the role and ARIA attributes need to be added, for example for the EntitiesSavedStatesForPreview that is used in the save panel of the Theme preview. And they need to be added conditionally, only then EntitiesSavedStatesForPreview is used in the save pael, not when used in the save modal dialog. So that the new wrapper component should be exported and reused in other places.
For a more complete fix, all the buttons that open the save panel with modal behavior and semantics, need to have an aria-haspopup="dialog" attribute.

More importantly, while the wrapper approach may seem nicer, I think it makes more difficult to reuse the uniwue IDs generated for the aria-labelledby and aria-describedby attributes.

In this PR these IDs are now generated in the EntitieDialogWrapper component. However, these IDs need to be actually used inside the content of EntitiesSavedStatesExtensible. Howe However, this PR removes the IDs here and here.

In fact, the new wrapper element that is rendered around the element with class entities-saved-states__panel does have a role=dialog, does have an aria-labelledby and aria-describedby but the references IDs point to non-existing IDs. As such, the dialog is unlabelled and there is no description:

Lastly in this PR the save panel in the Thene preview doesn't open at all (because it misses the wrapper).

afercia · 2024-11-28T14:33:39Z

packages/editor/src/components/entities-saved-states/index.js

@@ -160,16 +130,13 @@ export function EntitiesSavedStatesExtensible( {
 			</Flex>

 			<div className="entities-saved-states__text-prompt">
-				<div
-					className="entities-saved-states__text-prompt--header-wrapper"
-					id={ renderDialog ? dialogLabel : undefined }


removing this id will make the dialog labeling fail

I was thinking that might be the case, Might have to look into an alternate way to pass down Ids maybe?

yes but I would argue that given we need to pass down the unique ID, how this would be better than using the current approach that passed down the renderDialog prop? The only difference would be that the logic is abstracted in a wrapper component but I'm not sure that would be a substantial improvement code-wise.

True, but looking at the original code, it seems that the aria tags were missing anyway. I’m not entirely sure how much impact the aria tags would have, but we could consider #67351

True, they were missing because the renderDialog prop was not passe down correctly. Looking at the original PR that introduced thie behavior #59622, the intent is to make all 'Save panels' behave like a sort of modal dialog and use the useDialog props and also the IDs for aria-labelledby and aria-describedby.

afercia · 2024-11-28T14:34:01Z

packages/editor/src/components/entities-saved-states/index.js

 					<strong className="entities-saved-states__text-prompt--header">
 						{ __( 'Are you ready to save?' ) }
 					</strong>
 					{ additionalPrompt }
 				</div>
-				<p id={ renderDialog ? dialogDescription : undefined }>


removing this id will make the dialog aria-describedby fail

afercia · 2024-11-28T14:35:26Z

packages/editor/src/components/save-publish-panels/index.js

+ *
+ * @return {React.Element} The rendered dialog element with children.
+ */
+export function EntitieDialogWrapper( { children, close } ) {


this new wrapper should be used in other palces as well, for example for the save panel in the theme preview, see EntitiesSavedStatesForPreview

The name of the component should make clear it is meant as a wrapper for EntitiesSavedStates.

Noted. I only worked on save panel as If it was acceptable solution then probably can add to theme preview aswell.

afercia

I left a few points to address

afercia · 2024-11-28T14:39:21Z

packages/editor/src/components/save-publish-panels/index.js

+			aria-labelledby={ dialogLabel }
+			aria-describedby={ dialogDescription }
+		>
+			{ ' ' }


Why this empty space?

seems to be added by npm run format, will remove it now. Thanks

Mayank-Tripathi32 · 2024-11-28T16:53:10Z

<EntitiesSavedStatesExtensible
	 { ...{
	 ...isDirtyProps,
	 additionalPrompt,
	 close: onClose,
	 onSave,
	 saveEnabled: true,
	 saveLabel: activateSaveLabel,
	 } }
  />

Since EntitiesSavedStatesExtensible Is used with dialog in save-panel inside edit-site package as well, I think we may need to stick with modal for it. I don't think It makes any sense to import the dialog wrapper to wrap EntitiesSavedStatesExtensible here.

Renamed wrapper to EntitiesSavedStatesDialogWrapper
Exported wrapper from 'packages/editor/src/components/entities-saved-states/index.js` instead.
Updated Theme preview to conditionally use the wrapper.

Also, It might make more sense to clean out components a little as well.

cc: @afercia

~~Alternatively we can go with approach of fixing renderDialog prop instead, It will require less changes overall and also addresses the aria-label issue.~~

… updated preview panel to use wrapper

afercia · 2024-11-29T15:05:40Z

It might make more sense to clean out components a little as well

My feeling is that a wrapper would be 'nicer' from a code style perspecitve but in order to fix all the use cases the whole implementation should be cleaned out a little and better abstracted, as you pointed out. That would require soe larger refacttoring and I'm not sure it is worth it.

At the end we would have a wrapper but still we would need to pass down the unique IDs to the wrapped component or find an alternative way (?) to render those IDs correctly. Basically a wrapper cannot 'attach' all the behaviors and props we need automatically and will still need to pass down props.

t-hamano

Thanks for the PR!

If I make some changes like below I should be able to apply the id correctly but I'm not sure if this is the right approach 😅

Diff

diff --git a/packages/editor/src/components/entities-saved-states/index.js b/packages/editor/src/components/entities-saved-states/index.js
index b4fdafacf0..d207a5a649 100644
--- a/packages/editor/src/components/entities-saved-states/index.js
+++ b/packages/editor/src/components/entities-saved-states/index.js
@@ -4,8 +4,10 @@
 import { Button, Flex, FlexItem } from '@wordpress/components';
 import { __, _n, sprintf } from '@wordpress/i18n';
 import {
+       Children,
        useCallback,
        useRef,
+       cloneElement,
        createInterpolateElement,
 } from '@wordpress/element';
 import { useDispatch } from '@wordpress/data';
@@ -34,10 +36,14 @@ function identity( values ) {
  *
  * @return {JSX.Element} The rendered component.
  */
-export default function EntitiesSavedStates( { close } ) {
+export default function EntitiesSavedStates( { close, ...additionalProps } ) {
        const isDirtyProps = useIsDirty();
        return (
-               <EntitiesSavedStatesExtensible close={ close } { ...isDirtyProps } />
+               <EntitiesSavedStatesExtensible
+                       close={ close }
+                       { ...additionalProps }
+                       { ...isDirtyProps }
+               />
        );
 }
 
@@ -54,7 +60,8 @@ export default function EntitiesSavedStates( { close } ) {
  * @param {boolean}  props.isDirty               Flag indicating if there are dirty entities.
  * @param {Function} props.setUnselectedEntities Function to set unselected entities.
  * @param {Array}    props.unselectedEntities    Array of unselected entities.
- *
+ * @param            props.labelId
+ * @param            props.descriptionId
  * @return {JSX.Element} The rendered component.
  */
 export function EntitiesSavedStatesExtensible( {
@@ -67,6 +74,8 @@ export function EntitiesSavedStatesExtensible( {
        isDirty,
        setUnselectedEntities,
        unselectedEntities,
+       labelId,
+       descriptionId,
 } ) {
        const saveButtonRef = useRef();
        const { saveDirtyEntities } = unlock( useDispatch( editorStore ) );
@@ -134,13 +143,16 @@ export function EntitiesSavedStatesExtensible( {
                        </Flex>
 
                        <div className="entities-saved-states__text-prompt">
-                               <div className="entities-saved-states__text-prompt--header-wrapper">
+                               <div
+                                       className="entities-saved-states__text-prompt--header-wrapper"
+                                       id={ labelId }
+                               >
                                        <strong className="entities-saved-states__text-prompt--header">
                                                { __( 'Are you ready to save?' ) }
                                        </strong>
                                        { additionalPrompt }
                                </div>
-                               <p>
+                               <p id={ descriptionId }>
                                        { isDirty
                                                ? createInterpolateElement(
                                                                sprintf(
@@ -193,6 +205,13 @@ export function EntitiesSavedStatesDialogWrapper( { children, close } ) {
                'description'
        );
 
+       const childrenWithId = Children.map( children, ( child ) => {
+               return cloneElement( child, {
+                       labelId: dialogLabel,
+                       descriptionId: dialogDescription,
+               } );
+       } );
+
        return (
                <div
                        ref={ saveDialogRef }
@@ -201,7 +220,7 @@ export function EntitiesSavedStatesDialogWrapper( { children, close } ) {
                        aria-labelledby={ dialogLabel }
                        aria-describedby={ dialogDescription }
                >
-                       { children }
+                       { childrenWithId }
                </div>
        );
 }

t-hamano · 2024-11-30T13:09:50Z

packages/edit-site/src/components/save-panel/index.js

@@ -11,6 +11,7 @@ import {
 	EntitiesSavedStates,
 	useEntitiesSavedStatesIsDirty,
 	privateApis,
+	EntitiesSavedStatesDialogWrapper,


I think we should expose it as a private API initially, So, define it here:

gutenberg/packages/editor/src/private-apis.js

Line 10 in 9610000

import { EntitiesSavedStatesExtensible } from './components/entities-saved-states';

afercia · 2024-12-02T09:28:33Z

Thanks for the PR!

If I make some changes like below I should be able to apply the id correctly but I'm not sure if this is the right approach 😅

Looks a little overkill to me. Wouldn't a simpler approach be better?

ntsekouras · 2024-12-02T10:30:41Z

Thanks for the PR! I'm looking for a bit the issue, previous attempts and current suggestions (this one and the alternative).

While I also think the wrapper is better semantically I'm not sure it's worth it to add the complexity for also passing ids and the possible backwards compatibility issues by removing this flag. I lean more to the alternative PR.

Mayank-Tripathi32 added 3 commits November 27, 2024 20:27

feat: added wrapper for dialog behaviour

fca00ad

chore: removed renderDialog prop & relating behaviour

8c4caa5

chore: removed extra props "renderDialog" as no longer required

d977929

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 27, 2024

Mayank-Tripathi32 changed the title ~~Try/fix 67313 fix add dialog wrapper editor~~ Enhance: Added Dialog Wrapper for EntitiesSavedStatesExtensible Nov 27, 2024

Mayank-Tripathi32 mentioned this pull request Nov 27, 2024

Enhance: Added close on outside click flag & updated save post panel #67327

Closed

youknowriad requested review from Mamaduka, afercia, ntsekouras and t-hamano November 27, 2024 19:01

afercia added [Type] Bug An existing feature does not function as intended [Package] Editor /packages/editor [Package] Edit Site /packages/edit-site labels Nov 28, 2024

afercia reviewed Nov 28, 2024

View reviewed changes

afercia requested changes Nov 28, 2024

View reviewed changes

afercia reviewed Nov 28, 2024

View reviewed changes

chore: updated wrapper name, exported it from entities-saved-states &…

0db9b84

… updated preview panel to use wrapper

t-hamano reviewed Nov 30, 2024

View reviewed changes

afercia mentioned this pull request Dec 2, 2024

Documentation: EntitiesSavedStates editor component #62377

Merged

Mayank-Tripathi32 closed this Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance: Added Dialog Wrapper for `EntitiesSavedStatesExtensible` #67352

Enhance: Added Dialog Wrapper for `EntitiesSavedStatesExtensible` #67352

Mayank-Tripathi32 commented Nov 27, 2024

github-actions bot commented Nov 27, 2024 •

edited

Loading

github-actions bot commented Nov 27, 2024

Mayank-Tripathi32 commented Nov 27, 2024

youknowriad commented Nov 27, 2024

afercia commented Nov 28, 2024

afercia Nov 28, 2024

Mayank-Tripathi32 Nov 28, 2024

afercia Nov 29, 2024

Mayank-Tripathi32 Nov 29, 2024

afercia Nov 29, 2024

afercia Nov 28, 2024

afercia Nov 28, 2024

afercia Nov 28, 2024

Mayank-Tripathi32 Nov 28, 2024

afercia left a comment

afercia Nov 28, 2024

Mayank-Tripathi32 Nov 28, 2024

Mayank-Tripathi32 commented Nov 28, 2024 •

edited

Loading

afercia commented Nov 29, 2024

t-hamano left a comment

t-hamano Nov 30, 2024

afercia commented Dec 2, 2024

ntsekouras commented Dec 2, 2024

Enhance: Added Dialog Wrapper for EntitiesSavedStatesExtensible #67352

Enhance: Added Dialog Wrapper for EntitiesSavedStatesExtensible #67352

Conversation

Mayank-Tripathi32 commented Nov 27, 2024

What?

Why?

How?

Testing Instructions

github-actions bot commented Nov 27, 2024 • edited Loading

github-actions bot commented Nov 27, 2024

Mayank-Tripathi32 commented Nov 27, 2024

youknowriad commented Nov 27, 2024

afercia commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mayank-Tripathi32 commented Nov 28, 2024 • edited Loading

afercia commented Nov 29, 2024

t-hamano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Dec 2, 2024

ntsekouras commented Dec 2, 2024

Enhance: Added Dialog Wrapper for `EntitiesSavedStatesExtensible` #67352

Enhance: Added Dialog Wrapper for `EntitiesSavedStatesExtensible` #67352

github-actions bot commented Nov 27, 2024 •

edited

Loading

Mayank-Tripathi32 commented Nov 28, 2024 •

edited

Loading