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

Default appender: try editable paragraph instead of text area #30986

Merged
merged 8 commits into from
Apr 23, 2021
Merged

Default appender: try editable paragraph instead of text area #30986

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
3955b1c
Default appender: try editable paragraph instead of text area
ellatrix Apr 20, 2021
8695803
Move wp-block class
ellatrix Apr 20, 2021
9d8e777
Port over CSS from 30656 that's still useful.
jasmussen Apr 20, 2021
bf37fa3
Merge branch 'try/content-editable-default-appender' of github.com:Wo…
jasmussen Apr 20, 2021
02a5da3
Update snapshots.
jasmussen Apr 20, 2021
36e8678
Remove content editable in favour of tab index
ellatrix Apr 20, 2021
e416513
Fix tests
ellatrix Apr 20, 2021
4f7fe2a
Fix e2e test
ellatrix Apr 20, 2021
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
49 changes: 23 additions & 26 deletions packages/block-editor/src/components/default-block-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import TextareaAutosize from 'react-autosize-textarea';

/**
* WordPress dependencies
*/
Expand All @@ -18,6 +13,12 @@ import { withSelect, withDispatch } from '@wordpress/data';
import Inserter from '../inserter';
import { store as blockEditorStore } from '../../store';

/**
* Zero width non-breaking space, used as padding for the paragraph when it is
* empty.
*/
export const ZWNBSP = '\ufeff';
Copy link
Contributor

Choose a reason for hiding this comment

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

For posterity, what benefits do we see from using a non-breaking space vs an empty string?

I 🔍 a little bit and ZWNBSP is also better known for its use as a byte order mark. We might also want to consider using word joiner \u2060 instead if we do need something other than an empty string for padding.

From: https://www.unicode.org/faq/utf_bom.html#bom6

Q What should I do with U+FEFF in the middle of a file?
A: In the absence of a protocol supporting its use as a BOM and when not at the beginning of a text stream, U+FEFF should normally not occur. For backwards compatibility it should be treated as ZERO WIDTH NON-BREAKING SPACE (ZWNBSP), and is then part of the content of the file or string. The use of U+2060 WORD JOINER is strongly preferred over ZWNBSP for expressing word joining semantics since it cannot be confused with a BOM. When designing a markup language or data protocol, the use of U+FEFF can be restricted to that of Byte Order Mark. In that case, any U+FEFF occurring in the middle of a file can be treated as an unsupported character. [AF]

Copy link
Member Author

Choose a reason for hiding this comment

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

An empty string won't fill the element. We use the same character for rich text padding.


