Migrate post privacy confirmation from `confirm()` to `ConfirmDialog` #37602

chad1008 · 2021-12-22T20:22:00Z

Description

This PR aims to migrate the post editor's Switch to draft button away from the current confirm() implementation and instead use the new experimental ConfirmDialog component.

How has this been tested?

Before testing, cherrypick #37959 on top of this PR. That will ensure the ConfirmDialog has the proper z-index to render on top of its parent component.

Running WordPress 5.8.2 via wp-env:

Create and publish a new post
In the Post > Status & visibility settings, click the current visibility (Public) to open the popover
Choose Private and look for any unexpected console errors
Click Cancel and confirm that the post's visibility is not changed (no toast notification, and it should still appear private if you check the post list in another tab).
Open the Visibility popover again, and this time click OK
Confirm there are no unexpected console errors
Confirm the post updates to Private. You should see the usual updating animation on the Update/Publish button, and a toast notification should appear when complete
Confirm the post has been updated to Private.

I've tested in the latest Chrome, Firefox, and Safari.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
ironment, tests ran to see how -->

chad1008 · 2022-01-07T21:59:08Z

Updated this PR to simplify the implementation of ConfirmDialog (cc @ciampo)

ciampo

Hey @chad1008 , thank you for working on this!

I noticed that the dropdown with the Post visibility options sits on top of the confirm dialog.

Otherwise, my feedback is basically the same as the one I left in PR 37491, including the consideration about unit/e2e tests.

packages/editor/src/components/post-visibility/index.js

chad1008 · 2022-01-11T21:22:42Z

Thanks for the review @ciampo!

I noticed that the dropdown with the Post visibility options sits on top of the confirm dialog.
Good point. I'll need to look into this a bit more. The easiest approach would be to modify the DOM directly and set something like style.visibility('hidden') on the dropdown while the confirmation is open, but that felt a little janky.

I also tried lifting the ConfirmDialog up a level in the component tree, and got it set up so that opening the confirmation stopped the dropdown from rendering... but not rendering the dropdown means the ConfirmDialog looses the ability (at least when paired with my current knowledge of React) to invoke the necessary functions to actually process the post update. I'll have to play with a bit more, but feel free to share any thoughts you have :)

Otherwise, my feedback is basically the same as the one I left in PR 37491, including the consideration about unit/e2e tests.

Excellent, thank you again. I'll look into the e2e side of things and circle back.

chad1008 · 2022-01-13T22:04:29Z

Regarding the PostVisibility dropdown rendering on top of the confirmation dialog:

There's a z-index issue behind this that I've proposed a change for in #37959.

See related comment here :)

ciampo · 2022-01-21T09:33:00Z

Update:

I marked all of the memoization-related conversations as resolved for now, as discussed in this comment.
there's still an open comment that needs addressing
not sure if you managed to look into fixing any broken e2e/unit tests (if any) and potentially adding any new ones

There's a z-index issue behind this that I've proposed a change for in #37959.

Thank you for looking into the problem and opening #37959! As also explained in #37535 (comment), we'll likely have to wait until this issue is fixed before being able to merge this PR.

fullofcaffeine · 2022-01-25T01:37:06Z

not sure if you managed to look into fixing any broken e2e/unit tests (if any) and potentially adding any new ones

Agreed on adding a new E2E with some examples to test the Set to Private + cancel and confirm scenarios (the ones where the dialog is actually used). @chad1008 let me know if you need help with that :)

fullofcaffeine · 2022-01-25T02:09:27Z

Agreed on adding a new E2E with some examples to test the Set to Private + cancel and confirm scenarios (the ones where the dialog is actually used). @chad1008 let me know if you need help with that :)

Ah, there's already an E2E for this feature here (and it's currently failing in this PR), though I don't understand at first glance how it was supposed to deal with the blocking confirm call. There are other E2Es failing as part of this changeset, but some look flaky (like specs/site-editor/template-part.test.js). I'll have a look again tomorrow.

fullofcaffeine · 2022-01-25T16:59:03Z

though I don't understand at first glance how it was supposed to deal with the blocking confirm call

It looks like it was bypassing the logic that shows the native confirm dialog on purpose, and that makes sense: I assume there's no way to test a blocking confirm call given the orchestration code will also be blocked. Good news is that we will be able to include the dialog in this test now that we're using ConfirmDialog :)

chad1008 · 2022-01-25T22:05:52Z

I've updated packages/e2e-tests/specs/editor/various/post-visibility.test.js to account for the new ConfirmDialog.

