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

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

Merged
merged 4 commits into from Aug 21, 2018

Conversation

Projects
None yet
3 participants
@noisysocks
Member

noisysocks commented Aug 10, 2018

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.

links

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:

  1. Add a console.log( 'undo level created' ) to RichText::onCreateUndoLevel()
  2. Select some text
  3. Press the Link button in the formatting toolbar
  4. Verify that the selected text looks like a link and that no undo level was created
  5. Click on something else
  6. Verify that there's no longer a link and that no undo level was created
  7. Create a link for real this time
  8. 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.

@noisysocks noisysocks requested a review from talldan Aug 17, 2018

Thanks for the review! I've fixed your comments

@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Aug 17, 2018

Member

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.

Member

noisysocks commented Aug 17, 2018

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

This comment has been minimized.

Show comment
Hide comment
@azaozz

azaozz Aug 20, 2018

Contributor

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.

Contributor

azaozz commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@azaozz

azaozz Aug 20, 2018

Contributor

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.

Contributor

azaozz commented Aug 20, 2018

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.

noisysocks added some commits Aug 10, 2018

Rich Text: Indicate which text will be turned into a link
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.
Use an `if` instead of a `switch` in getFormatValue()
A `switch` is overkill when there's only one format value that acts
differently.
Rename Link E2E tests
Renames managing-links → links and makes the test descriptions read like
English sentences.
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Aug 20, 2018

Member

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.

Member

noisysocks commented Aug 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Aug 20, 2018

Member

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

Member

noisysocks commented Aug 20, 2018

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

@noisysocks noisysocks merged commit 4a184d5 into master Aug 21, 2018

2 checks passed

codecov/project 50.87% (-0.04%) compared to f631337
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ajitbohra ajitbohra deleted the fix/disappearing-link-selection branch Aug 21, 2018

@noisysocks noisysocks referenced this pull request Aug 22, 2018

Closed

Rich text formatting filter #6642

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment