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 after selecting html tag pastes twice #87067

Closed
mebibou opened this issue Dec 16, 2019 · 22 comments
Closed

Pasting after selecting html tag pastes twice #87067

mebibou opened this issue Dec 16, 2019 · 22 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug editor-synced-region Issues related to synced region functionality in editor html HTML support issues verified Verification succeeded
Milestone

Comments

@mebibou
Copy link

mebibou commented Dec 16, 2019

  • VSCode Version: 1.41.0
  • OS Version: MacOS 10.15.1

Steps to Reproduce:

  1. Write the html code <div></div>
  2. Click after <div (put cursor before the end of opening tag)
  3. Paste some content

=> The pasted content will appear both after <div and after </div. Example with pasting the content class="test" will output <div class="test"></div class="test">

This didn't happen before, it is quite recent bug

@gjsjohnmurray
Copy link
Contributor

Looks like a fault with this new 1.41 feature.

cc @octref

@octref
Copy link
Contributor

octref commented Dec 16, 2019

This is as-designed, since when you click after <div we creat another cursor. If you type space at this point, we do a special handling to remove the space that would have been inserted at the ending tag. All other actions don't receive this special handling.

You can also turn this off with html.mirrorCursorOnMatchingTag and stick to using F2.

@octref octref added the *as-designed Described behavior is as designed label Dec 16, 2019
@vscodebot
Copy link

vscodebot bot commented Dec 16, 2019

The described behavior is how it is expected to work. If you disagree, please explain what is expected and what is not in more detail. See also our issue reporting guidelines.

Happy Coding!

@vscodebot vscodebot bot closed this as completed Dec 16, 2019
@helgemahrt
Copy link

In my opinion there is a bug here. Yes, the feature should create a mirror cursor on a matching tag, but when I'm pasting content with a leading space onto the opening tag, I'd expect the mirroring cursor to handle this like having typed the space manually.
I encounter this a lot because I want to copy some attribute to another tag. On new tags, like

, the closing ">" is right behind the tag name and therefore I include leading spaces when copying, so I can just put the cursor right at the end of the tag and paste. Having to manually press space is an additional step in my workflow and makes this feature rather cumbersome.

@mebibou
Copy link
Author

mebibou commented Dec 17, 2019

Agreed this new feature should handle pasted content with leading space, just like if you typed it manually. Makes total sense

@gavin310
Copy link

gavin310 commented Dec 17, 2019

@octref I'm sorry, but I don't understand how this can possibly be "as-designed"? Shouldn't pasting detect spaces as well?

Also, many times when typing into an HTML tag it does not detect the space. I'll try to figure out if there's something repeatable that causes that issue to happen.

vscode-bug

@octref
Copy link
Contributor

octref commented Dec 17, 2019

OK I'll look into how to do this. Meanwhile you can press ESC before pasting for now.

@octref octref reopened this Dec 17, 2019
@octref octref added bug Issue identified by VS Code Team member as probable bug html HTML support issues and removed *as-designed Described behavior is as designed labels Dec 17, 2019
@octref octref added this to the December/January 2020 milestone Dec 17, 2019
@AleksandrHovhannisyan
Copy link

I was about to create a separate issue, but the following seems to be related to this behavior:

demo

Steps to reproduce:

  1. Create a <div></div>.

  2. Place the cursor to the right of the closing div tag, and move it left once, either with your mouse or with the left arrow key.

  3. Move the cursor back to its original position, to the right of the closing tag. Notice that a mirror cursor is created.

  4. Hit Enter, or do just about anything else, and it will be mirrored with the opening tag.

Also, I concur—I don't see how this can possibly be intended behavior.

@GaryHyland
Copy link

GaryHyland commented Jan 10, 2020

Hmm.. certainly hiliting matching tags is HUGELY useful. But (certaintly for HTML code) I struggle to think of when I would be wanting to type or paste same test into both the open and closing tags..
Think I will look to turn off for present as I find I am getting a lot of compile issues and have to go back and delete text from the end tags..

@gjsjohnmurray
Copy link
Contributor

@GaryHyland, this issue has been tagged as a bug by @octref. If you are willing to use Insiders you should get earlier access to a fix.

@octref
Copy link
Contributor

octref commented Jan 30, 2020

Update: I looked through all mirror cursor issues. We'll explore #88424, which should fix some issues.
I turned html.mirrorCursorOnMatchingTag off by default. If you find this feature useful and can work around this issue (by typing a space or pressing ESC before pasting), you can turn it on by default.

I think there's no one correct solution here. I'll make #88424 work this way:

  • By default, you press a key to enter "Synced Region" mode.
  • "Synced Region" mode will have UI indication (all regions highlighted with background color).
  • When you are in "Synced Region" mode. All behaviors are duplicated, including pasting.
  • When you exit "Synced Region" mode. Your action only affect current cursor.
  • You can set a setting to automatically enter "Synced Region" mode when your cursor enter one.

I want it to work this way, so you are sure about the effect of your action. Having two modes with clear distinction, where one always duplicates action and one always doesn't, is better than making it one mode, where you are unsure if the editor will do some "magic" for you.

@octref octref modified the milestones: January 2020, Backlog Jan 30, 2020
@LWChris
Copy link

LWChris commented Feb 11, 2020

Having two modes with clear distinction, where one always duplicates action and one always doesn't, is better than making it one mode, where you are unsure if the editor will do some "magic" for you.

I do not share this sentiment. I'm fairly certain a user never wants to paste copied attributes into the closing tag, since it is always syntactically wrong.

Hence you might as well rephrase your principle of consistency into:

Having the mode consistently do what the user intended to do, is better than making it a mode, where you are unsure if the editor will do "the right thing" (aka. "some magic", i. e. revert last mirrored edit & quit mirror-mode) by default (when hitting spacebar) or not (when pasting).

@bergamin
Copy link

Until this and #90146 (selecting causes selection of things outside the end tag as well) aren't fixed, I'm turning this feature off.

It is a useful feature, but not usable right now. Looks like it wasn't tested in a real world scenario.

also, the end tag will never have more things than the name of the tag itself, therefore pasting shouldn't be part of this feature. At most, when pasting, substitute the end tag with the resulting starting tag after pasting, ignoring the clipboard content.

@octref
Copy link
Contributor

octref commented Feb 11, 2020

since it is always syntactically wrong.

To your analogy, when I press enter/space/delete on here <div></div|>, VS Code should not do anything, since that always creates syntactically wrong HTML?

We never sell this feature as "magically editing HTML so the result is always semantically correct". What the feature does is just adding another multi-cursor.

What I'll do is add clear UI indication that you are in this mode and your actions will be duplicated. What I won't do is to anticipate any keypress you can press to make HTML semantically invalid and create special-case fixes to prevent that.

Finally this is made an opt-in feature. If you are ok with "Now I see two cursors, what I do will be duplicated, so I should do some action but not others, if I want my action not duplicated I'll ESC to dismiss the extra cursor", then you can turn it on.

@jsgoupil
Copy link

@octref are you the author of this feature? You have to listen to the community. A project changes based on different usages/requests. With all the bugs around this feature, I would doubt a lot of people will turn this feature on. If you keep saying this is by design, I will also just keep this feature off.

The idea of changing the tag is genial and happens great in VS Razor, but somehow, I end up pressing CTRL+Z way more than I use to and it actually slows me down in my development with visual studio code.

The reason I'm commenting on this issue is because I actually care about the feature, but if you just say this won't happen, then you care more about your code than admitting there might be errors and it should be fixed.

@octref
Copy link
Contributor

octref commented Feb 11, 2020

@jsgoupil I'm saying it should happen in a consistent way, not a magical way. Your issue is #87732, which I plan to fix with #88424.

The dilemma is:

  • Pasting with two cursors should paste content twice.
  • Pasting content twice, in this case, creates invalid HTML.

A consistent solution is:

  • Have two clear mode. One where your action is duplicated. The other not.
  • User paste in duplicated mode. Content is pasted twice.
  • User paste in normal mode. Content is not duplicated.
  • User press an action to enter the duplicated mode.

You can see how it works in Eclipse:
Kapture 2020-02-11 at 11 05 26

What I'll not do is:

  • When you press space here, now we emit an additional edit to remove space in the closing div :<div|></div|>
  • I emit additional edit for paste, enter, delete-after, etc etc to make the HTML valid. That is the magical way that is neither consistent nor understandable. That also makes this feature inapplicable to other languages.

@jsgoupil
Copy link

Don't re-invent the wheel here. Razor did it well.

Your "mirror-cursor", is surely a great name, but the real feature (correct me if I'm wrong) is to update the matching HTML tag.
Any other side effects should be handled properly, in respect to get valid HTML (or XML...)
How you handle it under the hood is up to you.

The screenshot you pasted is handled well yes, because it's basically taking into account that in this particular language, space is not part of a variable. But I think this is quite digressing from the main problem. We are not refactoring here. We are just updating a matching HTML tag.

@octref
Copy link
Contributor

octref commented Feb 11, 2020

The screenshot you pasted is handled well yes, because it's basically taking into account that in this particular language, space is not part of a variable.

That's not my point. I meant to say:

  • When your cursor is in that region, paste is duplicated
  • When your cursor is out, paste is not duplicated
  • If you dismiss the synced region mode with ESC, paste is not duplicated

Consider this case. Suppose your paste content can either be:

  • class="foo"
  • -bar, provided <foo-bar> is a valid tag

Tell me, what should an editor do when pasting in <foo|></foo|>? Isn't <foo class="foo"></foo> and <foo-bar></foo-bar> both valid?

That's why I'm making it explicit when editor is duplicating your changes and not. I never made the promise to say "this feature will always keep your HTML valid".

@bergamin
Copy link

@octref I may be a bit shortsighted, but what exactly this feature brings of new that F2 can't perform? I mean... F2 will change the end tag as well, won't it? Or is this just a WYSIWYG-ish refactoring?

If that's the case, shouldn't F2 be changed to show the changes in real time instead of creating a new feature that tries to do something VSCode already can do?

@jsgoupil
Copy link

@octref Give it a shot with Razor. I would expect to work like Razor. No magic.

If you paste any without a space, then you duplicate your paste in the closing tag.
If your paste data begins with a space, then you don't duplicate in the closing tag.

If I press space before I paste, then nothing is duplicated in the closing tag.

@LWChris
Copy link

LWChris commented Feb 12, 2020

Apparently this feature is ill advised to make use of multicursors in the first place. It should probably be re-implemented from scratch, this time as "When editing an HTML tag, copy whatever is the first word of the opening tag to the closing tag, no matter what part of and how the opening tag was edited".

@octref octref modified the milestones: Backlog, March 2020 Mar 18, 2020
@octref
Copy link
Contributor

octref commented Mar 18, 2020

In tomorrow's insiders html.mirrorCursorOnMatchingTag is replaced with editor.renameOnType. The new implementation fixes this issue. You can find more details here: #88424 (comment)

@octref octref closed this as completed Mar 18, 2020
@gregvanl gregvanl added the verified Verification succeeded label Apr 2, 2020
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug editor-synced-region Issues related to synced region functionality in editor html HTML support issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests