WordPress.org
Get WordPress

Make WordPress Core

Opened 2 years ago

Closed 2 months ago

Last modified 2 months ago

#60111 closed defect (bug) (fixed)

Twenty Twenty-One: Dark mode toggle script loaded twice in frontend

Reported by: flixos90's profile flixos90 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 7.0 Priority: normal
Severity: normal Version: 6.3
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: Cc:

Description

I just noticed that the TT1 theme's dark-mode-toggler.js script can be loaded twice in the frontend, and the editor-specific editor-dark-mode-support.js is loaded in the frontend as well although it is only needed for the editor.

It looks like this was introduced by [56300], which changed the hook to support the editor iframe, but in the process may not have considered that now editor-specific scripts are being loaded in the frontend too.

For the frontend, the contents of the dark-mode-toggler.js script are inlined so the script should not be enqueued. And the editor-dark-mode-support.js is irrelevant for the frontend entirely.

Change History (17)

#1 follow-up: @flixos90
2 years ago

@audrasjb @poena @desrosj for visibility

I'm not entirely sure, but potentially the fix would be to simply not enqueue the assets when ! is_admin()?

This ticket was mentioned in ​PR #5795 on ​WordPress/wordpress-develop by ​@flixos90.
2 years ago #2

  • Keywords has-patch added; needs-patch removed

This PR uses the Interactivity API to replace the custom frontend JS code for the Twenty Twenty-One theme. More concretely:

  • Uses the Interactivity API module if available (currently requires the Gutenberg plugin).
    • In WordPress core, the Interactivity API is still private and cannot be used in that way.
  • Adds a store for handling state and a few other dynamic aspects (in interactivity.js module).
  • Avoids loading the regular frontend JS code (primary-navigation.js, responsive-embeds.js, dark-mode-toggler.js) as it replaces those functionalities.

This is an experimental PR, primarily as a proof of concept and to get a better idea of how the API works, and what _should_ or _should not_ be done with it.

To test it, you need to have the Gutenberg plugin activated. You should then see a script[type="module"] pointing to the new Twenty Twenty-One interactivity.js file in the <head>.

A few observations:

  • 207 lines of custom JS (with Interactivity API) vs 314 lines of custom JS (manual approach)
  • 2.1 kB custom JS (with Interactivity API) vs 4.5 kB custom JS (manual approach)
  • 27.2 kB overall JS (with Interactivity API) vs 16.9 kB overall JS (manual approach)
    • Both numbers include 12.4 kB from import map polyfill.
    • While in this example the Interactivity API itself leads to a net increase in JS, it has to be considered that the benefits will become better the more code uses the API. For instance, a site with just a default WordPress theme is hardly realistic, and the amount of JS typically increases heavily the more plugins are used. With several sites loading 100s of kilobytes of JS, the size of the Interactivity API itself would play a minor role compared to the reduction of custom JS code gained from it (see the amount of custom JS as an indicator for that).

Unrelated: While working on this, I discovered and reported https://core.trac.wordpress.org/ticket/60111 which I think is a bug.

Trac ticket:

#3 @flixos90
2 years ago

  • Keywords needs-patch added; has-patch removed

:/ I just mentioned this ticket in ​https://github.com/WordPress/wordpress-develop/pull/5795, but not in the place to indicate it's a PR for that ticket. Anyway, I guess now it's connected to this one by accident.

​@luisherranz commented on ​PR #5795:
2 years ago #4

The only modification I would do is to use directives for the submenus to make them declarative, instead of manipulating the DOM manually: ​762b3b

By the way, making this part declarative brings the Interactivity API JS lines down to 189, vs the 314 of the vanilla JS approach, a reduction of 40% in the amount of code that the developer needs to write.

#5 @zlvkrxjv47
2 years ago

It appears that there is an issue with the Twenty Twenty-One theme where the dark mode toggle script (dark-mode-toggler.js) is being loaded twice in the frontend, and the editor-specific script (editor-dark-mode-support.js) is also being loaded unnecessarily.

This issue seems to have been introduced by changes made in revision [56300], which modified the hook to support the editor iframe. However, this change may not have considered the unintended consequence of loading editor-specific scripts in the frontend as well.

To address this issue:

  1. Remove Redundant Loading: Since the contents of dark-mode-toggler.js are already inlined for the frontend, there is no need to enqueue it. Remove any enqueueing of dark-mode-toggler.js for the frontend.
  1. Exclude Editor-Specific Scripts: Ensure that editor-dark-mode-support.js is not loaded in the frontend at all, as it is only relevant for the editor. This can be achieved by conditionally enqueueing the script only when needed, such as within the editor context.
  1. Review Changes in [56300]: Take a closer look at the changes made in revision [56300] to understand how the hook was modified to support the editor iframe. Determine if there was an oversight regarding the loading of editor-specific scripts in the frontend.
  1. Test the Changes: After making the necessary adjustments, thoroughly test the frontend to confirm that the dark mode toggle functionality still works as expected and that the editor-specific script is no longer loaded unnecessarily.

By addressing these points, you can resolve the issue of the dark mode toggle script being loaded twice in the frontend and prevent the loading of editor-specific scripts where they are not needed, thereby improving the efficiency and performance of the Twenty Twenty-One theme.

This ticket was mentioned in ​PR #9083 on ​WordPress/wordpress-develop by ​@gautam23.
9 months ago #6

  • Keywords has-patch added; needs-patch removed

There were a couple of quirks found in the Trac ticket about assets enqueuing in the Twenty Twenty-One theme. This PR fixes those issues by:

  1. Loading the editor-dark-mode-support.js script only if is_admin() is true since that script is not needed on the frontend.
  2. Removing the_script() function because it was enqueuing the dark-mode-toggler.js again via an include statement when it is already being enqueued in editor_custom_color_variables()

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

#7 @gautam23
9 months ago

  • Keywords needs-testing added

#8 @huzaifaalmesbah
4 months ago

  • Keywords needs-refresh added

I tested the last patch from PR ​https://github.com/WordPress/wordpress-develop/pull/9083 on WordPress 6.9-beta3-61129-src. Recently, I noticed changes to the the_script() function in PR ​https://github.com/WordPress/wordpress-develop/pull/9416, so I believe this ticket needs an updated patch.

This ticket was mentioned in ​PR #10714 on ​WordPress/wordpress-develop by ​@sabernhardt.
2 months ago #9

  • Keywords needs-refresh removed

Checks whether is_admin() before adding scripts and styles on 'enqueue_block_assets' action in both Twenty_Twenty_One_Custom_Colors and Twenty_Twenty_One_Dark_Mode.

Trac 60111

#10 in reply to: ↑ 1 @sabernhardt
2 months ago

  • Milestone changed from Awaiting Review to 7.0
  • Version changed from 6.4 to 6.3

potentially the fix would be to simply not enqueue the assets when ! is_admin()?

[56783] used is_admin() in Twenty Twenty. To follow that example more closely, I could update PR 10714 to include the version condition and 'enqueue_block_editor_assets' fallback for older WordPress, but I did not notice any problems with adding Twenty Twenty-One's resources on 'enqueue_block_assets' with WordPress 6.2 or 5.6.

Last edited 2 months ago by sabernhardt (previous) (diff)

​@mukesh27 commented on ​PR #10714:
2 months ago #11

@sabernhardt Does this PR fully resolve the issue for the ticket, or is there anything still pending? I see there are two other PRs are those still needed, or can we close them if they’re no longer required?

​@sabernhardt commented on ​PR #10714:
2 months ago #12

@mukeshpanchal27 This PR should resolve the frontend issues for ticket 60111, though it might be worth adding the enqueue_block_editor_assets fallback used in Twenty Twenty.

As for the other two PRs:

  • 5795 was intended for a different ticket.
  • ​9083 addressed the dark mode scripts on the frontend, taking a very different direction.

#13 @r1k0
2 months ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/10714

Environment

  • WordPress: 7.0-alpha-20260111.052907
  • PHP: 7.4.33
  • Server: PHP.wasm
  • Database: WP_SQLite_Driver (Server: 8.0.38 / Client: 3.51.0)
  • Browser: Chrome 143.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-One 2.7
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.1

Actual Results

  1. ✅ Issue resolved with patch.
  2. The dark-mode-toggler.js file is no longer enqueued.
  3. The editor-dark-mode-support.js script no longer loads on the frontend.
  4. Only the inline script of the dark-mode-toggler is loaded.

Additional Notes

  • None

Supplemental Artifacts

Before:

https://i.ibb.co/x8fncs0V/dark-mode-js-before.png

After:

https://i.ibb.co/Lhk8Lq3x/dark-mode-js-after.png

#14 @SergeyBiryukov
2 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 61471:

Twenty Twenty-One: Only enqueue block editor assets in the admin.

Follow-up to [56300].

Props sabernhardt, flixos90, luisherranz, gautam23, huzaifaalmesbah, mukesh27, r1k0, SergeyBiryukov.
Fixes #60111.

​@westonruter commented on ​PR #5795:
2 months ago #15

This was erroneously closed by the bot.

​@westonruter commented on ​PR #5795:
2 months ago #16

Or maybe it was expectedly closed, but the changes proposed here are not what were committed.

​@SergeyBiryukov commented on ​PR #10714:
2 months ago #17

Thanks for the PR! Merged in r61471.

Note: See TracTickets for help on using tickets.