8000 document.createElement returns an HTMLElement by trusktr · Pull Request #7136 · mdn/content · GitHub
[go: up one dir, main page]
Skip to content

document.createElement returns an HTMLElement #7136

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
Closed
wants to merge 1 commit into from
Closed

document.createElement returns an HTMLElement #7136

wants to merge 1 commit into from

Conversation

trusktr
Copy link
Contributor
@trusktr trusktr commented Jul 21, 2021

What was wrong/why is this fix needed? (quick summary only)

document.createElement more accurately returns an HTMLElement instances. document.createElementNS is needed (unfortunately) for non-HTMLElement types, like SVGElement.

The built-in type definition in TypeScript also returns HTMLElement.

Issue number (if there is an associated issue)

Anything else that could help us review it

@trusktr trusktr requested a review from a team as a code owner July 21, 2021 15:59
@trusktr trusktr requested review from jpmedley and removed request for a team July 21, 2021 15:59
@github-actions
Copy link
Contributor

Preview URLs

Flaws

URL: /en-US/docs/Web/API/Document/createElement
Title: Document.createElement()
on GitHub
Flaw count: 2

  • macros:
    • /en-us/docs/web/api/parentnode (url: /en-US/docs/Web/API/ParentNode) does not exist
    • /en-us/docs/web/api/geometryutils (url: /en-US/docs/Web/API/GeometryUtils) does not exist

External URLs

URL: /en-US/docs/Web/API/Document/createElement
Title: Document.createElement()
on GitHub

No new external URLs

@jpmedley
Copy link
Collaborator

@Rumyra,

Chrome still returns Element. (It's probably out of date with the spec.) I'm not sure how to handle this, but at the very least, we'll need to add notes to the BCD indicating when various browsers changed.

@Rumyra
Copy link
Collaborator
Rumyra commented Jul 22, 2021

Thanks @jpmedley - a quick sanity check and FF returns HTMLElement. The question is, how do we find out when browsers updated this? Maybe we add a note to BCD explaining that Chrome returns Element - then we can update that once Chrome has updated

@jpmedley jpmedley mentioned this pull request Jul 22, 2021
Closed
@jpmedley
Copy link
Collaborator
jpmedley commented Jul 22, 2021 •
edited
Loading

@Rumyra Here's the PR for that.

@foolip
Copy link
Contributor
foolip commented Jul 23, 2021

document.createElement() doesn't always return an HTMLElement instance. The spec for this is here:
https://dom.spec.whatwg.org/#dom-document-createelement

The return type in Web IDL is Element, and what is returned depends on the document's type.

Here's a snipped where the returned Element wouldn't be a HTMLElement:

var doc = document.implementation.createDocument(null, null, null); // or new Document()
var el = doc.createElement('bla');
// Object.getPrototypeOf(el) is Element.prototype

I believe this is ancient behavior.

foolip
foolip reviewed Jul 23, 2021
View reviewed changes
files/en-us/web/api/document/createelement/index.html
@@ -30,7 +30,7 @@ <h3 id="Parameters">Parameters</h3>

<h3 id="Return_value">Return value</h3>

<p>The new {{domxref("Element")}}.</p>
<p>The new {{domxref("HTMLElement")}}.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though the return type is accurately described as Element, it would be useful to document when an HTMLElement is returned, and that this is the case that web developers will see 99.9% of the time.

@jpmedley
Copy link
Collaborator

@foolip Good catch. This is something we should document completely.

@trusktr
Copy link
Contributor Author
trusktr commented Aug 4, 2021

@foolip Interesting. So what the spec says is actually not what practically all browser developers experience. They don't deal with different types of "documents", only window.document documents from with Windows's document properties, which are all one type of Document.

It seems the spec covers cases for other engines that aren't Firefox, Chrome, Safari, or Edge (which ones?). This is so much the case, that even the official TypeScript type definition returns HTMLElement:

https://github.com/microsoft/TypeScript/blob/03dff41c9f2038f66fb358e5c23ebd7271145978/lib/lib.dom.d.ts#L4464

To follow the spec? Or to follow what we all see in practice? That is the question. (It seems the latter is more useful).

@jpmedley
Copy link
Collaborator
jpmedley commented Aug 5, 2021

This is a reference for web developers explaining what the web does, not a reference for browser implementors explaining what it should do. That's what specs are for.

@jpmedley
Copy link
Collaborator
jpmedley commented Aug 5, 2021

It seems the spec covers cases for other engines that aren't Firefox, Chrome, Safari, or Edge (which ones?).

I'm not sure what would lead you to this conclusion. It's more likely that browser vendors haven't gotten around to updating their implementations for one reason or another. It's also possible that someone was supposed to update the spec and didn't. I would suggest opening an issue on the spec repo itself. The editors of the spec should be able to clear this up.

@foolip
Copy link
Contributor
foolip commented Aug 5, 2021 •
edited
Loading

Maybe there's more than meets the eye here, but AFAICT there isn't anything funny going on, I believe browsers match the spec.

Since HTMLElement inherits from Element it would be correct (but less precise than possible) to say that a method that always returns a HTMLElement object has a return type of Element. In the case of document.createElement the returned object is typically a HTMLElement but it can also be a plain Element.

It seems to me the TypeScript bindings are wrong here. Maybe the return type could be made to depend on the type of this?

@Ryuno-Ki
Copy link
Collaborator
Ryuno-Ki commented Aug 7, 2021

Pinging @sandersn since he reviewed changes on this part of the TypeScript code base

@Rumyra
Copy link
8000 Collaborator
Rumyra commented Aug 8, 2021

I would like to - as soon as I get 5 mins - close this PR in favour for the explanation @foolip has given to be included on this page.

@sandersn
Copy link
sandersn commented Aug 11, 2021 •
edited
Loading

@orta has thought about this particular problem more than I have, but to quote two early comments:

@foolip

it would be useful to document when an HTMLElement is returned, and that this is the case that web developers will see 99.9% of the time.

@trusktr

To follow the spec? Or to follow what we all see in practice? That is the question. (It seems the latter is more useful).

Typescript aims for the 99.9% case here, not spec compatibility. That's why it intentionally returns HTMLElement even though it won't be correct sometimes.

Edit: For Typescript, the penalty for returning a subclass is low: the people who find themselves in the 0.01% case will see extraneous HTMLElement properties in completions when they should see only the Element ones.

@jpmedley
Copy link
Collaborator

Web developers don't come here to learn what's supposed to happen. They come here to learn what actually happens.

@foolip
Copy link
Contributor
foolip commented Aug 11, 2021 •
edited
Loading

@jpmedley this isn't a spec-fiction vs. browser-reality situation, it's an issue of HTMLElement usually being returned except when dealing with non-HTML documents.

With the code snippet in #7136 (comment), what actually happens is that Chrome, Firefox and Safari return an Element instance. This is also what's supposed to happen per spec.

MDN could say that an HTMLElement is returned if the document is an HTMLDocument, which is most common case, and otherwise an Element is returned.`

@orta
Copy link
orta commented Aug 12, 2021

Yeah, just confirming @sandersn - we chose HTMLElement for a few API cases like this which are specifically not conforming to the spec but conform to what happens nearly all the time. We have tried to switch down to Element but use generics to default to HTMLElement ( microsoft/TypeScript-DOM-lib-generator#885 ) but found that the results would really make JS folks lives worse microsoft/TypeScript#4689 (comment)

IMO #7136 (comment) is probably the right take 👍🏻

@Ryuno-Ki
Copy link
Collaborator

Thanks for chiming in and providing the rationale behind the decisions made in TypeScript 🚀

@Rumyra Rumyra mentioned this pull request Aug 16, 2021
Merged
@Rumyra
Copy link
Collaborator
Rumyra commented Aug 16, 2021

Closing this PR in favour of this one #7971 which adds a note based on this discussion 👍

@Rumyra Rumyra closed this Aug 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2022
@trusktr trusktr deleted the patch-3 branch January 5, 2024 08:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@foolip foolip foolip left review comments

@jpmedley jpmedley Awaiting requested review from jpmedley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@trusktr @jpmedley @Rumyra @foolip @Ryuno-Ki @sandersn @orta
0