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

Run emoji script correctly in editable #3520

Closed
wants to merge 1 commit into from
Closed

Conversation

ellatrix
Copy link
Member

Description

See #2799. In some browsers, the emoji script should be run. This PR adds it to editable correctly, similar to WP core, and removes the images on save.

How Has This Been Tested?

In Chrome, verify that emoji are replaced with an image on insertion. Switch to HTML mode. There should be no image. Switch back to visual, and there should again be an image.
In Chrome, paste some text from a WordPress site with emoji into Gutenberg. It should work as expected.

@ellatrix ellatrix requested review from mcsf and pento November 16, 2017 14:53
@codecov
Copy link

codecov bot commented Nov 16, 2017

Codecov Report

Merging #3520 into master will decrease coverage by 0.03%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3520      +/-   ##
==========================================
- Coverage   34.54%   34.51%   -0.04%     
==========================================
  Files         261      271      +10     
  Lines        6710     7588     +878     
  Branches     1225     1471     +246     
==========================================
+ Hits         2318     2619     +301     
- Misses       3704     4114     +410     
- Partials      688      855     +167
Impacted Files Coverage Δ
blocks/editable/index.js 9.72% <0%> (-0.52%) ⬇️
blocks/api/raw-handling/image-corrector.js 74.07% <100%> (+2.07%) ⬆️
blocks/editable/emoji.js 4.54% <4.54%> (ø)
blocks/library/text-columns/index.js 25.92% <0%> (-2.65%) ⬇️
blocks/library/pullquote/index.js 35.71% <0%> (-1.79%) ⬇️
blocks/library/heading/index.js 23.68% <0%> (-1.32%) ⬇️
blocks/editable/patterns.js 1.23% <0%> (-0.62%) ⬇️
blocks/library/preformatted/index.js 44.44% <0%> (ø) ⬆️
editor/modes/visual-editor/sibling-inserter.js 0% <0%> (ø) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7aaa99...29aa24a. Read the comment docs.

@ellatrix ellatrix changed the title Run emoji script in editable Run emoji script correctly in editable Nov 16, 2017
@@ -57,4 +57,9 @@ export default function( node ) {
if ( node.height === 1 || node.width === 1 ) {
node.parentNode.removeChild( node );
}

// Restore emoji as images.
if ( node.className === 'emoji' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to also verify that there is a usable node.alt in the if?

@mcsf
Copy link
Contributor

mcsf commented Nov 16, 2017

I'm experiencing this:

gutenberg-emoji-broken

@mcsf
Copy link
Contributor

mcsf commented Nov 16, 2017

^ Also present in that screencast is some weirdness in caret position when moving around the emoji with the arrows. Chrome is Version 62.0.3202.94 (Official Build) (64-bit).

@ellatrix
Copy link
Member Author

The issue with the caret is probably one of the concerns @ephox-mogran raised regarding images and arrow key nav some time ago.

@ephox-mogran
Copy link
Contributor

If the emojis are being represented by img tags inside the editor, then it is possible that using textContent in your edge detection of blocks is breaking. See #3014 and #3015 for details.

Short reason is that img tags don't have any text content, and a block with just an image tag in doesn't have any text content, therefore, it things you are at the end of your block on the first character.

I don't have an environment here to check this, though, so it could be unrelated to the issue. I'll try to look into it today if @iseulde hasn't fixed it already :)

@ellatrix
Copy link
Member Author

@mcsf I have the same Chrome version, but cannot reproduce the emoji => text issue... Save draft + refresh saves normally.

@ephox-mogran
Copy link
Contributor

@iseulde It looks like it is the edge detection. I've updated #3015 to match master (and fixes a few things). I'm not 100% sure it's ready for review, though --- I'd like to add some tests. It seems to handle the emoji better (so you can try it out), but there will always be edge cases when dealing with zero-width cursors, br tags, hr tags, input fields, image tags etc. As soon as you move away from just a container with text, edge detection becomes a lot more complex.

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 1, 2018
@aduth
Copy link
Member

aduth commented Mar 1, 2018

What's the status of this pull request?

@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Mar 1, 2018
@jeffpaul jeffpaul added this to the Merge Proposal milestone Mar 8, 2018
@ellatrix
Copy link
Member Author

ellatrix commented Mar 8, 2018

Let's disable instead per chat. #2799 remains open.

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 [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants