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

Fix Block Settings sidebar unexpectedly collapsing #34543

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

@mreishus
Copy link
Contributor

@mreishus mreishus commented Sep 3, 2021

Description

  • The new block widgets feature allows Gutenberg to be loaded inside the Customizer sidebar. Inside this sidebar, you are able to visit the "Block Settings" area. In certain circumstances. which I will explain in more detail later, the "Block Settings" area would unexpectedly collapse and send the user back to the block view, requiring the user to do a couple of clicks to move back to the area they were working in and did not intend to leave. This PR solves this problem.
  • Here is a video of the original issue happening:
Peek.2021-09-02.13-49-wporg-settings-move.mp4

In this video, I visit the widgets area, pick a block, visit the Block Settings area, make a change and publish. So far, all is well. Note the publish button is now disabled. Now I visit the Block Settings area and make another change. I intend to stay in the area, but I am sent back to the paragraph block. This can be disrupting if you are intending to make several changes in the Block Settings area but you're unexpectedly sent out of the area.

Now we can discuss the exact circumstances that cause the bug to happen. In this video, when I make a change the second time and I'm sent back, the publish button is also moving from disabled to enabled. Being sent back is a side effect of this change.

How often the bug happens depends on the active theme's capabilities. There are two cases:

  • Theme supports customize-selective-refresh-widgets: The bug only happens when the publish button moves from disabled to enabled. Thankfully, most modern themes, including the defaults like twentynineteen, twentytwenty, and twentytwentyone have this setting.
  • Theme does not support customize-selective-refresh-widgets: The bug happens whenever making a change in the block settings area, making it more disruptive. These are older themes and some custom themes.

Now, to discuss the mechanism and why it happens. The root cause is the customizer running reflowPaneContents, which means it takes some time to figure out what should and shouldn't be visible.

At one point, all panels and sections run through this little bit of code, which figures out if they're active, and then calls onChangeActive accordingly. What's happening in this case is that it incorrectly determines the Block Settings section is not active, and calls .onChangeActive( active=false, args ), even though that section is up front and I am interacting with the controls in that section. This ends up calling this .collapse() call, which moves me out of the section. And this block is entered because active is false (but it should be true).

So why is active false? Let's go back to the block that is figuring out the active value. For the Block Settings Sidebar, it is actually considering two active values and needs them to both be on: see this line of code. The first active, is coming from a function argument, and in the case of the Block Settings section which I am interacting with, is true and is totally fine. What's happening is the second check, the container.isContextuallyActive() is always returning false, which in turn sets active to be false, and causes the section to collapse.

What is isContextuallyActive() doing? Through the magic of inheritance, even though the Block Settings Sidebar is defined in gutenberg, its definition for isContextuallyActive() comes from the base class in the customizer: default isContextuallyActive(). Here, it is calling .controls() to look through all of the controls belonging to the section, checking if at least one is active. The problem is this is a customizer specific abstraction. Even though Gutenberg's Block Settings section has a number of controls, none of them are returned by .controls(). It's just not a part of the integration that fits cleanly.

So, my solution is to override isContextuallyActive() for the sections that are defined in Gutenberg, and have them only return if the section itself is active, while ignoring the .controls() check. Note that this is not without precendent, as themes in the customizer had to do the same thing. This completely fixes the problem and stops me from unexpectedly being sent away from the block settings. It works in both cases of the theme supporting and not supporting customize-selective-refresh-widgets as mentioned above.

How has this been tested?

  • Cloned https://github.com/WordPress/gutenberg
  • Ensure that .wp-env.json has "." listed in plugins
  • Cd into it and wp-env destroy && wp-env start to create a clean WP
  • Visit http://localhost:8888/wp-admin/ and login with admin:password
  • Appearance -> Customize in sidebar menu
  • Widgets in sidebar menu
  • Plus sign button to create a new block, select paragraph block, type "hello" into it
  • Click on the block you just added to see the floating gutenberg bar
  • Click the three dots menu then select "Show More Settings"
  • Change the text color
  • Click 'publish'
  • Click on the block you just added to see the floating gutenberg bar
  • Click the three dots menu then select "Show More Settings"
  • Change the text color
  • Expect to see: Remain in the "Block Settings" area
  • Actually see: You are taken out of the block settings area 1

To test this branch, check it out, npm run dev, and redo the steps.
To add debugging code in the customizer, docker ps | grep wordpress | grep 8888 | cut -d' ' -f1 to get the container id, then docker exec -it container_id /bin/bash. Inside the container, apt update && apt install vim, then vim /www/html/wp-admin/js/customize-controls.js.

Screenshots

See Video

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
@mreishus mreishus requested a review from noisysocks as a code owner Sep 3, 2021
@mreishus
Copy link
Contributor Author

@mreishus mreishus commented Sep 3, 2021

Checking the git history, @kevin940726 worked in this area most recently and might be familiar with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

1 participant