Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance on Navigation view scripts #42394

Closed
afercia opened this issue Jul 13, 2022 · 4 comments · Fixed by #52536
Closed

Improve performance on Navigation view scripts #42394

afercia opened this issue Jul 13, 2022 · 4 comments · Fixed by #52536
Labels
[Block] Navigation Affects the Navigation Block [Block] Submenu Affects the Submenu Block - for submenus in navigation [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts

Comments

@afercia
Copy link
Contributor

afercia commented Jul 13, 2022

Description

Noticed while reviewing #41986

In the navigation and submenu view.js there's room for performance improvements. See for example the packages/block-library/src/navigation/view.js file:

  • The load event callback: load is fired when the page and all assets (e.g. images) are fully loaded. On a page with large images, that could be a bit late. Using the domReady package would be a better option.
  • The 'Close on click outside' callback selects all the navigation blocks at any click on the page again and again. ideally, querySelectorAll should run only once.
  • The 'Close on focus outside or escape key' callback does the same thing: querySelectorAll runs at any key press on the page for example at any Tab key press or any other key press, even when users type into a form field (e.g. the Comment textarea)
  • The remaining part of the keyup event callback runs at any key press. I'm not sure attaching a keyup event to the document is the best option in the first place.

Similar improvements should be considered for the submenu view.js as well.

Step-by-step reproduction instructions

  • Log something to the console at the beginning of the 'Close on click outside.' callback.
  • Build.
  • Go to the front end. Make sure you added a navigation menu.
  • Observe the console and see the click callback runs at any click on any part of the page.
  • Do the same for the 'Close on focus outside or escape key.' callback. Test by pressing any key on any part of the page.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts [Block] Navigation Affects the Navigation Block [Block] Submenu Affects the Submenu Block - for submenus in navigation labels Jul 13, 2022
@westonruter
Copy link
Member

Something else I just noticed: the scripts are being loaded in the head. This is bad as it blocks rendering of the page, and it is unwarranted since the window load event is being used anyway. So they should be loaded in the footer at least.

Additionally, it seems that even better the scripts would be printed immediately right before the block's </nav> closing tag, and instead of using the window load event or domReady it could use async so that the scripts run as soon as the script is loaded. This would ensure that the nav menu would be functional even before the page fully loads (which can happen if the page is particularly heavy). (This depends on the Script Loading Strategies work in the Performance team.)

Some considerations would be needed when there are multiple Navigation blocks on the page, however. Ideally the same scripts wouldn't be printed more than once, so in addition to the scripts running immediately (after loading asynchronously), it should also run again at domReady so as to initialize any other Navigation blocks further down on the page.

Note that this also intersects with the Interactivity API proposal, which could involve a new system for adding scripts for blocks.

@westonruter
Copy link
Member

westonruter commented Apr 26, 2023

(The File block is also printing its viewScript in the head, but it doesn't incorporate any load/domReady event listener, meaning it is currently not able to do anything since no File blocks are present in the head.)

Update: See #50113

@westonruter
Copy link
Member

westonruter commented Apr 26, 2023

Just saw there is a Trac ticket to allow a block's view scripts to load in the footer: Core-54018

@westonruter
Copy link
Member

westonruter commented Jul 18, 2023

I believe I have fixed these issues in #52536. See §Navigation Block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Block] Submenu Affects the Submenu Block - for submenus in navigation [Type] Bug An existing feature does not function as intended [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants