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

Emmet css abbreviation not expanded in style block in html file #28039

Closed
jens1o opened this issue Jun 5, 2017 · 27 comments
Closed

Emmet css abbreviation not expanded in style block in html file #28039

jens1o opened this issue Jun 5, 2017 · 27 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug emmet Emmet related issues verified Verification succeeded
Milestone

Comments

@jens1o
Copy link
Contributor

jens1o commented Jun 5, 2017

  • VSCode Version: Code - Insiders 1.13.0-insider (f977399, 2017-06-02T10:54:04.063Z)
  • OS Version: Windows_NT ia32 10.0.15063
  • Extensions:
Extension Author Version
vscode-reveal evilz 0.0.9
php-intellisense felixfbecker 1.3.0
auto-close-tag formulahendry 0.4.2
php-docblocker neilbrayfield 1.2.0
vscode-icons robertohuertasm 7.8.1
Material-theme zhuangtongfa 2.8.4

Steps to Reproduce:
(you can use any css property, this was my first, real-world, live example)

  1. Open up https://docs.emmet.io/cheat-sheet/ and search for d:b
  2. See that emmet usually resolves it to display: block;
  3. Go to Visual Studio Code
  4. Set up your environment:
    image
  5. Type d:b and hit Tab/Enter/...
  6. Get this:
    image
  7. Get worried.
@vscodebot vscodebot bot added the insiders label Jun 5, 2017
@jens1o
Copy link
Contributor Author

jens1o commented Jun 5, 2017

I created this issue, because it is not mentioned here: #27697
Did you missed it?

@kieferrm kieferrm added the emmet Emmet related issues label Jun 5, 2017
@ramya-rao-a
Copy link
Contributor

This works in css/scss/less files, but not inside the style block of a html file.
Good catch.

@ramya-rao-a ramya-rao-a added this to the June 2017 milestone Jun 5, 2017
@ramya-rao-a ramya-rao-a changed the title Emmet only provides html snippets, not css ones Emmet css abbreviation not expanded in style block in html file Jun 5, 2017
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jun 5, 2017

I initially thought that the completion provider for css from emmet does not get called here because the document has the html language mode. And so logged #28047

But interestingly, the built-in css completion provider does get called.

image

@aeschli @jrieken How is that in the above case the built-in css completion provider is called but not the emmet css completion provider?

If the css extension/language server does something special to be able to provide completion items in html file, what do you suggest other extensions who want to do the same do?

@jrieken
Copy link
Member

jrieken commented Jun 6, 2017

How is that in the above case the built-in css completion provider is called but not the emmet css completion provider?

It always depends on the language you are registering for. I'd say that inside style-tags we use css as language and therefore only call completion item providers that have registered for css

@ramya-rao-a
Copy link
Contributor

@jrieken See https://github.com/Microsoft/vscode/blob/ramyar/emmet-fixes/extensions/emmet/src/extension.ts#L54

I have 2 separate completion providers for html vs css
While inside style tag, the one for html gets called and not the css.

should we specify anything else in the DocumentSelector for VS code to use the registered completion provider for embedded languages?

@jrieken
Copy link
Member

jrieken commented Jun 8, 2017

Hm, that looks correct... Would require some debugging in suggest.ts where we merge the results etc. Is this locally reproducible with those steps?

@ramya-rao-a
Copy link
Contributor

@jrieken Yes. The changes are in master, so you can try this locally.

@jrieken
Copy link
Member

jrieken commented Jun 8, 2017

Funky.. If I press d I see completions coming from emmet, also checking what completion provider buckets we have it seems that there are two registered fro CSS.

screen shot 2017-06-08 at 17 48 11

screen shot 2017-06-08 at 17 50 13

Also, when I invoke suggestions inside an empty rule (remove d) I see this

Cannot set property 'kind' of undefined: TypeError: Cannot set property 'kind' of undefined
    at EmmetCompletionItemProviderHtml.getExpandedAbbreviation (/Users/jrieken/Code/vscode/extensions/emmet/out/emmetCompletionProvider.js:52:29)
    at EmmetCompletionItemProviderHtml.provideCompletionItems (/Users/jrieken/Code/vscode/extensions/emmet/out/emmetCompletionProvider.js:18:33)
    at /Users/jrieken/Code/vscode/out/vs/workbench/api/node/extHostLanguageFeatures.js:385:85
    at /Users/jrieken/Code/vscode/out/vs/base/common/async.js:33:24
    at new Promise_ctor (/Users/jrieken/Code/vscode/out/vs/base/common/winjs.base.raw.js:1641:17)
    at Object.asWinJsPromise (/Users/jrieken/Code/vscode/out/vs/base/common/async.js:32:16)
    at SuggestAdapter.provideCompletionItems (/Users/jrieken/Code/vscode/out/vs/workbench/api/node/extHostLanguageFeatures.js:385:28)
    at /Users/jrieken/Code/vscode/out/vs/workbench/api/node/extHostLanguageFeatures.js:732:98
    at ExtHostLanguageFeatures._withAdapter (/Users/jrieken/Code/vscode/out/vs/workbench/api/node/extHostLanguageFeatures.js:573:20)
    at ExtHostLanguageFeatures.$provideCompletionItems (/Users/jrieken/Code/vscode/out/vs/workbench/api/node/extHostLanguageFeatures.js:732:25)

