-
Notifications
You must be signed in to change notification settings - Fork 23k
Fix content issues #41236
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
Fix content issues #41236
Conversation
Preview URLs (7 pages)
Flaws (8)Note! 5 documents with no flaws that don't need to be listed. 🎉 URL:
URL:
External URLs (1)URL:
(comment last updated: 2025-09-22 14:05:13) |
| ### `text` vs `textContent` vs `innerText` | ||
|
|
||
| The `text` and {{domxref("HTMLScriptElement.textContent", "textContent")}} properties of `HTMLScriptElement` are equivalent: both can be set with a string or a `TrustedScript` type and both return a string representing the content of the script element. | ||
| The `text` and {{domxref("Node.textContent", "textContent")}} properties of `HTMLScriptElement` are equivalent: both can be set with a string or a `TrustedScript` type and both return a string representing the content of the script element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at least some of these changes are wrong, cc @hamishwillee
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, yes none of these changes should be made in this document.
@Josh-Cena The existing docs are correct - the properties have been added to HTMLScriptElement as well. They should be coming through from BCD soon. I still have to add docs for textContent and innerText - they are different than in the parent interfaces in that they can be assigned (and may required) TrustedTypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, wasn't aware of it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to ask for care with changes like this. Typoes are usually clear-cut, but this isn't, and it was just chance that I happened to see the PR when I had been involved in the review last week. It would be really easy for this to slip in otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not just a typo fix PR; I send PRs that fix broken links and broken code twice every week. This is a broken link and I wasn't aware that it's supposed to exist, because there's no BCD (had there been, it would be filtered out by virtue of being in web-docs-backlog).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not just a typo fix PR
I do understand that, that's why I'm asking for care. It's easy for cleanup PRs to introduce errors in the docs, because often the author and the reviewer weren't involved in the original, so don't have the context.
In this case it's obvious from the next sentence that the change doesn't make sense:
The text and textContent properties of HTMLScriptElement are equivalent: both can be set with a string or a TrustedScript type and both return a string representing the content of the script element. The main difference is that textContent is also defined on Node and can be used with other elements to set their content with a string.
...but when a reviewer is looking at a cleanup PR that spans multiple different bits of the docs, that they haven't seen before, it's really easy for a busy reviewer to semi-rubberstamp it: look at a broken link, see that the PR fixes it, approve the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing and undoing the trusted types change
(FWIW I would have done the same thing under time pressure. It's really only a problem because you are so trusted that generally the reviewer won't be as careful as they should be).
Approving but not merging in case @wbamberg had anything else outstanding.
wbamberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fixes @Josh-Cena !
|
Thanks. I'll be more careful next time, but sorry I can't really promise it won't happen again😅 |
Fix #41233