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

Pasting a plain text will inherit selection attributes #7778

Merged
merged 4 commits into from
Aug 6, 2020
Merged

Conversation

pomek
Copy link
Member

@pomek pomek commented Aug 5, 2020

Suggested merge commit message (convention)

Feature (clipboard): Pasting a plain text will inherit selection attributes. Closes #1006.

@pomek pomek requested a review from jodator August 5, 2020 07:08
@Reinmar
Copy link
Member

Reinmar commented Aug 5, 2020

I wonder if that's a correct heuristic for detecting whether text attributes should be inherited, as it actually does not check what was in the clipboard but instead what is it in the model. It's not something that was discussed, but it has its merits.

@Mgsy, you know most about the issues that our community and customers are reporting. Could you test it from the user perspective and tell us how do you feel about this approach?

@Reinmar Reinmar requested a review from Mgsy August 5, 2020 08:58
@jodator
Copy link
Contributor

jodator commented Aug 5, 2020

t actually does not check what was in the clipboard but instead what is it in the model.

Well we can't check view data for that also. We could however introduce some additional data for the listeners.

The ctrl+shift+v pastes text/plain only data - so we might think about extending inputTransformation data with isPlainText attribute.

The other thing is how browser behaves when pasting plain text. AFAICS there are two cases:

  1. text from plain text editor (ie from address bar) - it is the same as ctrl+shift+v case so the flag would cover that.
  2. text from editor/editable (ie have 'foo' copied)
    1. Firefox pastes foo as both text/html and text/plain - a simple string comparison would be enough to set isPlainText
    2. Chrome however, would paste <meta http-equiv="content-type" content="text/html; charset=utf-8">foo as text/html - here I see a potential fix for this.
    3. I didn't check Safari nor Edge

But OTOH the @pomek's check operates on model level, after conversion so it would probably catch the situation of pasting not-enabled formatting (let's say strikethrouhg) in editor. In the end we will insert single text node (without attributes) so considering pasting "foo" into "bar | bar" - user could see either (none of which have unsupported formatting)

  1. "bar foo| bar" (formatting copied from selection)
  2. "bar foo**| bar**" (pasted as plain-text)

If, we'd like option 1 in this case I think that this check will actually be valid.

If we however would like option 2 better - then we should check for a true plain text in the clipboard.

@pomek - before we get agreement on solution I see that we have too little tests:

  1. Plain text in the clipboard when both text/html and text/plain are available - let's test both FF and Chrome cases.
  2. Unsupported formatting in the clipboard pasted.

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

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

As in the discussion comment above - a few more tests are needed:

  1. Plain text in data transfer set in text/html and text/plain.
  2. As above but in Chrome-style: <meta http-equiv="content-type" content="text/html; charset=utf-8">foo (then foo in text/plain).
  3. Unsupported formatting in text/html.

@pomek
Copy link
Member Author

pomek commented Aug 6, 2020

@jodator, I added the mentioned tests.

@jodator jodator merged commit 2a163e3 into master Aug 6, 2020
@jodator jodator deleted the i/1006 branch August 6, 2020 09:29
@Reinmar
Copy link
Member

Reinmar commented Aug 6, 2020

  1. Firefox pastes foo as both text/html and text/plain - a simple string comparison would be enough to set isPlainText

Good idea, we could consider that. That'd be a bit more complex as the HTML could look like this: 'foo &nbsp;bar&gt;bom' (foo  bar<bom) but with some simple regexp replace it's doable.

As for the other things that @jodator wrote, I think that we need to be predictable:

  • If you pressed ctrl+shift+v it's very likely that you want to inherit selection attributes. But then @pomek's solution won't handle multi-line text as in the model it'd contain soft breaks or paragraphs. Do we want to handle multi-line text the same way as a single line? If yes, we need an additional check for that.
  • If you pressed ctrl+v but you had very simple content in the clipboard, how likely is it that you want it to inherit selection styles? I'd say, quite likely, so I found @pomek's idea really interesting. However, I'm slightly worried about the randomness of this solution. I copied some formatted text from paste from office or other application and that formatting is not supported by the editor – that will make this text to inherit the selection styles. But if a supported style was included in that text, the style will not be inherited. It may feel "random". But that's where I count on your intuition and experience, @Mgsy. Can we live with that?

@jodator jodator mentioned this pull request Aug 7, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasted plain text should inherit formatting of the surrounding text
3 participants