Rich Text: Indicate which text will be turned into a link #8807

noisysocks · 2018-08-10T05:54:35Z

Fixes #6156. Supersedes #4909.

When inserting a link, the text selection disappears when the focus changes into the URLInput text field. This makes it hard to tell which text will be turned into a link.

The Classic Editor solves this by inserting a placeholder <a> element, which is the approach that I'm borrowing here.

The placeholder <a> is removed when the selection changes or when the block is deselected. We set data-mce-bogus on the placeholder <a> so that it doesn't ever get serialised into post content.

To test:

I've included E2E tests for most basic link interactions, so look for the ✅!

It's worthwhile manually testing that no undo levels are created when placeholder links are created and destroyed. To do this:

Add a console.log( 'undo level created' ) to RichText::onCreateUndoLevel()
Select some text
Press the Link button in the formatting toolbar
Verify that the selected text looks like a link and that no undo level was created
Click on something else
Verify that there's no longer a link and that no undo level was created
Create a link for real this time
Verify that a link appears and that an undo level was created

talldan

Works well, and code looks good, though I'm not particularly familiar with the workings of RichText. This was a good chance to learn a few things.

I think the test descriptions could be improved, happy with the rest.

talldan · 2018-08-15T08:47:47Z

packages/editor/src/components/rich-text/index.js

 	switch ( formatName ) {
 		case 'link' : {
-			const anchor = find( parents, ( node ) => node.nodeName.toLowerCase() === 'a' );
-			return !! anchor ? { value: anchor.getAttribute( 'href' ) || '', target: anchor.getAttribute( 'target' ) || '', node: anchor } : {};
+			const anchor = find( parents, ( node ) => node.nodeName === 'A' );


Just wondering why keep this a switch? Is it for future extensibility?

Yeah, it's probably not useful. Removed in 7d2ffce 👌

talldan · 2018-08-15T09:31:23Z

test/e2e/specs/managing-links.test.js

 describe( 'Managing links', () => {
 	beforeEach( async () => {
 		await newPost();
 	} );

+	it( 'Creating a link by selecting text and clicking the Link UI', async () => {


I think this should be something like this, so that it makes a nice sentence:

describe( 'Creating a link by selecting text and clicking the Link UI', () => { it( 'creates the link placeholder', async () => {

seems to be the same thing with the other test descriptions as well.

I've rejigged the describe() and it() descriptions in 543da2f to read like English sentences 🇬🇧

Thanks for the review! I've fixed your comments

noisysocks · 2018-08-17T00:05:49Z

I'm not particularly familiar with the workings of RichText. This was a good chance to learn a few things.

Yay, me too! 😂

Thanks for the review. I'd like some 👀 from @iseulde or @azaozz before continuing since they're the in-house TinyMCE experts.

azaozz · 2018-08-20T14:44:42Z

Played with this a bit more. Works well as far as I can tell. Was a bit concerned that we do find() on the nodes passed from TinyMCE's nodechange that may not include the end node in a range in some cases (as far as I remember), but wasn't able to trigger that.

Noticed some general inconsistencies with adding a link though. Not sure if any is related to this pr, but sometimes undo doesn't undo just the action of adding a link. It removes/reverts most of the typing together with the link as one step, and redo add everything back in one step.

Testing in the Classic block, it behaves quite inconsistently, especially when the (old) link modal is used (by clicking the cogwheel there). That will probably need another issue if one doesn't exist already.

azaozz · 2018-08-20T14:54:14Z

Uh, one more: when clicking in the middle of a word (not selecting it) and clicking the Add link button in the Classic editor the word is selected automatically. Here the word is not selected. That results in inserting a link where the linked text is the whole URL. So we end up with the word split by an linked URL.

Thinking this should work like in the Classic editor and select the word where the caret is as soon as Add link is clicked. It works that way when setting to "bold" for example.

When inserting a link, the text selection disappears when the focus changes into the URLInput text field. This makes it hard to tell which text will be turned into a link. The Classic Editor solves this by inserting a placeholder <a> element, which is the approach that we borrow here.

A `switch` is overkill when there's only one format value that acts differently.

Renames managing-links → links and makes the test descriptions read like English sentences.

noisysocks · 2018-08-20T23:49:09Z

Thanks @azaozz! Since those issues you've identified exist in master let's ticket and address them separately.

Noticed some general inconsistencies with adding a link though. Not sure if any is related to this pr, but sometimes undo doesn't undo just the action of adding a link. It removes/reverts most of the typing together with the link as one step, and redo add everything back in one step.

Could you create an issue for this? I'm unable to reproduce it in my environment.

Thinking this should work like in the Classic editor and select the word where the caret is as soon as Add link is clicked. It works that way when setting to "bold" for example.

Agree with this. Let's tackle it in #9183.

noisysocks · 2018-08-20T23:49:29Z

@talldan @azaozz: Can one of you approve this to make GitHub happy? 🙂

aduth · 2018-10-19T15:40:49Z

test/e2e/specs/links.test.js

+		expect( await getEditedPostContent() ).toMatchSnapshot();
+	} );
+
+	const setFixedToolbar = async ( b ) => {


b? What is b?

Verbose good. Cryptic bad.

noisysocks added [Type] Bug An existing feature does not function as intended [Status] In Progress Tracking issues with work in progress [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Aug 10, 2018

noisysocks force-pushed the fix/disappearing-link-selection branch 7 times, most recently from 8dcb073 to 06c8f1a Compare August 14, 2018 06:42

noisysocks removed the [Status] In Progress Tracking issues with work in progress label Aug 14, 2018

noisysocks requested review from ellatrix, azaozz and talldan August 14, 2018 07:09

talldan previously requested changes Aug 15, 2018

View reviewed changes

noisysocks requested a review from talldan August 17, 2018 00:03

noisysocks force-pushed the fix/disappearing-link-selection branch from e5ef740 to 543da2f Compare August 17, 2018 01:17

noisysocks mentioned this pull request Aug 17, 2018

Automatically create a link when selected text is a URL #9067

Merged

noisysocks added this to the 3.7 milestone Aug 20, 2018

noisysocks added 4 commits August 21, 2018 09:25

Add E2E tests for creating, editing and removing links

7e0be36

Use an if instead of a switch in getFormatValue()

213b8d1

A `switch` is overkill when there's only one format value that acts differently.

Rename Link E2E tests

47d0bd8

Renames managing-links → links and makes the test descriptions read like English sentences.

noisysocks force-pushed the fix/disappearing-link-selection branch from 543da2f to 47d0bd8 Compare August 20, 2018 23:25

noisysocks mentioned this pull request Aug 20, 2018

Placing cursor and clicking Add Link should make the whole word a link #9183

Closed

talldan approved these changes Aug 21, 2018

View reviewed changes

noisysocks merged commit 4a184d5 into master Aug 21, 2018

ajitbohra deleted the fix/disappearing-link-selection branch August 21, 2018 12:49

noisysocks mentioned this pull request Aug 22, 2018

Rich text formatting filter #6642

Closed

4 tasks

aduth reviewed Oct 19, 2018

View reviewed changes

aduth mentioned this pull request Oct 19, 2018

Components: Add info prop support to MenuItem, FeatureToggle #10802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rich Text: Indicate which text will be turned into a link #8807

Rich Text: Indicate which text will be turned into a link #8807

noisysocks commented Aug 10, 2018 •

edited

Loading

talldan left a comment

talldan Aug 15, 2018

noisysocks Aug 17, 2018

talldan Aug 15, 2018 •

edited

Loading

noisysocks Aug 17, 2018 •

edited

Loading

noisysocks commented Aug 17, 2018 •

edited

Loading

azaozz commented Aug 20, 2018

azaozz commented Aug 20, 2018

noisysocks commented Aug 20, 2018

noisysocks commented Aug 20, 2018

aduth Oct 19, 2018

Rich Text: Indicate which text will be turned into a link #8807

Rich Text: Indicate which text will be turned into a link #8807

Conversation

noisysocks commented Aug 10, 2018 • edited Loading

talldan left a comment

Choose a reason for hiding this comment

talldan Aug 15, 2018

Choose a reason for hiding this comment

noisysocks Aug 17, 2018

Choose a reason for hiding this comment

talldan Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

noisysocks Aug 17, 2018 • edited Loading

Choose a reason for hiding this comment

noisysocks commented Aug 17, 2018 • edited Loading

azaozz commented Aug 20, 2018

azaozz commented Aug 20, 2018

noisysocks commented Aug 20, 2018

noisysocks commented Aug 20, 2018

aduth Oct 19, 2018

Choose a reason for hiding this comment

noisysocks commented Aug 10, 2018 •

edited

Loading

talldan Aug 15, 2018 •

edited

Loading

noisysocks Aug 17, 2018 •

edited

Loading

noisysocks commented Aug 17, 2018 •

edited

Loading