I'd also like to add a test that ensures the Cancel button of the dialog works as expected, but I'm running into an odd error that the button isn't clickable when the test runs, that I'll need to investigate further.

For some reason

await cancelButton.click();

fails and returns that the node is either not clickable or not an HTMLElement... but

await cancelButton.evaluate( ( b ) => b.click() );

works perfectly. Not sure why Puppeteer's click() is having trouble where JavaScript's isn't, but I'll investigate more tomorrow.

chad1008 · 2022-01-26T20:38:30Z

packages/e2e-tests/specs/editor/various/post-visibility.test.js

+			await page.waitForSelector( '.components-confirm-dialog' );
+
+			const cancelButton = await page.waitForSelector(
+				'.components-confirm-dialog button.is-tertiary'


I'd prefer to locate the Cancel button using an xpath selection rather than CSS, but for reasons I haven't yet identified,
page.$x( '//button[text()="Cancel"]' )
and
page.waitForXPath( '//button[text()="Cancel"]' )
both cause Puppeteer's ElementHandle.click() to error out stating the node being passed in isn't visible or isn't an HTML element. The only way I've found to get either of those to successfully click the button is to swap out the click() method as described previously.

So basically it seems like a choice between the alternate click() method, or the CSS selector... I'm not sure which would be the lesser of two evils...

I'm not an expert of Puppeteer and XPath.

the node being passed in isn't visible or isn't an HTML element.

This, paired with the previous comment about having to await, makes me think that you may be trying to access the Cancel button too early? Maybe the dialog is still opening?

I've found out what's wrong: there are actually two buttons on the page with the Cancel inner text that happened to match that XPath expression. We were selecting the wrong one, which happened to not be accessible at the time puppeteer tried to click it (haven't dug into it, though).

The fix is as simple as changing the XPath expression to //div[contains(@class, "components-confirm-dialog")]//button[text()="Cancel"]'). This still has the downside of relying on a CSS class (components-confirm-dialog) which might change in the future but is slightly better than relying on two CSS classes, which is the case with the CSS selector now. We could also traverse to the dialog div by role (which has a value of dialog atm).

On the other hand, I think it's not a huge deal to leave it with the CSS selector you've used, @chad1008.

Instead of using the components-confirm-dialog classname, could we use something like role="dialog" ?

Alternatively, if/when we will add props for confirm/cancel button labels, we'll be able to add a unique label that will avoid the problem altogether

Instead of using the components-confirm-dialog classname, could we use something like role="dialog" ?

~~Yep, we can do that 👍🏻~~

I think the best trade-off is to use the //div[contains(@class, "components-confirm-dialog")]//button[text()="Cancel"]') xpath.

It seems there are more divs on the page with the same role, so it's not specific enough for our use-case :(

This may be worth looking into a little bit — it's weird that there are 2 open dialogs in the page (with 2 cancel buttons). It almost sounds as if the ConfirmDialog is being rendered twice? And if not the same ConfirmDialog, it's still weird that there are 2 open dialogs in the same page?

I think the best trade-off is to use the //div[contains(@class, "components-confirm-dialog")]//button[text()="Cancel"]') xpath.

One last attempt: what about[role='dialog'] > [role='document'] > button[text='Cancel]` (of course translated to the correct xpath syntax)?

If nothing else works, we can definitely go with your suggestion. But I'd definitely look a bit more into why there isn't only one "Cancelbutton inside adialog` in the page.

But I'd definitely look a bit more into why there isn't only one "Cancelbutton inside adialog` in the page.

Found it. It's the core UI link inserter:

It gets rendered when the page loads, but is hidden by a couple of display: none on #wp-link-wrap (one inline style and one added via editor.css). The modal (and our phantom button) only actually appear on screen in the Classic Editor when adding a link to text on the page.

I'll work on an alternate XPath with this in mind and update when I have something!

//*[@role="dialog"][not(@id="wp-link-wrap")]//button[text()="Cancel"] should do the trick. (423ff74)

We're relying on the other button's wrapper not to go and change its ID on us, but I don't know that we have a better way to differentiate between the two. Thoughts? 🙂

Of all alternatives, this is my favourite! This test will break only if:

the dialog's id changes

a new dialog with a "Cancel" button is added

In both scenarios, it should be quite easy to understand that the change caused the test failure.

I also think that we should update this test as soon as we introduce props for changing the confirm/cancel button labels on ConfirmDialog:

Alternatively, if/when we will add props for confirm/cancel button labels, we'll be able to add a unique label that will avoid the problem altogether

ciampo · 2022-01-31T19:48:47Z

I'll give this PR a proper look tomorrow and hopefully we'll be able to merge!

ciampo

Code changes LGTM, and the dialog now appears correctly on top of the post visibility dropdown — also extra props for updating and improving the e2e tests!

I just left a few minor comments regarding the selectors used in the e2e tests, but once those are addressed this PR is ready to be merged 🎉

packages/e2e-tests/specs/editor/various/post-visibility.test.js

chad1008 · 2022-02-01T22:31:12Z

Thanks for the e2e review @ciampo. I was focused on tests for one of the other PRs today, but I'll address your suggestions here in the morning!

Co-authored-by: Marco Ciampini <[email protected]>

…enders

…ing post visibility

chad1008 · 2022-02-02T12:36:19Z

Updated tests based on @ciampo's helpful suggestions ✅

ciampo

Thank you @chad1008 for addressing all feedback! Changes LGTM and test well as per instruction (and the newly added e2e tests!)

As next steps, I see 2 separate tasks:

Add props to ConfirmDialog to allow its consumers to set custom text labels for Confirm and Cancel buttons (which will also allow us to remove some of "hacky" CSS selectors in the e2e tests)
Make sure that we have a good UX in place when setting the post visibility fails for some reason (more details in the comment below)

ciampo · 2022-02-04T14:41:39Z

packages/editor/src/components/post-visibility/index.js

+	confirmPrivate = () => {
 		const { onUpdateVisibility, onSave } = this.props;

 		onUpdateVisibility( 'private' );
-		this.setState( { hasPassword: false } );
+		this.setState( {
+			hasPassword: false,
+			showPrivateConfirmDialog: false,
+		} );
 		onSave();
-	}
+	};


As a future improvement, it'd be good to investigate what happens in case of failure to set the post visibility to private (for example, if the internet connection stops working of is the server returns an error).

How do the onUpdateVisibility and onSave functions behave? In case of error, should we close the dialog and display a message? Or should we keep the dialog open until we have a confirmation of success, and potentially show an error message within the dialog?

This task is actually partially related to the conversation in #37492 (comment) regarding adding a isDisabled prop to the ConfirmDialog component

ciampo self-requested a review January 3, 2022 16:18

ciampo self-assigned this Jan 3, 2022

ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components labels Jan 3, 2022

ciampo assigned chad1008 and unassigned ciampo Jan 3, 2022

ciampo requested a review from mirka January 3, 2022 16:20

chad1008 force-pushed the migrate/post-visibility-confirm-dialog branch from cb75532 to 60d22f3 Compare January 7, 2022 21:57

ciampo requested a review from fullofcaffeine January 9, 2022 17:44

ciampo reviewed Jan 9, 2022

View reviewed changes

packages/editor/src/components/post-visibility/index.js Outdated Show resolved Hide resolved

packages/editor/src/components/post-visibility/index.js Outdated Show resolved Hide resolved

chad1008 mentioned this pull request Jan 13, 2022

Increase ConfirmDialog overlay's z-index to render above popovers #37959

Merged

6 tasks

chad1008 force-pushed the migrate/post-visibility-confirm-dialog branch from 45dd8ac to 2d0cfa5 Compare January 25, 2022 18:02

chad1008 requested review from ajitbohra, nerrad and ntwb as code owners January 25, 2022 22:01

chad1008 commented Jan 26, 2022

View reviewed changes

chad1008 force-pushed the migrate/post-visibility-confirm-dialog branch from 8601043 to e2511b0 Compare January 31, 2022 15:19

ciampo approved these changes Feb 1, 2022

View reviewed changes

chad1008 and others added 7 commits February 2, 2022 07:33

migrate post privacy confirmation from confirm() to ConfirmDialog

cb36195

simplify ConfirmDialog state using the built in isOpen prop

0fc5365

Clean up ConfirmDialog string

976ede7

Co-authored-by: Marco Ciampini <[email protected]>

implement handleDialogCancel callback to improve performance on rer…

99eaeb4

…enders

update existing e2e tests to account for new ConfirmDialog when chang…

fbda819

…ing post visibility

add test to verify canceling works correctly in the new ConfirmDialog

73c12bb

update XPath to accurately select to correct Cancel button

68e2a04

chad1008 force-pushed the migrate/post-visibility-confirm-dialog branch from 423ff74 to 68e2a04 Compare February 2, 2022 12:34

improve element selectors on new e2e tests

7ee3283

ciampo approved these changes Feb 4, 2022

View reviewed changes

ciampo merged commit 002767f into WordPress:trunk Feb 4, 2022

github-actions bot added this to the Gutenberg 12.6 milestone Feb 4, 2022

ciampo added [Package] Editor /packages/editor [Type] Enhancement A suggestion for improvement. and removed [Package] Components /packages/components labels Feb 4, 2022

chad1008 mentioned this pull request Feb 22, 2022

Confirmdialog: add custom button text #38994

Merged

8 tasks

worldomonation mentioned this pull request Feb 22, 2022

E2E: fix permanently failing tests in WPCOM/Gutenberg. Automattic/wp-calypso#61376

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate post privacy confirmation from `confirm()` to `ConfirmDialog` #37602

Migrate post privacy confirmation from `confirm()` to `ConfirmDialog` #37602

chad1008 commented Dec 22, 2021 •

edited

Loading

chad1008 commented Jan 7, 2022

ciampo left a comment

chad1008 commented Jan 11, 2022

chad1008 commented Jan 13, 2022

ciampo commented Jan 21, 2022

fullofcaffeine commented Jan 25, 2022

fullofcaffeine commented Jan 25, 2022 •

edited

Loading

fullofcaffeine commented Jan 25, 2022 •

edited

Loading

chad1008 commented Jan 25, 2022 •

edited

Loading

chad1008 Jan 26, 2022 •

edited

Loading

ciampo Jan 27, 2022

fullofcaffeine Jan 27, 2022 •

edited

Loading

ciampo Jan 27, 2022

fullofcaffeine Jan 27, 2022 •

edited

Loading

fullofcaffeine Jan 27, 2022 •

edited

Loading

ciampo Jan 28, 2022

chad1008 Jan 31, 2022

chad1008 Jan 31, 2022 •

edited

Loading

ciampo Jan 31, 2022

ciampo commented Jan 31, 2022

ciampo left a comment

chad1008 commented Feb 1, 2022

chad1008 commented Feb 2, 2022

ciampo left a comment

ciampo Feb 4, 2022 •

edited

Loading

Migrate post privacy confirmation from confirm() to ConfirmDialog #37602

Migrate post privacy confirmation from confirm() to ConfirmDialog #37602

Conversation

chad1008 commented Dec 22, 2021 • edited Loading

Description

How has this been tested?

Checklist:

chad1008 commented Jan 7, 2022

ciampo left a comment

Choose a reason for hiding this comment

chad1008 commented Jan 11, 2022

chad1008 commented Jan 13, 2022

ciampo commented Jan 21, 2022

fullofcaffeine commented Jan 25, 2022

fullofcaffeine commented Jan 25, 2022 • edited Loading

fullofcaffeine commented Jan 25, 2022 • edited Loading

chad1008 commented Jan 25, 2022 • edited Loading

chad1008 Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

ciampo Jan 27, 2022

Choose a reason for hiding this comment

fullofcaffeine Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

ciampo Jan 27, 2022

Choose a reason for hiding this comment

fullofcaffeine Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

fullofcaffeine Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

ciampo Jan 28, 2022

Choose a reason for hiding this comment

chad1008 Jan 31, 2022

Choose a reason for hiding this comment

chad1008 Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

ciampo Jan 31, 2022

Choose a reason for hiding this comment

ciampo commented Jan 31, 2022

ciampo left a comment

Choose a reason for hiding this comment

chad1008 commented Feb 1, 2022

chad1008 commented Feb 2, 2022

ciampo left a comment

Choose a reason for hiding this comment

ciampo Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Migrate post privacy confirmation from `confirm()` to `ConfirmDialog` #37602

Migrate post privacy confirmation from `confirm()` to `ConfirmDialog` #37602

chad1008 commented Dec 22, 2021 •

edited

Loading

fullofcaffeine commented Jan 25, 2022 •

edited

Loading

fullofcaffeine commented Jan 25, 2022 •

edited

Loading

chad1008 commented Jan 25, 2022 •

edited

Loading

chad1008 Jan 26, 2022 •

edited

Loading

fullofcaffeine Jan 27, 2022 •

edited

Loading

fullofcaffeine Jan 27, 2022 •

edited

Loading

fullofcaffeine Jan 27, 2022 •

edited

Loading

chad1008 Jan 31, 2022 •

edited

Loading

ciampo Feb 4, 2022 •

edited

Loading