-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 link popover keyboard accessibility #10983
Changes from all commits
017601d
0bdbc2b
45c09f5
fd432f0
ec5bfb3
861c00c
6c0a2b7
97dfc04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -372,6 +372,7 @@ export class BlockListBlock extends Component { | |
isPartOfMultiSelection, | ||
isFirstMultiSelected, | ||
isTypingWithinBlock, | ||
isCaretWithinFormattedText, | ||
isMultiSelecting, | ||
hoverArea, | ||
isEmptyDefaultBlock, | ||
|
@@ -399,7 +400,7 @@ export class BlockListBlock extends Component { | |
// We render block movers and block settings to keep them tabbale even if hidden | ||
const shouldRenderMovers = ! isFocusMode && ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock; | ||
const shouldShowBreadcrumb = ! isFocusMode && isHovered && ! isEmptyDefaultBlock; | ||
const shouldShowContextualToolbar = ! hasFixedToolbar && ! showSideInserter && ( ( isSelected && ! isTypingWithinBlock ) || isFirstMultiSelected ); | ||
const shouldShowContextualToolbar = ! hasFixedToolbar && ! showSideInserter && ( ( isSelected && ( ! isTypingWithinBlock || isCaretWithinFormattedText ) ) || isFirstMultiSelected ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These bools scare me - might be worth refactoring them into functions or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely yes, I recall them being quite scary the last time I worked on this file. Might be worth a separate code quality issue as a follow-up, unless you have the time to do it now 😄 |
||
const shouldShowMobileToolbar = shouldAppearSelected; | ||
const { error, dragging } = this.state; | ||
|
||
|
@@ -588,6 +589,7 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV | |
isFirstMultiSelectedBlock, | ||
isMultiSelecting, | ||
isTyping, | ||
isCaretWithinFormattedText, | ||
getBlockIndex, | ||
getEditedPostAttribute, | ||
getBlockMode, | ||
|
@@ -613,6 +615,7 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV | |
// We only care about this prop when the block is selected | ||
// Thus to avoid unnecessary rerenders we avoid updating the prop if the block is not selected. | ||
isTypingWithinBlock: ( isSelected || isParentOfSelectedBlock ) && isTyping(), | ||
isCaretWithinFormattedText: isCaretWithinFormattedText(), | ||
order: getBlockIndex( clientId, rootClientId ), | ||
meta: getEditedPostAttribute( 'meta' ), | ||
mode: getBlockMode( clientId ), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -424,7 +424,13 @@ export class RichText extends Component { | |
return; | ||
} | ||
|
||
const { start, end } = this.createRecord(); | ||
const { start, end, formats } = this.createRecord(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like onChange only triggers when typing instead of when using cursor keys or changing selection with the mouse like I need. The createRecord was there before, should it be getRecord instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, never mind then, I only saw the green line. :) Then this looks good, sorry! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries - thanks for taking a look. 😄 |
||
|
||
if ( formats[ start ] ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be moved in |
||
this.props.onEnterFormattedText(); | ||
} else { | ||
this.props.onExitFormattedText(); | ||
} | ||
|
||
if ( start !== this.state.start || end !== this.state.end ) { | ||
this.setState( { start, end } ); | ||
|
@@ -962,12 +968,16 @@ const RichTextContainer = compose( [ | |
createUndoLevel, | ||
redo, | ||
undo, | ||
enterFormattedText, | ||
exitFormattedText, | ||
} = dispatch( 'core/editor' ); | ||
|
||
return { | ||
onCreateUndoLevel: createUndoLevel, | ||
onRedo: redo, | ||
onUndo: undo, | ||
onEnterFormattedText: enterFormattedText, | ||
onExitFormattedText: exitFormattedText, | ||
}; | ||
} ), | ||
withSafeTimeout, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -388,4 +388,36 @@ describe( 'Links', () => { | |
await page.keyboard.press( 'Escape' ); | ||
expect( await page.$( '.editor-url-popover' ) ).toBeNull(); | ||
} ); | ||
|
||
it( 'can be modified using the keyboard once a link has been set', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ for the test! |
||
const URL = 'https://wordpress.org/gutenberg'; | ||
|
||
// Create a block with some text and format it as a link. | ||
await clickBlockAppender(); | ||
await page.keyboard.type( 'This is Gutenberg' ); | ||
await pressWithModifier( SELECT_WORD_MODIFIER_KEYS, 'ArrowLeft' ); | ||
await pressWithModifier( META_KEY, 'K' ); | ||
await waitForAutoFocus(); | ||
await page.keyboard.type( URL ); | ||
await page.keyboard.press( 'Enter' ); | ||
|
||
// Deselect the link text by moving the caret to the end of the line | ||
// and the link popover should not be displayed. | ||
await page.keyboard.press( 'End' ); | ||
expect( await page.$( '.editor-url-popover' ) ).toBeNull(); | ||
|
||
// Move the caret back into the link text and the link popover | ||
// should be displayed. | ||
await page.keyboard.press( 'ArrowLeft' ); | ||
expect( await page.$( '.editor-url-popover' ) ).not.toBeNull(); | ||
|
||
// Press Cmd+K to edit the link and the url-input should become | ||
// focused with the value previously inserted. | ||
await pressWithModifier( META_KEY, 'K' ); | ||
await waitForAutoFocus(); | ||
const activeElementParentClasses = await page.evaluate( () => Object.values( document.activeElement.parentElement.classList ) ); | ||
expect( activeElementParentClasses ).toContain( 'editor-url-input' ); | ||
const activeElementValue = await page.evaluate( () => document.activeElement.value ); | ||
expect( activeElementValue ).toBe( URL ); | ||
} ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that the block's wrapper is aware of something very specific to the implementation of a given block (RichText). It doesn't seem like a generic enough solution for me. All blocks don't use RichText. As I see it the block wrapper should work similarily no matter what's the content of the
edit
function.That said, I'm not familiar enough with the issue and the PR to propose an alternative. (which means I won't block this PR either)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I don't have a better solution either off the top of my head, aside from just documenting that it's
RichText
specific, which still isn't great/is the whole problem. 🤷♂️