#51425 closed defect (bug) (fixed)
Customizer Restore Autosave Notice dismiss function is not called
Reported by: |
|
Owned by: |
|
---|---|---|---|
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
#2
@
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
@
4 years ago
Hi,
Thanks for tidying up the ticket.
Steps to reproduce the issue:
- Install fresh WordPress 5.5.1
- Navigate to
wp-admin/customize.php
- Make any change/s (do not hit publish / do not save)
- 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
- This gives the following notice:
- Try to dismiss the notice. Notice disappears, but notice area doesn't collapse
- 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
@
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.
#6
follow-ups:
↓ 7
↓ 9
@
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
@
4 years ago
Replying to noisysocks:
I'm not sure why r48925 added
common
as a dependency ofupdates
. 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
@
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
@
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
@
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
?
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
#19
@
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
hellofromtonya commented on PR #560:
4 years ago
#22
Resolved in changeset https://core.trac.wordpress.org/changeset/49625
hellofromtonya commented on PR #690:
4 years ago
#23
Resolved in changeset https://core.trac.wordpress.org/changeset/49625
After 5.5.1 upgrade
dismissAutosave
function is not being called on notice dismissal for recent Autosave Notice (Undercustomize-control.js
).An extra
.notice-dismiss
button is added undercommon.js
Fix: Check if the
.notice-dismiss
already exists under the notice element.Trac ticket: https://core.trac.wordpress.org/ticket/51425