Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51425 closed defect (bug) (fixed)

Customizer Restore Autosave Notice dismiss function is not called

Reported by: karthikbhatb's profile karthikbhatb Owned by: noisysocks's profile noisysocks
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.5
Component: Customize Keywords: has-patch needs-testing
Focuses: ui, javascript Cc:

Description

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.

Change History (24)

This ticket was mentioned in PR #560 on WordPress/wordpress-develop by karthikax.


4 years ago
#1

  • Keywords has-patch added

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

#2 @SergeyBiryukov
4 years ago

  • Component changed from General to Customize
  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 5.6

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.

#3 @karthikbhatb
4 years ago

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.

#4 @dlh
4 years ago

  • Version set to 5.5

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

#5 @noisysocks
4 years ago

  • Owner set to noisysocks
  • Status changed from new to reviewing

#6 follow-ups: @noisysocks
4 years ago

  • Keywords 2nd-opinion added; needs-testing removed

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.

#7 in reply to: ↑ 6 @SergeyBiryukov
4 years ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#9 in reply to: ↑ 6 @SergeyBiryukov
4 years ago

  • Keywords 2nd-opinion removed

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.

#10 @noisysocks
4 years ago

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.

This ticket was mentioned in PR #690 on WordPress/wordpress-develop by noisysocks.


4 years ago
#11

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

This ticket was mentioned in PR #690 on WordPress/wordpress-develop by noisysocks.


4 years ago
#12

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

#13 @noisysocks
4 years ago

  • Keywords needs-testing added

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.

TimothyBJacobs commented on PR #690:


4 years ago
#14

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

ocean90 commented on PR #690:


4 years ago
#15

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.

noisysocks commented on PR #690:


4 years ago
#16

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?

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#18 @SergeyBiryukov
4 years ago

Related: #51200, #51317.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#19 @noisysocks
4 years ago

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.

This ticket was mentioned in Slack in #core by noisysocks. View the logs.


4 years ago

#21 @noisysocks
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.