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

Restore hiding drop cap on focus to prevent bugs with contenteditable #6359

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Apr 23, 2018

Description

Fixes #6273.

This behaviour seems to have been dropped by accident in #1418. Not sure why, but maybe @BE-Webdesign recalls this PR?

We can't show the drop cap when the user is editing the paragraph for some reasons:

  • You cannot move the caret around it.
  • Chrome (maybe others) fails to calculate the caret position with range.getClientRects(), which causes errors and things that rely on that such as the link inserter and writing flow to break.
  • Typing seems broken in different ways in different browsers. Chrome and Safari seem to insert characters in the wrong position, and additionally Safari just overwrites the last character when the caret is at then end.

I know this isn't a nice solution, but it's all I can come up with at the moment. These CSS rules and contenteditable just don't seem compatible and I've not found a way to make it work. I'm unsure if there's any way we can let the user know that the block has a drop cap while the editable field has focus. Cc @jasmussen.

How has this been tested?

Ensure the above points work fine.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

}
}

p.has-drop-cap:not( :focus ) {
overflow: hidden;
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this line because it seems to have just been taken over from the editable CSS, where it targeted a different element. I don't know why this line was needed back then and if it still does anything now, but it seems unlikely. It should have been commented.

@ellatrix ellatrix added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Apr 23, 2018
@ellatrix ellatrix added this to the 2.8 milestone Apr 23, 2018
@ellatrix ellatrix requested a review from a team April 23, 2018 16:45
@ellatrix
Copy link
Member Author

Maybe we could try to add the drop cap in JavaScript instead... Extract the letter, but it in an element and align it. Maybe worth trying in a separate PR.

@jasmussen
Copy link
Contributor

I feel like it is totally totally fine to not show the dropcap when you're typing. I can't imagine we can make the typing experience feel very good with a dropcap there, contenteditable bugs or not. Dropcap is inherently a decorative element, usually something you apply after the fact. And finally, it is completely according the design guidelines, that the selected block can change appearance to surface buttons or controls or change layout. It is the unselected block that should be a preview of the end result. As such, just from a design POV, this seems completely fine to do.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Only glitch I saw (but not sure how to fix) is if the paragraph is the last in the post and you click under the paragraph, it flickers because the focus is moved out and back to the paragraph.

@mcsf
Copy link
Contributor

mcsf commented Apr 24, 2018

This makes the drop cap usable in Safari but I'm still seeing this, which hopefully we can address in this very PR:

  1. Select a plain Paragraph block; type in it.
  2. With the mouse, toggle drop cap attribute. The decoration is added, even though the block is selected. The caret also remains roughly where it was in the RichText.
  3. Now, type some more: you'll still experience the parent issue.
  4. To fix, deselect and select the block.

gutenberg-drop-cap-toggle-bug

@ellatrix
Copy link
Member Author

it flickers because the focus is moved out and back to the paragraph

The focus back in is intended, not sure how to avoid the flicker either, we'd probably have to prevent moving focus away at all.

Thanks for reviewing!

I love that you can just use an emoji as a drop cap haha.

screen shot 2018-04-24 at 11 58 41

I can already predict a future where we'll have tons of posts with giant emoji in front of paragraphs on the internet. Cc @pento.

@ellatrix ellatrix merged commit fcecbb2 into master Apr 24, 2018
@ellatrix ellatrix deleted the fix/drop-cap-contenteditable branch April 24, 2018 10:01
@ellatrix
Copy link
Member Author

ellatrix commented Apr 24, 2018

@mcsf Sorry, I caught your comment too late... I can't reproduce this issue though. As soon as I start typing in Safari, the drop cap disappears for me. Not sure what is going on with the :focus selector in your case. :/

@ellatrix
Copy link
Member Author

Here's what I get:

drop-cap

@ellatrix
Copy link
Member Author

@mcsf I can reproduce now, but I have to start with a brand new block as you suggested, which makes it even stranger.

@ellatrix
Copy link
Member Author

I'm unsure atm what we can do about tis...

@lsinger
Copy link

lsinger commented Apr 24, 2018

Should clicking outside of the text area blur the cursor, possibly?

@ellatrix
Copy link
Member Author

@lsinger Yeah, that sounds like the right behaviour. It should blur the text field, but keep the block selection.

@lsinger
Copy link

lsinger commented Apr 24, 2018

that sounds like the right behaviour

I wonder though whether it would be the right behavior only in this specific case or whether that still holds true in the general case. 🤔

@ellatrix
Copy link
Member Author

Focus should normally move to the drop cap switcher, as it does correctly in Chrome.

@ellatrix
Copy link
Member Author

Focus behaviour in Safari is very different... you can't move focus to every focusable element by default. Just tab through the blocks and you'll see a big difference in behaviour. Cc @afercia.

jeremyfelt added a commit to bu-ist/bu-blocks that referenced this pull request Dec 11, 2018
`contenteditable` doesn't appear to work seamlessly when style is
applied differently on one of the inline letters. This change
will apply the drop cap styling in the editor only when the block
is not focused.

See WordPress/gutenberg#6359 for the same
bug fix in Gutenberg.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants