Fix for Emmet's wrap with abbreviation inserting extra spaces #43345

gushuro · 2018-02-09T20:00:02Z

Fix for #29898
Now, when wrapping text with a multiline abbreviation such as ul>li>a, no extra spaces are added.

Added a unit test to keep track of this issue.

ramya-rao-a · 2018-02-12T19:59:55Z

extensions/emmet/src/abbreviationActions.ts

+				if (expandOptions['text'][0].indexOf('\n') === -1) {
+					expandOptions['text'][0] = '\n\t' + expandOptions['text'][0] + '\n';
+				}
+				expandedText = helper.expandAbbreviation(input.abbreviation, expandOptions);


Why expand again? Why not just add the \n\t and t around $TM_SELECTED_TEXT in the expanded text?

ramya-rao-a · 2018-02-12T20:00:31Z

extensions/emmet/src/abbreviationActions.ts

@@ -436,14 +443,20 @@ function expandAbbr(input: ExpandAbbreviationInput): string | undefined {
 		let expandedText = helper.expandAbbreviation(input.abbreviation, expandOptions);

 		if (input.textToWrap) {
+			// Fetch innermost element in the expanded abbreviation


Shouldnt we do the search for block elements only if textToWrap was a single line?

if (input.textToWrap.length === 1 && input.rangeToReplace.isSingleLine)

ramya-rao-a · 2018-02-12T20:04:20Z

extensions/emmet/src/abbreviationActions.ts

+			// Fetch innermost element in the expanded abbreviation
+			let wrappingEnd = expandedText.substring(expandedText.indexOf('$TM_SELECTED_TEXT'));
+			let tagName = wrappingEnd.substring(wrappingEnd.indexOf('/') + 1, wrappingEnd.indexOf('>'));
+


what do you think about using regular expressions here instead?

ramya-rao-a · 2018-02-14T18:49:44Z

Instead of expanding the abbreviation to figure out if the wrapping element is inline or not, we can parse the abbreviation to get a tree and use it to figure out the same. And then, the same tree can be used by the expand module

ramya-rao-a · 2018-02-14T23:20:14Z

extensions/emmet/src/abbreviationActions.ts

+			let parsedAbbr = helper.parseAbbreviation(input.abbreviation, expandOptions);
+			if (input.rangeToReplace.isSingleLine) {
+
+				// Fetch innermost element in the expanded abbreviation


"parsed" not "expanded"

ramya-rao-a · 2018-02-14T23:21:08Z

extensions/emmet/src/abbreviationActions.ts

+					tagName = lastNode.name;
+				}
+
+				// If wrapping with a block element, insert newline and expand again


These comments need to be updated as we are not expanding "again" anymore

You're right

ramya-rao-a · 2018-02-14T23:22:43Z

extensions/emmet/src/abbreviationActions.ts

+
+				// If wrapping with a block element, insert newline and expand again
+				// We need to expand again because emmet module can't know if the text being wrapped is multiline.
+				if (inlineElements.indexOf(tagName) === -1 && input.textToWrap.length === 1) {


We can move the input.textToWrap.length === 1 check up to line 449

ramya-rao-a · 2018-02-14T23:23:38Z

extensions/emmet/src/abbreviationActions.ts

+
+				// Fetch innermost element in the expanded abbreviation
+				let lastNode = parsedAbbr;
+				let tagName = parsedAbbr.name || '';


we can move this after finding the innermost node i.e after the while loop. This way we can avoid setting the tagName multiple times inside the loop

ramya-rao-a · 2018-02-14T23:24:22Z

extensions/emmet/src/abbreviationActions.ts

+			if (input.rangeToReplace.isSingleLine) {
+
+				// Fetch innermost element in the expanded abbreviation
+				let lastNode = parsedAbbr;


lets rename this to innerMostNode. It will then match with your comment and is also more intuitive than lastNode

ramya-rao-a · 2018-02-14T23:24:59Z

extensions/emmet/src/abbreviationActions.ts

+				// Fetch innermost element in the expanded abbreviation
+				let lastNode = parsedAbbr;
+				let tagName = parsedAbbr.name || '';
+				while (lastNode.children && lastNode.children.length > 0) {


what if parsedAbbr was null or undefined?

It'll throw an error when trying to obtain its property 'name', and we'll catch that. Do you think we should treat it differently?

ramya-rao-a · 2018-02-14T23:25:41Z

extensions/emmet/src/test/wrapWithAbbreviation.test.ts

@@ -197,6 +197,30 @@ suite('Tests for Wrap with Abbreviations', () => {
 		});
 	});

+	test('Wrap with multiline abbreviation doesnt add extra spaces', () => {


Add the bug id here in the test name

…microsoft#43345)" This reverts commit 7420a04.

* Revert "Fix for Emmet's wrap with abbreviation inserting extra spaces (#43345)" This reverts commit 7420a04. * Adding ability for emmet to wrap with abbreviation in real time. Currently it's only working for single cursor. * Fixes to wrap in real time: - Removed flickering when typing abbreviation - Removed tabstops - Fixed bug when wrapping multiline text * Fixes to a few issues. - Added checks for not reverting previews more times than needed, that was causing extra text to be deleted. - Fixed issue when wrapping nodes with multiple level of indentation. - Removed all the undo commands. Now all the logic of going back to the original state is handled by revertPreview. * Ammend for previous revert * Reapplying reverted commit, fixing the bug for this branch's version Refactoring some of the code, now a single object contains the current and original ranges, as well as the original content to wrap * Adding multicursor support * Renaming, refactoring and other stuff * More refactorings * More renaming and refactoring * Replacing placeholders when previewing, simplifying the extracting of preceeding whitespace, added a check for validity of expandedtext on each selection. * More refactoring * Adding a comment. * Readding test removed by mistake. * Refactoring * Carefully reverting changes in yarn.lock * carefully but right

brezanac · 2018-05-04T00:42:53Z

There needs to be a setting for VSCode which will allow to completely disable automatic indentation based on identifying what type of element the wrapping tag is.

For example, current implementation for wrapping a selected piece of text with any block level element like p or div will produce the following result

<p>    
    Selected text
</p>

which might be technically correct but it's ugly and completely unnecessary in terms of wasting lines and space.

I basically need VSCode to stop overthinking everything and just wrap code with a tag of my selection without automatically indenting it.

ramya-rao-a · 2018-05-07T04:54:44Z

I basically need VSCode to stop overthinking everything and just wrap code with a tag of my selection without automatically indenting it.

@brezanac, That might work well for your case, but doesn't really help when wrapping multi-line text or better yet, multi line html

If you do have a solution that works for both cases, please do send a PR. We would appreciate it.

gushuro requested a review from ramya-rao-a February 9, 2018 20:00

octref assigned ramya-rao-a Feb 12, 2018

ramya-rao-a suggested changes Feb 12, 2018

View reviewed changes

Gustavo Martin Hurovich added 6 commits February 14, 2018 15:10

Fixing trailing spaces in expansion

edf7f31

Emmet - Fixing extra spaces when wrapping with abbreviation

4e42216

Avoid fetching tag if there's no need to ezpand again

25735a6

Replacing several 'indexof's with regexp

3c80088

Making regexp for tagname stricter. Removing unnecessary check.

5b95c7a

Removing extra call to expand.

797b9ba

gushuro force-pushed the extraspaces branch from 62fba89 to 797b9ba Compare February 14, 2018 23:14

ramya-rao-a suggested changes Feb 14, 2018

View reviewed changes

Gustavo Martin Hurovich added 2 commits February 14, 2018 15:53

tiny changes

ceda77d

tinitiny changes

c310b53

ramya-rao-a approved these changes Feb 15, 2018

View reviewed changes

ramya-rao-a merged commit 7420a04 into microsoft:master Feb 15, 2018

ramya-rao-a mentioned this pull request Feb 15, 2018

Emmet wrap with abbreviation inserts extra spaces before content when abbr is inline element #29898

Closed

gushuro deleted the extraspaces branch February 28, 2018 18:34

gushuro pushed a commit to gushuro/vscode that referenced this pull request Mar 5, 2018

Revert "Fix for Emmet's wrap with abbreviation inserting extra spaces (…

39e6a2b

…microsoft#43345)" This reverts commit 7420a04.

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Emmet's wrap with abbreviation inserting extra spaces #43345

Fix for Emmet's wrap with abbreviation inserting extra spaces #43345

gushuro commented Feb 9, 2018

ramya-rao-a Feb 12, 2018

ramya-rao-a Feb 12, 2018

ramya-rao-a Feb 12, 2018

ramya-rao-a commented Feb 14, 2018

ramya-rao-a Feb 14, 2018

ramya-rao-a Feb 14, 2018

gushuro Feb 14, 2018

ramya-rao-a Feb 14, 2018

ramya-rao-a Feb 14, 2018

ramya-rao-a Feb 14, 2018

ramya-rao-a Feb 14, 2018

gushuro Feb 14, 2018

ramya-rao-a Feb 14, 2018

brezanac commented May 4, 2018

ramya-rao-a commented May 7, 2018

Fix for Emmet's wrap with abbreviation inserting extra spaces #43345

Fix for Emmet's wrap with abbreviation inserting extra spaces #43345

Conversation

gushuro commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brezanac commented May 4, 2018

ramya-rao-a commented May 7, 2018