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

[js] getComputedStyle argument type #9085

Open
mockey opened this issue Jan 15, 2020 · 4 comments
Open

[js] getComputedStyle argument type #9085

mockey opened this issue Jan 15, 2020 · 4 comments
Assignees
Labels
bug feature-html-externs HTML externs for platform-js
Milestone

Comments

@mockey
Copy link
Contributor

mockey commented Jan 15, 2020

I guess js.html.Window.getCompiutedStyle should better use DOMElement (a.k.a. Element) instead of Element (a.k.a .HTMLElement) for its argument type, shouldn't it?

@mockey mockey changed the title [JS] getComputedStyle argument type [js] getComputedStyle argument type Jan 15, 2020
@RealyUniqueName
Copy link
Member

Yeah. According to MDN getComputedStyle accepts an argument of JS's Element, which is called DOMElement in our externs for some reason.
And our Element is actually HTMLElement in JS.
I guess there may be other errors in our externs caused by this mess.

@RealyUniqueName RealyUniqueName added feature-html-externs HTML externs for platform-js bug labels Jan 15, 2020
@RealyUniqueName RealyUniqueName added this to the Release 4.1 milestone Jan 15, 2020
@haxiomic
Copy link
Member

haxiomic commented Jan 15, 2020

Follows on from the discussion on DOMElement / HTMLElement -> Element differences and remapping
#4081

I think the original remap of Element -> HTMLElement was done because many methods technically return Elements but practically return HTMLElements

It would be great to see if we can straighten this out for 4.1. My current thinking on how to improve the externs going forward is to use the TypeScript extern generator as a base for a rewrite, since TS put a lot of effort into getting the html externs perfect

@mockey
Copy link
Contributor Author

mockey commented Jan 15, 2020

I think the original remap of Element -> HTMLElement was done because many methods technically return Elements but practically return HTMLElements

An HTMLElement is always an Element as well (technically and practically). Why exclude SVGElements that way?

I always thought that this weird remap was mainly done because of the js.html namespace. The original names all have the HTML prefix like HTMLDivElement which sort of translates to html.DivElement. Of course this is not the case for a lot of other classes under js.html (because they are not HTML* elements). And there are classes like js.html.HTMLDocumentand there is js.html.svg.* for SVGElements and there are subfolders like js.html.midi where the classes keep their prefix, e.g. MIDIAccess and js.html.idb where they don't, e.g. IDBCursor...
All in all it's a mess, but the worst is that Element -> js.html.DOMElement and HTMLElement -> js.html.Element remap.

My suggestion would be:

  • use another namespace like js.dom or js.web (according to MDN it's a "Web API")
  • stick to the original "Web API" names (as weird as they might be, like HTMLHtmlElement)

@haxiomic
Copy link
Member

haxiomic commented Jan 15, 2020

If it would go down well with the community I'd love to do as you suggest @mockey and rebuild the externs without any renaming. I like your suggestion of moving to js.dom or js.web and deprecating js.html. Definitely something on my mind for the next pass at externs.

Big change of course because so many projects rely on these but I guess we can always add --remap js.html js.web to extraParams in a compat repo

To ball-park, the timeline for something like this is out at around 6 months because I'd want to do it as part of the html-externs rewrite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature-html-externs HTML externs for platform-js
Projects
None yet
Development

No branches or pull requests

4 participants