Skip to content

Add focusin/focusout to GlobalEventHandlers #696

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 4, 2019
Merged

Add focusin/focusout to GlobalEventHandlers #696

merged 5 commits into from
Jun 4, 2019

Conversation

Fabianopb
Copy link
Contributor

@Fabianopb Fabianopb commented Apr 18, 2019 •
edited
Loading

According to microsoft/TypeScript#30716 focusin/focusout are missing from GlobalEventHandlersEventMap.

I have added onfocusin and onfocusout to GlobalEventHandlers and the events focusin and focusout, which were supposed to be mapped, but they haven't shown up in GlobalEventHandlersEventMap, so I could use a small help!

@saschanaz, I've searched in the history and I noticed that in #674 you've managed to add deviceorientationabsolute to WindowEventMap. How did you achieve that? I think I'm missing some detail here...

Also, I noticed that onfocus has a comment explaining its purpose, would it make sense to include comments also for onfocusin and onfocusout?

Fixes microsoft/TypeScript#30716

@saschanaz
Copy link
Contributor

onfocusin and onfocusout shouldn't be added as those IDL attributes are not implemented in any browsers nor included in any spec.

So those events are without IDL attributes, and currently we only supports events handlers with corresponding attributes, so this goes tricky...

We have to fix the code so that we can add attribute-less events including DOMContentLoaded.

@Fabianopb
Copy link
Contributor Author

Good to know @saschanaz , I was not sure, the references around onfocusin and onfocusout are a bit uncertain, even though MDN points them as event handler properties, you can't really find any spec.

Thanks for the clarification, so apparently the work is around these lines of the emitter. I'll revert the added properties and try to work something out in the emitter.

@Fabianopb
Copy link
Contributor Author

@saschanaz I have now updated the PR to allow adding "attributeless-events" to interfaces, which are mapped to the -EventMaps, but we have to still figure out the IDL checks (reason why tests are failing). Any tips for that? Also, why do tests pass locally but break in the CI?

@fregante fregante mentioned this pull request Apr 19, 2019
Merged
@saschanaz
Copy link
Contributor

The master branch is currently failing (probably by TypeScript nightly behavior change). Try fixing line 200 of widlprocess.ts to name: operation.name || undefined.

@Fabianopb
Copy link
Contributor Author

Thanks @saschanaz, Ci fixed! I'll remove the "WIP" from the title, this PR is good to be reviewed!

@Fabianopb Fabianopb changed the title WIP: Add focusin/focusout to GlobalEventHandlers Add focusin/focusout to GlobalEventHandlers Apr 20, 2019
saschanaz
saschanaz approved these changes Apr 20, 2019
View reviewed changes
Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

I once thought maybe we could fix iNameToEhList instead so that it would map event-to-property rather than current property-to-event, and found it's more complicated because some events are not in GlobalEventHandlers bun is rather spread into specific element interfaces.

@Fabianopb
Copy link
Contributor Author

Good point @saschanaz, and thanks for the approval! Ping @RyanCavanaugh as this is probably ready to be merged.

jeffryang24 added a commit to jeffryang24/TSJS-lib-generator that referenced this pull request May 12, 2019
@jeffryang24
fix(build): fix build error
a75062e 
from microsoft#696
@Fabianopb
Copy link
Contributor Author

Keeping the PR alive, ping perhaps @sandersn ? :)

@sandersn sandersn self-assigned this Jun 4, 2019
@sandersn sandersn merged commit b08ca62 into microsoft:master Jun 4, 2019
sandersn pushed a commit that referenced this pull request Jun 4, 2019
@sandersn
Add generic support to Element.closest() (#697)
7851d16 
* Add generic support to `Element.closest()`

* Update widlprocess.ts

#696 (comment)

* Add generated files
@Fabianopb Fabianopb deleted the focusin-focusout branch June 5, 2019 06:08
This was referenced Aug 21, 2019
Closed
Merged
@andrewbranch andrewbranch mentioned this pull request Sep 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn sandersn approved these changes

+1 more reviewer

@saschanaz saschanaz saschanaz approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@sandersn sandersn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

focusin/focusout events missing in lib.dom.d.ts
3 participants
@Fabianopb @saschanaz @sandersn