@ramya-rao-a
Copy link
Contributor

If I press d I see completions coming from emmet

Yes, that is from emmet. If you expand the docs you will see the preview for the expansion.

In your case above, the completion item is coming from the html completion provider in emmet i.e for d you get <d>/</d> where as the css completion provider would have given display:block

also checking what completion provider buckets we have it seems that there are two registered fro CSS.

The first one must be coming from the html extension.
The second one is the html completion provider from emmet. ( I can tell the difference from the trigger chars. For css they would have been just :.)
This one should have been the css completion provider from emmet.

Also, when I invoke suggestions inside an empty rule (remove d) I see this

I dont see that in Insiders but I see that in the locally built VS Code. Weird.. how is that? We dont surface errors from extensions in a non dev env?

@jrieken
Copy link
Member

jrieken commented Jun 8, 2017

Ok, now I know what's up. @aeschli Will have all the detail but I remember correctly nested languages aren't surfaced like that. While we have a scope source.css inside the html-file we actually treat all contents as html and call into html-providers which then must do the correct thing.

@ramya-rao-a
Copy link
Contributor

😭

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Jun 8, 2017

And of course, we don't expose the scope to extensions.

You know this is like the 4th time I get to this crossroads of needing scopes in extensions 😢

@aeschli Any plans to expose the nested language info to extensions? Or suggestions to extensions on any workaround?

@jens1o
Copy link
Contributor Author

jens1o commented Jun 8, 2017

Interrupt: Why did you decided to split off and make emmet an internal extension?

@jrieken
Copy link
Member

jrieken commented Jun 8, 2017

Or suggestions to extensions on any workaround?

I think @aeschli has a re-usable html parser (on npm) that you could use to figure out if you are in css or not. And then that info will be reliable, not based tokens (see attached screen shot, the css-scope only starts on the next line)

screen shot 2017-06-08 at 20 39 02

@ramya-rao-a
Copy link
Contributor

@jrieken Thanks, the emmet modules has a parser as well which I am using for other emmet features. Will try that.

@jens1o Do you mean to ask why is it internal extension vs external extension? Or why is it an extension at all?

@jens1o
Copy link
Contributor Author

jens1o commented Jun 9, 2017

@ramya-rao-a internal vs. external, when you have so many problems, why did you decided to switch so fast, even when there are no apis available?

@ramya-rao-a
Copy link
Contributor

Why so fast?

@jens1o Well I'd say not fast enough :) The 2 issues with current emmet integration in VS Code as described here: https://code.visualstudio.com/updates/v1_13#_emmet-abbreviation-expansion-in-suggestion-list are the main reasons to move to the newer model of using the suggestion list for emmet. Over the past year we have had many users face this issue and moving to the suggestion model was the right move.

Why internal extension vs external extension?

Emmet has been a feature shipped with VS Code since the beginning. Making it an external extension would require users to install it explicitly. Therefore, the decision for internal vs external.

Why an extension at all? Why not build it inside VS Code like current emmet?

Emmet features are not core to an editor per say. These are add-ons. And so make sense more as an extension.

Initially, the way emmet library worked, it was not possible to have it as an extension for VS Code. For Atom, say it was possible because it has a different extensibility model, it is hackable at every level. Now, that @sergeche has modularized the core logic of emmet, we can now have emmet expansion coming from completion providers and rest of the emmet features from an extension.

Why switch now? Apis are not ready!!!

Unlike @sergeche or many power users of emmet I don't have the luxury of knowing emmet and all its scenarios in and out. Because I don't use emmet at all in my job. That's why I depend on amazing people like you to dogfood the initial implementation and come up with the corner cases where the feature doesn't work. Thanks to the open source model of things, we can do that :) i.e work together to hash out scenarios

