Customizer Restore Autosave Notice dismiss function is not called

[karthikbhatb]

After 5.5.1 upgrade dismissAutosave function is not being called on notice dismissal for recent Autosave Notice (Under customize-control.js).
An extra .notice-dismiss button is added under common.js

Fix: Check if the .notice-dismiss already exists under the notice element.

After 5.5.1 upgrade dismissAutosave function is not being called on notice dismissal for recent Autosave Notice (Under customize-control.js).
An extra .notice-dismiss button is added under common.js

Fix: Check if the .notice-dismiss already exists under the notice element.

Trac ticket: https://core.trac.wordpress.org/ticket/51425

[SergeyBiryukov]

Hi there, welcome to WordPress Trac! Thanks for the ticket and the patch.

It would be great to have the steps to reproduce the issue on a clean install, and to also track down which change caused the regression.

[karthikbhatb]

Hi,

Thanks for tidying up the ticket.

Steps to reproduce the issue:

1. Install fresh WordPress 5.5.1
2. Navigate to wp-admin/customize.php
3. Make any change/s (do not hit publish / do not save)
4. Reload the page (confirm reload by clicking the reload alert button)
   • This gives the following notice: There is a more recent autosave of your changes than the one you are previewing. Restore the autosave
5. Try to dismiss the notice. Notice disappears, but notice area doesn't collapse
6. Reload the page. The notice appears once again, expected behavior would be to not see the notice again.

I could not track down what changes caused the issue (tried with trac. any help/link on how to do this for the future is appreciated.)

Anyway, what I found is that the notice dismissal and admin ajax call on .notice-dismiss is already handled in https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/wp/customize/base.js#L912 and
https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/wp/customize/controls.js#L8360

It would not make sense for makeNoticesDismissible in https://core.trac.wordpress.org/browser/trunk/src/js/_enqueues/admin/common.js#L1092 to override that.

Let me know if there is more to it.

[dlh]

Confirmed that this was working as expected in 5.4; assigning the version accordingly. I'm not sure yet where it broke, though.

[noisysocks]

Welcome @karthikbhatb! Thanks for the bug report and PR.

I ran git bisect with 5.4.0 as the last known good commit and found that r48925 (#51123) is what caused this regression.

Adding common as a dependency of updates has the unintended side effect of loading common in customize.php. This breaks notices because now both customize-controls and common are initialising notices.

The attached PR does indeed fix this error by making common return early if notices have already been initialised, but I think it would be better if we fixed the underlying issue here and made it so that notices are only initialised once.

I'm not sure why r48925 added common as a dependency of updates. Maybe it's unnecessary? Seeking a second opinion here.

[SergeyBiryukov]

Replying to noisysocks:

I'm not sure why r48925 added common as a dependency of updates. Maybe it's unnecessary? Seeking a second opinion here.

It was to make the deprecateL10nObject() function available, since it's now used in that file​.

[SergeyBiryukov]

Replying to noisysocks:

The attached PR does indeed fix this error by making common return early if notices have already been initialised, but I think it would be better if we fixed the underlying issue here and made it so that notices are only initialised once.

I agree that would be the preferred fix here.

It looks like the patch might need some adjustment, but the common dependency is required, per comment:7.

[noisysocks]

Thinking about this some more, I think that it's wrong to enqueue common in customize.php.

common contains JavaScript that makes common WP Admin elements interactive: table columns, form validation, notices, screen meta, help tabs, menu interactivity, etc. But Customize is a very unique screen that contains essentially none of these elements.

I suspect there are other undiscovered bugs similar to this one that come as a result of common trying to add functionality to the Customize screen that isn't relevant.

At the very least, by enqueuing common here, we're making the browser parse and execute ~2000 lines of unnecessary JavaScript which will negatively affect Customize page load performance.

deprecateL10nObject should be moved elsewhere so that updates can use it without having to depend on all of common.

By moving deprecateL10nObject() to its own script, we're able to remove common as a dependency of updates. This means that we're not unnecessarilly executing common in customize.php which both fixes 51425 and means that there are ~2000 less lines of JavaScript executed in customize.php.

Trac ticket: https://core.trac.wordpress.org/ticket/51425

By moving deprecateL10nObject() to its own script, we're able to remove common as a dependency of updates. This means that we're not unnecessarilly executing common in customize.php which both fixes 51425 and means that there are ~2000 less lines of JavaScript executed in customize.php.

Trac ticket: https://core.trac.wordpress.org/ticket/51425

[noisysocks]

I wasn't able to find any dependency that dashboard, theme-editor-plugin, updates, widgets and common all have in common so instead I pulled deprecateL10nObject() out into its own script. That then lets us remove common as a dependency of updates which fixes this bug.

Please test ​PR #690 thoroughly, in particular for regressions to #51123.

Is it worth considering putting this in @wordpress/deprecated?

Note that this was only a temporary solution which will be removed in 5.7 so I don't think we should move it into its own file.

Is it worth considering putting this in @wordpress/deprecated?

It's a little cumbersome to do this because @wordpress/deprecated is a package in the Gutenberg repo. I'm not sure that it's worth the effort if this is a temporary function like @ocean90 says.

Note that this was only a temporary solution which will be removed in 5.7 so I don't think we should move it into its own file.

Aha, I see. So deprecateL10nObject will be completely removed? Do you think then it's best to hold off on fixing 51425 until 5.7?

[SergeyBiryukov]

Related: #51200, #51317.

[noisysocks]

I think probably the most sensible path forward here is to commit ​https://github.com/WordPress/wordpress-develop/pull/560 as a temporary fix for 5.6. In 5.7, when deprecateL10nObject is removed, we can remove the temporary fix which will no longer be needed.

[noisysocks]

In 49625:

Customize: Temporary fix for autosave restore notice not being removed

Fixes the "There is a more recent autosave of your changes" notice from not
being removed when the dismiss button is clicked.

The problem is caused by the notice being initialized twice: once by the
common script and then again by the customize-controls script.

This temporary fix prevents customize-controls from initializing a notice if
it has already been initialized.

A better fix would be to not initialize notices twice. This can be done by
removing common as a dependency of updates when deprecateL10nObject is
removed.

When this happens (est: 5.7), this temporary fix should be reverted.

Fixes #51425.
See #51317.
Props karthikbhatb, dlh, SergeyBiryukov.

Resolved in changeset https://core.trac.wordpress.org/changeset/49625

Resolved in changeset https://core.trac.wordpress.org/changeset/49625