Default appender: try editable paragraph instead of text area #30986
Conversation
|
Thanks so much for taking a look. This is what I see before:
And this is after:
The jump is gone! And because it is a paragraph, just like #30656, it should inherit any fonts, font styles, line heights and margins that themes would assign this appender. Excellent! We should probably add some CSS to make the opacity of the placeholder text the same as that of an empty paragraph, which is 0.62:
I can push some CSS if you like? There's probably also some CSS from 30656 that we could copy over, such as https://github.com/WordPress/gutenberg/pull/30656/files#diff-5337798f1b609fe4df9b9fe53f0e4bfdf349b1ceebec2a52b6e62cf999398615R27 and https://github.com/WordPress/gutenberg/pull/30656/files#diff-5e55c7c9e7afd644f3829ad8df092bcb826a1881ca57cc6df51100414d78042fR58. |
Of course, feel free! |
|
Size Change: -111 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
…rdPress/gutenberg into try/content-editable-default-appender
| @@ -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. | |||
There was a problem hiding this comment.
Check out how much CSS we can remove due to this change. Very cool!
|
Alright, I should've pulled before I pushed, so there's a small merge commit as well. But as far as I can tell, this looks great now, and works even better. Note that in the CSS that was removed, there were a few references to "dark themes". Because I would expect the question to come up in reviews, I'll address it right here. When the appender was a textarea, it could not reliably inherit theme colors and know that it would work. For that reason, placeholder text color was explicitly set. Because of that, we also needed to change colors based on a calculated "dark theme". Because it's now a paragraph, it will always inherit the color set by the theme, which means there's no reason for us to explicitly set the colors. That also means we don't need to set them for dark themes. And to be sure, I tested that colors work fine there too:
|
| // 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. |
There was a problem hiding this comment.
Arrow key navigation into the appender works fine. Confirmed in Firefox too.
| // | ||
| // 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. |
There was a problem hiding this comment.
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.
ellatrix
force-pushed
the
try/content-editable-default-appender
branch
from
cf4f524 to
36e8678
Compare
| 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; |
There was a problem hiding this comment.
What makes this different from a normal paragraph? Do we have this rule for the paragraph block too?
There was a problem hiding this comment.
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!
|
Nice cleanup! |
| // 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 |
There was a problem hiding this comment.
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.
|
@jasmussen Is this good to be merged? |
|
I think so, but because I was so involved I would love if we could get a sanity check. @gwwar do you have a spare moment to give a thumbs up? To be clear, in my testing this works very well, so it's mostly a gutcheck. |
|
For folks testing this, to select the There's some additional setup required per browser/OS to make sure this tabbing works. See instructions in: https://www.a11yproject.com/posts/2017-12-29-macos-browser-keyboard-navigation/ |
There was a problem hiding this comment.
Lovely simplification @ellatrix @jasmussen ✨
I manually tested this with FF, Safari, and Chrome. Shift+tabbing to select the appender with keyboard only still works without the textarea!
One oddity is that VoiceOver literally reads the non breaking white space, if the paragraph is selected and we press the down key, but it looks like this behavior is also present in trunk.
I left a note on a potential alternative for the non breaking space, but this tests well for me. I also smoke tested other container types, but it might be worth giving folks a heads up in core editor slack after merging to see if anyone notices differences in insertion behavior.
| } | ||
| .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; |
There was a problem hiding this comment.
Minor: Is there a SASS variable for this?
There was a problem hiding this comment.
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.
| * Zero width non-breaking space, used as padding for the paragraph when it is | ||
| * empty. | ||
| */ | ||
| export const ZWNBSP = '\ufeff'; |
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
An empty string won't fill the element. We use the same character for rich text padding.
|
Hm, if VoiceOver reads out the character, this will happen for rich text padding too, so perhaps we should investigate it separately. |
Yes, the behavior is already on trunk so I think it's fine for a follow up issue 👍 |
Description
Swaps out the text area in the default appender (which isn't styled by themes) for a paragraph with a button role.
How has this been tested?
Create a new post and click the default appender.
Specifically in Firefox, check that you can use the arrow down key to navigate to the appender.
Also create a post that ends with a block other than a paragraph. There will be a default appender without label.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).