As @sergeche suggested here: #27628 (comment), we can still parse the file in an efficient way to get the info we need to solve both this and the linked issue. So even if scope info or the nested language info is not available as apis, we can still do this.

Worst case scenario, I can easily move the new emmet from the internal extension to VS Code core and make use of the scope info available there.

Either way, by the end of the June milestone, we are going to have a great emmet experience

@jens1o
Copy link
Contributor Author

jens1o commented Jun 9, 2017

Wow, thanks for the detailed answer. I'll continue to work with it(and report odd things), to help you. 😋

@ramya-rao-a
Copy link
Contributor

The fix will be out in the Tuesday's Insiders

@yume-chan
Copy link
Contributor

@ramya-rao-a You only changed the Completion Provider, but not the actions (at least expandAbbreviation action)?

And another problem: You implementation will provide emmet in the start tag of a style element, because you set the syntax to css for this tag.

<style |>

I'm working on a tool for my team with monaco-editor and trying to backport the new emmet vscode extension to monaco (Now the Completion Provider and expandAbbreviation action just work). I had never used emmet before neither, but now I think the implementation will be very complex: you need to handle normal HTML, in style attribute (doesn't need selectors) and in style element (need selectors) very differently.

@sergeche
Copy link

sergeche commented Jun 19, 2017

@ramya-rao-a you have to check if cursor is not inside .open or .close tag token to fix the problem that @CnSimonChan pointed. See https://github.com/emmetio/atom-plugin/blob/master/lib/actions/balance.js#L84 as example of getting inner node range

@ramya-rao-a
Copy link
Contributor

@CnSimonChan I had another task in mind to consolidate the code between the completion provider and the action which I was planning to do today which would cover your concern.

And good catch on the style tag, will add the check on the tag as well. Will re-open this issue until this is fixed. Thanks for reporting!

@sergeche For now I am using https://github.com/Microsoft/vscode/blob/master/extensions/emmet/src/emmetCompletionProvider.ts#L134 for validating the position. I'll take another look at the atom implementation to ensure I have not missed any other cases.

@ramya-rao-a ramya-rao-a reopened this Jun 19, 2017
@ramya-rao-a ramya-rao-a added the bug Issue identified by VS Code Team member as probable bug label Jun 19, 2017
@ramya-rao-a
Copy link
Contributor

@CnSimonChan With 8902a30 you now have the same validations being run and syntax being used by the completion provider and the expand abbreviation action.

  • css inside style element
  • no expansions inside the open tag for style element

You bring up another good point about the style attribute. Logged #29066 for the same.

Yes, things are getting complex in this model where a lot of work has moved from emmet to editor.
I am not sure what it takes for the new emmet functionality to be ported to monaco editor, but I can look into it and get back to you.

@yume-chan
Copy link
Contributor

@ramya-rao-a It works very well now, maybe another improvement will be re-parse the content of <style> tag as css to provide emmet only for rules.

For monaco, I wrote a minimal compatibility layer to bridge monaco's IStandaloneCodeEditor and IModel to vscode's TextEditor and TextDocument , then bridge vscode's CompletionItemProvider to monaco one. I'm not familiar with vscode and I can't find any relative code in the repo so maybe I'm doing something redundant.

I have two more question with this extension:

  1. My npm@5 crashed when restoring packages for this extension, it seems the npm-shrinkwarp.json file caused the crash, I deleted this file then it restores correctly.

  2. Do you have definition files for emmet packages somewhere? I found one in src folder but looks like it's for the old all-in-one version. Or you just work with any?

@ramya-rao-a
Copy link
Contributor

re-parse the content of <style> tag as css

I gave this a thought, but will not be doing this for the current iteration. Logged #29113 and added a help-wanted label on it.

npm@5 crashing

Not sure about this. The shrink-wrap is used during shipping time, so not needed during the development phase. Did you try with a lower npm version?

Definition for emmet modules

Ahhh... This is something I have been needing as well. @sergeche Could you help here ?

@yume-chan
Copy link
Contributor

Did you try with a lower npm version?

No, I have only 5.

Definition for emmet modules

I have noImplicitAny on so I wrote some definition by myself, just covered the need of the extension (although I'm not very clear about the return type of html-matcher and css-parser). It's better if someone good at creating type definition can help, but if not I will post it tomorrow (after 10 hours) and hope it can be useful.

@ramya-rao-a
Copy link
Contributor

Note to verifier:

  • Set emmet.useNewEmmet to true
  • You should get css related emmet suggestions inside a <style> block and no suggestions at all inside the open tag of the <style> tag

@roblourens roblourens added the verified Verification succeeded label Jun 29, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
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 emmet Emmet related issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

7 participants