Skip to content

Conversation

@atscott
Copy link
Contributor

@atscott atscott commented Aug 31, 2021

Native DOM events were previously not included in the completions in Ivy
because the dom schema registry would filter out events completely. This
change updates the registry to include events in the private
element->property map and excludes events from lookups outside of the
new allKnownEventsOfElement function.

fixes angular/vscode-ng-language-service#1479

Reviewer notes: Autocompletion in the VE language service did include native events so this addresses a regression from previous behavior.

case ATTR.IDENT_EVENT:
// event binding via on- or ()
results.push(...eventNames(elem.name), ...ngAttrs.outputs);

export function eventNames(elementName: string): string[] {
return SchemaInformation.instance.eventsOf(elementName);
}

eventsOf(elementName: string): string[] {
const elementType = this.schema[elementName.toLowerCase()] || {};
return Object.keys(elementType).filter(property => elementType[property] === EVENT);
}

@atscott atscott added action: review The PR is still awaiting reviews from at least one requested reviewer area: language-service Issues related to Angular's VS Code language service target: patch This PR is targeted for the next patch release labels Aug 31, 2021
@atscott atscott requested a review from alxhub August 31, 2021 20:45
@ngbot ngbot bot modified the milestone: Backlog Aug 31, 2021
@google-cla google-cla bot added the cla: yes label Aug 31, 2021
@pullapprove pullapprove bot requested review from IgorMinar and jelbourn August 31, 2021 20:45
@atscott atscott force-pushed the autocompletenative branch 3 times, most recently from 0728079 to f13357a Compare September 2, 2021 17:21
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM. As far as I can tell this doesn't change the security properties of the schema validation, so I'm ok with not blocking this on the ISE review, but I do want to give them a heads up.

@bjarkler - fyi: we are making a change to the DOM security schema that looks a bit dangerous on the surface, but in reality it doesn't change anything significant, and doesn't have any security impact.

Reviewed-for: fw-security

@bjarkler
Copy link
Contributor

My only security concern is that the current change makes it a little bit easier for a future change to expose the event properties by accident. You could fix that by moving the events into a separate event schema table so that you wouldn't have to apply the current filtering in all the other getters. But in any case, this LGTM.

@IgorMinar
Copy link
Contributor

thanks @bjarkler! I agree with your suggestion, @atscott could you please give it a shot? thank you!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: fw-security

Native DOM events were previously not included in the completions
because the dom schema registry would filter out events completely. This
change updates the registry to include events in the private
element->property map and excludes events from lookups outside of the
new `allKnownEventsOfElement` function.

fixes angular/vscode-ng-language-service#1479
@atscott
Copy link
Contributor Author

atscott commented Sep 24, 2021

presubmit

@atscott atscott added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 24, 2021
@atscott
Copy link
Contributor Author

atscott commented Sep 24, 2021

merge assistance: All required reviews are approved. Pullapprove seems a bit stuck

alxhub pushed a commit that referenced this pull request Sep 27, 2021
Native DOM events were previously not included in the completions
because the dom schema registry would filter out events completely. This
change updates the registry to include events in the private
element->property map and excludes events from lookups outside of the
new `allKnownEventsOfElement` function.

fixes angular/vscode-ng-language-service#1479

PR Close #43299
@alxhub alxhub closed this in 3e37e89 Sep 27, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: language-service Issues related to Angular's VS Code language service cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Native events not listed in completions for output bindings in templates: <div (|)

5 participants