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

Button block: hide link popover if not in visual edit mode #30166

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 39 additions & 51 deletions packages/block-library/src/button/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
useBlockEditingMode,
} from '@wordpress/block-editor';
import { displayShortcut, isKeyboardEvent, ENTER } from '@wordpress/keycodes';
import { link, linkOff } from '@wordpress/icons';
import { link } from '@wordpress/icons';
import {
createBlock,
cloneBlock,
Expand Down Expand Up @@ -310,66 +310,54 @@ function ButtonEdit( props ) {
} }
/>
) }
{ ! isURLSet && isLinkTag && ! lockUrlControls && (
{ isLinkTag && ! lockUrlControls && (
<ToolbarButton
name="link"
icon={ link }
title={ __( 'Link' ) }
shortcut={ displayShortcut.primary( 'k' ) }
onClick={ startEditing }
/>
) }
{ isURLSet && isLinkTag && ! lockUrlControls && (
<ToolbarButton
name="link"
icon={ linkOff }
title={ __( 'Unlink' ) }
shortcut={ displayShortcut.primaryShift( 'k' ) }
onClick={ unlink }
isActive
onClick={ () => setIsEditingURL( ! isEditingURL ) }
isActive={ isURLSet }
/>
) }
</BlockControls>
{ isLinkTag &&
isSelected &&
( isEditingURL || isURLSet ) &&
! lockUrlControls && (
<Popover
placement="bottom"
onClose={ () => {
setIsEditingURL( false );
{ isLinkTag && isSelected && isEditingURL && ! lockUrlControls && (
<Popover
placement="bottom"
onClose={ () => {
setIsEditingURL( false );
richTextRef.current?.focus();
} }
anchor={ popoverAnchor }
focusOnMount={ isEditingURL ? 'firstElement' : false }
__unstableSlotName="__unstable-block-tools-after"
shift
>
<LinkControl
value={ linkValue }
onChange={ ( {
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} ) =>
setAttributes(
getUpdatedLinkAttributes( {
rel,
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} )
)
}
onRemove={ () => {
unlink();
richTextRef.current?.focus();
} }
anchor={ popoverAnchor }
focusOnMount={ isEditingURL ? 'firstElement' : false }
__unstableSlotName="__unstable-block-tools-after"
shift
>
<LinkControl
value={ linkValue }
onChange={ ( {
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} ) =>
setAttributes(
getUpdatedLinkAttributes( {
rel,
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} )
)
}
onRemove={ () => {
unlink();
richTextRef.current?.focus();
} }
forceIsEditingLink={ isEditingURL }
settings={ LINK_SETTINGS }
/>
</Popover>
) }
forceIsEditingLink={ ! isURLSet }
settings={ LINK_SETTINGS }
/>
</Popover>
) }
<InspectorControls>
<WidthPanel
selectedWidth={ width }
Expand Down
5 changes: 0 additions & 5 deletions test/e2e/specs/editor/blocks/buttons.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ test.describe( 'Buttons', () => {
await expect(
editor.canvas.locator( 'role=textbox[name="Button text"i]' )
).toBeFocused();

// The link control should still be visible when a URL is set.
await expect(
page.locator( 'role=link[name=/^example\\.com/]' )
).toBeVisible();
} );

test( 'appends http protocol to links added which are missing a protocol', async ( {
Expand Down
18 changes: 14 additions & 4 deletions test/e2e/specs/editor/various/pattern-overrides.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@
requestUtils,
editor,
context,
pageUtils,
} ) => {
const buttonName = 'Editable button';
const { id } = await requestUtils.createBlock( {
Expand All @@ -339,14 +340,15 @@
attributes: { ref: id },
} );

// Focus the button, open the link popup.
await editor.canvas
const button = editor.canvas
.getByRole( 'document', { name: 'Block: Button' } )
.getByRole( 'textbox', { name: 'Button text' } )
.focus();
.getByRole( 'textbox', { name: 'Button text' } );
// Focus the button, open the link popup.
await button.focus();
await pageUtils.pressKeys( 'Meta+k' );
await expect(
page.getByRole( 'link', { name: 'wp.org' } ).getByText( '↗' )
).toHaveAttribute( 'aria-label', '(opens in a new tab)' );

Check failure on line 351 in test/e2e/specs/editor/various/pattern-overrides.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 4

[chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings

1) [chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings Error: Timed out 5000ms waiting for expect(locator).toHaveAttribute(expected) Locator: getByRole('link', { name: 'wp.org' }).getByText('↗') Expected string: "(opens in a new tab)" Received: <element(s) not found> Call log: - expect.toHaveAttribute with timeout 5000ms - waiting for getByRole('link', { name: 'wp.org' }).getByText('↗') 349 | await expect( 350 | page.getByRole( 'link', { name: 'wp.org' } ).getByText( '↗' ) > 351 | ).toHaveAttribute( 'aria-label', '(opens in a new tab)' ); | ^ 352 | 353 | // The link popup doesn't have a role which is a bit unfortunate. 354 | // These are the buttons in the link popup. at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/pattern-overrides.spec.js:351:5

Check failure on line 351 in test/e2e/specs/editor/various/pattern-overrides.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 4

[chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings

1) [chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings Retry #1 ─────────────────────────────────────────────────────────────────────────────────────── Error: Timed out 5000ms waiting for expect(locator).toHaveAttribute(expected) Locator: getByRole('link', { name: 'wp.org' }).getByText('↗') Expected string: "(opens in a new tab)" Received: <element(s) not found> Call log: - expect.toHaveAttribute with timeout 5000ms - waiting for getByRole('link', { name: 'wp.org' }).getByText('↗') 349 | await expect( 350 | page.getByRole( 'link', { name: 'wp.org' } ).getByText( '↗' ) > 351 | ).toHaveAttribute( 'aria-label', '(opens in a new tab)' ); | ^ 352 | 353 | // The link popup doesn't have a role which is a bit unfortunate. 354 | // These are the buttons in the link popup. at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/pattern-overrides.spec.js:351:5

Check failure on line 351 in test/e2e/specs/editor/various/pattern-overrides.spec.js

View workflow job for this annotation

GitHub Actions / Playwright - 4

[chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings

1) [chromium] › editor/various/pattern-overrides.spec.js:317:2 › Pattern Overrides › handles button's link settings Retry #2 ─────────────────────────────────────────────────────────────────────────────────────── Error: Timed out 5000ms waiting for expect(locator).toHaveAttribute(expected) Locator: getByRole('link', { name: 'wp.org' }).getByText('↗') Expected string: "(opens in a new tab)" Received: <element(s) not found> Call log: - expect.toHaveAttribute with timeout 5000ms - waiting for getByRole('link', { name: 'wp.org' }).getByText('↗') 349 | await expect( 350 | page.getByRole( 'link', { name: 'wp.org' } ).getByText( '↗' ) > 351 | ).toHaveAttribute( 'aria-label', '(opens in a new tab)' ); | ^ 352 | 353 | // The link popup doesn't have a role which is a bit unfortunate. 354 | // These are the buttons in the link popup. at /home/runner/work/gutenberg/gutenberg/test/e2e/specs/editor/various/pattern-overrides.spec.js:351:5

// The link popup doesn't have a role which is a bit unfortunate.
// These are the buttons in the link popup.
Expand Down Expand Up @@ -395,6 +397,10 @@
'noreferrer noopener'
);

// Reopen the link popup.
await button.focus();
await pageUtils.pressKeys( 'Meta+k' );

// Uncheck both checkboxes.
await editLinkButton.click();
await openInNewTabCheckbox.setChecked( false );
Expand All @@ -411,6 +417,10 @@
await expect( buttonLink ).toHaveAttribute( 'target', '' );
await expect( buttonLink ).toHaveAttribute( 'rel', '' );

// Reopen the link popup.
await button.focus();
await pageUtils.pressKeys( 'Meta+k' );

// Check only the "mark as nofollow" checkbox.
await editLinkButton.click();
await markAsNoFollowCheckbox.setChecked( true );
Expand Down
Loading