export function DefaultBlockAppender( {
isLocked,
isVisible,
Expand All @@ -33,34 +34,30 @@ export function DefaultBlockAppender( {
const value =
decodeEntities( placeholder ) || __( 'Type / to choose a block' );

// The appender "button" is in-fact a text field so as to support
// transitions by WritingFlow occurring by arrow key press. WritingFlow
// only supports tab transitions into text fields and to the block focus
// boundary.
Copy link
Member Author

Choose a reason for hiding this comment

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

Arrow key navigation into the appender works fine. Confirmed in Firefox too.

//
// See: https://github.com/WordPress/gutenberg/issues/4829#issuecomment-374213658
//
// If it were ever to be made to be a proper `button` element, it is
// important to note that `onFocus` alone would not be sufficient to
// capture click events, notably in Firefox.
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to make it a proper button element, p role="button" is fine and it doesn't suffer from the bug in Firefox.

//
// See: https://gist.github.com/cvrebert/68659d0333a578d75372

// The wp-block className is important for editor styles.

return (
<div
data-root-client-id={ rootClientId || '' }
className="wp-block block-editor-default-block-appender"
className="block-editor-default-block-appender"
>
<TextareaAutosize
<p
tabIndex="0"
// Only necessary for `useCanvasClickRedirect` to consider it
// as a target. Ideally it should consider any tabbable target,
// but the inserter is rendered in place while it should be
// rendered in a popover, just like it does for an empty
// paragraph block.
contentEditable
Copy link
Member Author

Choose a reason for hiding this comment

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

So, as commented above, we need move the inserter component below to a popover (just like in the empty paragraph) in order for this to work correctly without adding the content editable attribute. Will do that in a follow up.

suppressContentEditableWarning
// We want this element to be styled as a paragraph by themes.
// eslint-disable-next-line jsx-a11y/no-noninteractive-element-to-interactive-role
role="button"
aria-label={ __( 'Add block' ) }
className="block-editor-default-block-appender__content"
readOnly
// The wp-block className is important for editor styles.
className="wp-block block-editor-default-block-appender__content"
onFocus={ onAppend }
value={ showPrompt ? value : '' }
/>
>
{ showPrompt ? value : ZWNBSP }
</p>
<Inserter
rootClientId={ rootClientId }
position="bottom right"
Expand Down
42 changes: 3 additions & 39 deletions packages/block-editor/src/components/default-block-appender/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,34 +12,9 @@
outline: 1px solid transparent;
}

textarea.block-editor-default-block-appender__content { // Needs specificity in order to override input field styles from WP-admin styles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Check out how much CSS we can remove due to this change. Very cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice!

font-family: inherit;
font-size: inherit;
border: none;
background: none;
box-shadow: none;
display: block;
cursor: text;
width: 100%;
outline: $border-width solid transparent;
transition: 0.2s outline;
@include reduce-motion("transition");
margin-top: $default-block-margin;
margin-bottom: $default-block-margin;

// This needs high specificity as it's output as an inline style by the library.
resize: none !important;

// Emulate the dimensions of a paragraph block.
// On mobile and in nested contexts, the plus to add blocks shows up on the right.
// The rightmost padding makes sure it doesn't overlap text.
padding: 0 #{ $block-padding + $button-size } 0 0;

// Use opacity to work in various editor styles.
color: $dark-gray-placeholder;
.is-dark-theme & {
color: $light-gray-placeholder;
}
.block-editor-default-block-appender__content {
// Set the opacity of the initial block appender to the same as placeholder text in an empty Paragraph block.
opacity: 0.62;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Is there a SASS variable for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I checked and I think it's fine for now to not create a variable. The value is only used in one other place and there's a comment on the reason for this value. Maybe we should use a class instead of a variable? Cc @jasmussen

In any case, we can iterate.

}

// Dropzone.
Expand All @@ -48,11 +23,6 @@
}
}

// Ensure that the height of the first appender, and the one between blocks, is the same as text.
.block-editor-default-block-appender__content {
line-height: $editor-line-height;
}

// Empty / default block side inserter.
.block-editor-block-list__empty-block-inserter.block-editor-block-list__empty-block-inserter, // Empty paragraph, needs specificity to override inherited popover styles.
.block-editor-default-block-appender .block-editor-inserter { // Empty appender.
Expand All @@ -79,9 +49,3 @@
display: none;
}
}

.block-editor-default-block-appender .block-editor-inserter {
@include break-small {
align-items: center;
}
}
45 changes: 27 additions & 18 deletions packages/block-editor/src/components/default-block-appender/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@

exports[`DefaultBlockAppender should append a default block when input focused 1`] = `
<div
className="wp-block block-editor-default-block-appender"
className="block-editor-default-block-appender"
data-root-client-id=""
>
<ForwardRef
<p
aria-label="Add block"
className="block-editor-default-block-appender__content"
className="wp-block block-editor-default-block-appender__content"
contentEditable={true}
onFocus={
[MockFunction] {
"calls": Array [
Expand All @@ -21,10 +22,12 @@ exports[`DefaultBlockAppender should append a default block when input focused 1
],
}
}
readOnly={true}
role="button"
value="Type / to choose a block"
/>
suppressContentEditableWarning={true}
tabIndex="0"
>
Type / to choose a block
</p>
<WithSelect(WithDispatch(IfCondition(Inserter)))
__experimentalIsQuick={true}
isAppender={true}
Expand All @@ -35,17 +38,20 @@ exports[`DefaultBlockAppender should append a default block when input focused 1

exports[`DefaultBlockAppender should match snapshot 1`] = `
<div
className="wp-block block-editor-default-block-appender"
className="block-editor-default-block-appender"
data-root-client-id=""
>
<ForwardRef
<p
aria-label="Add block"
className="block-editor-default-block-appender__content"
className="wp-block block-editor-default-block-appender__content"
contentEditable={true}
onFocus={[MockFunction]}
readOnly={true}
role="button"
value="Type / to choose a block"
/>
suppressContentEditableWarning={true}
tabIndex="0"
>
Type / to choose a block
</p>
<WithSelect(WithDispatch(IfCondition(Inserter)))
__experimentalIsQuick={true}
isAppender={true}
Expand All @@ -56,17 +62,20 @@ exports[`DefaultBlockAppender should match snapshot 1`] = `

exports[`DefaultBlockAppender should optionally show without prompt 1`] = `
<div
className="wp-block block-editor-default-block-appender"
className="block-editor-default-block-appender"
data-root-client-id=""
>
<ForwardRef
<p
aria-label="Add block"
className="block-editor-default-block-appender__content"
className="wp-block block-editor-default-block-appender__content"
contentEditable={true}
onFocus={[MockFunction]}
readOnly={true}
role="button"
value=""
/>
suppressContentEditableWarning={true}
tabIndex="0"
>

</p>
<WithSelect(WithDispatch(IfCondition(Inserter)))
__experimentalIsQuick={true}
isAppender={true}
Expand Down
8 changes: 4 additions & 4 deletions packages/block-editor/src/components/default-block-appender/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { shallow } from 'enzyme';
/**
* Internal dependencies
*/
import { DefaultBlockAppender } from '../';
import { DefaultBlockAppender, ZWNBSP } from '../';

describe( 'DefaultBlockAppender', () => {
const expectOnAppendCalled = ( onAppend ) => {
Expand Down Expand Up @@ -35,7 +35,7 @@ describe( 'DefaultBlockAppender', () => {
<DefaultBlockAppender isVisible onAppend={ onAppend } showPrompt />
);

wrapper.find( 'ForwardRef' ).simulate( 'focus' );
wrapper.find( 'p' ).simulate( 'focus' );

expect( wrapper ).toMatchSnapshot();

Expand All @@ -51,9 +51,9 @@ describe( 'DefaultBlockAppender', () => {
showPrompt={ false }
/>
);
const input = wrapper.find( 'ForwardRef' );
const input = wrapper.find( 'p' );

expect( input.prop( 'value' ) ).toEqual( '' );
expect( input.prop( 'children' ) ).toEqual( ZWNBSP );

expect( wrapper ).toMatchSnapshot();
} );
Expand Down
7 changes: 3 additions & 4 deletions packages/edit-post/src/components/visual-editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,9 @@
margin-left: auto;
margin-right: auto;

// Apply default block margin below the post title.
// This ensures the first block on the page is in a good position.
// This rule can be retired once the title becomes an actual block.
margin-bottom: $default-block-margin;
// Margins between the title and the first block, or appender, do not collapse.
// By explicitly setting this to zero, we avoid "double margin".
margin-bottom: 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

What makes this different from a normal paragraph? Do we have this rule for the paragraph block too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can be removed. I think at one point a "wp-block" class was attached to this as well. But it looks like it can be removed from my testing!

}
}

Expand Down