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.

Sign up for GitHub

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

Jump to bottom

Fix the navigation issue inside cover blocks #66093

Merged
merged 8 commits into from
Oct 17, 2024
Merged

Fix the navigation issue inside cover blocks #66093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb4dc79
Implement a simple solution for the navigation open inside the cover
renatho Oct 11, 2024
64c14d3
Add the has-modal-open to the editor reproducing the same behavior of…
renatho Oct 11, 2024
b6f62e2
Add test
renatho Oct 14, 2024
10e1007
Remove not necessary comment
renatho Oct 15, 2024
0a244ec
Update check to see if second cover inner blocks container is clickable
renatho Oct 15, 2024
0850938
Add useEffect cleanup to remove modal class
renatho Oct 16, 2024
efe0a9e
Refactor html element selector
renatho Oct 16, 2024
eaabc88
Make sure element exists before trying to remove the class
renatho Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/block-library/src/cover/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@
color: inherit;
// Reset the fixed LTR direction at the root of the block in RTL languages.
/*rtl:raw: direction: rtl; */

// Reset the z-index to auto when the body has a modal open. So when the
// modal is inside the cover, it doesn't create a stacking context.
@at-root .has-modal-open & {
z-index: auto;
}
}

// Position: Top
Expand Down
33 changes: 30 additions & 3 deletions packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,33 @@ import AccessibleDescription from './accessible-description';
import AccessibleMenuDescription from './accessible-menu-description';
import { unlock } from '../../lock-unlock';

function useResponsiveMenu( navRef ) {
const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] =
useState( false );

useEffect( () => {
if ( ! navRef.current ) {
return;
}

const htmlElement = navRef.current.ownerDocument.documentElement;

// Add a `has-modal-open` class to the <html> when the responsive
// menu is open. This reproduces the same behavior of the frontend.
Comment on lines +87 to +88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit odd for a block to be manipulating the root node but if this brings it inline with the frontend perhaps it's already been decided this is ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does feel unusual. Is there any precedent?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

Screen.Capture.on.2024-10-15.at.13-07-37.mp4

Copy link
Contributor Author

@renatho renatho Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's really needed because also happens in the editor.

Screenshot 2024-10-15 at 09 51 06

Since it's the behavior in the frontend, I think it would be fine to have the same behavior replicated in the editor.

I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

I noticed sometimes some kind of cache. I didn't dig into to figure out, but sometimes when it delayed I restarted the build and restarted the browser, also marking the "Disable cache" in my network tab.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there specific steps to replicate that state? I tried following the PR instructions but it didn't manifest. Maybe it was prior to the rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised this PR. I'd appreciate if someone could review it to improve the solution.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors. If someone could explain me what is happening and how I could fix, it would also be very helpful to save some time. =)

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked I was wrong about the deprecation and it's transparent for the user

Yep, the only real sign within the editor should be a console message stating the block was migrated.

And the solution of the has-modal-open will be only for backward compatibility

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

I'll comment to that effect when I get to review the alternate PR.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors.

Without looking I can't say if this is the reason or not but another misconception about deprecations is that they are chained together to migrate one deprecated version to the next, all the way to the latest version.

Instead, all deprecations need to be updated (if required) to migrate that deprecated version to the latest. We can continue the discussion over on #66249 as needed.

Thanks for spinning it up 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

It's good to have that anyway, so it's fixed everywhere not needing a manual save in all the broken posts/templates. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the original issue only in the editor? If that's the case, the deprecation will migrate and fix that display without needing further styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connecting the points, I sent an answer here: #66249 (comment)

if ( isResponsiveMenuOpen ) {
htmlElement.classList.add( 'has-modal-open' );
} else {
htmlElement.classList.remove( 'has-modal-open' );
}

return () => {
htmlElement?.classList.remove( 'has-modal-open' );
};
}, [ navRef, isResponsiveMenuOpen ] );

return [ isResponsiveMenuOpen, setResponsiveMenuVisibility ];
}

function ColorTools( {
textColor,
setTextColor,
Expand Down Expand Up @@ -284,8 +311,10 @@ function Navigation( {
__unstableMarkNextChangeAsNotPersistent,
} = useDispatch( blockEditorStore );

const navRef = useRef();

const [ isResponsiveMenuOpen, setResponsiveMenuVisibility ] =
useState( false );
useResponsiveMenu( navRef );

const [ overlayMenuPreview, setOverlayMenuPreview ] = useState( false );

Expand Down Expand Up @@ -367,8 +396,6 @@ function Navigation( {
__unstableMarkNextChangeAsNotPersistent,
] );

const navRef = useRef();

// The standard HTML5 tag for the block wrapper.
const TagName = 'nav';

Expand Down
71 changes: 71 additions & 0 deletions test/e2e/specs/editor/blocks/cover.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,77 @@ test.describe( 'Cover', () => {
await expect( overlay ).toHaveCSS( 'background-color', 'rgb(0, 0, 0)' );
await expect( overlay ).toHaveCSS( 'opacity', '0.5' );
} );

test( 'other cover blocks are not over the navigation block when the menu is open', async ( {
editor,
page,
} ) => {
// Insert a Cover block
await editor.insertBlock( { name: 'core/cover' } );
const coverBlock = editor.canvas.getByRole( 'document', {
name: 'Block: Cover',
} );

// Choose a color swatch to transform the placeholder block into
// a functioning block.
await coverBlock
.getByRole( 'option', {
name: 'Color: Black',
} )
.click();

// Insert a Navigation block inside the Cover block
await editor.selectBlocks( coverBlock );
await coverBlock.getByRole( 'button', { name: 'Add block' } ).click();
await page.keyboard.type( 'Navigation' );
const blockResults = page.getByRole( 'listbox', {
name: 'Blocks',
} );
const blockResultOptions = blockResults.getByRole( 'option' );
await blockResultOptions.nth( 0 ).click();

// Insert a second Cover block.
await editor.insertBlock( { name: 'core/cover' } );
const secondCoverBlock = editor.canvas
.getByRole( 'document', {
name: 'Block: Cover',
} )
.last();

// Choose a color swatch to transform the placeholder block into
// a functioning block.
await secondCoverBlock
.getByRole( 'option', {
name: 'Color: Black',
} )
.click();

// Set the viewport to a small screen and open menu.
await page.setViewportSize( { width: 375, height: 1000 } );
const navigationBlock = editor.canvas.getByRole( 'document', {
name: 'Block: Navigation',
} );
await editor.selectBlocks( navigationBlock );
await editor.canvas
.getByRole( 'button', { name: 'Open menu' } )
.click();

// Check if inner container of the second cover is clickable.
const secondInnerContainer = secondCoverBlock.locator(
'.wp-block-cover__inner-container'
);
let isClickable;
try {
isClickable = await secondInnerContainer.click( {
trial: true,
timeout: 1000, // This test will always take 1 second to run.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this timing simply arbitrary or was there some metric driving it?

@kevin940726 in your testing wisdom do you have any better ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't metric-driven. I just used a value that I considered "acceptable". I'm looking forward to seeing if @kevin940726 has ideas to improve this. :)

} );
} catch ( error ) {
isClickable = false;
}

expect( isClickable ).toBe( false );
} );
} );

class CoverBlockUtils {
Expand Down
Loading