-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Trusted Types - FF updates and missing entries in TT spec #25839
base: main
Are you sure you want to change the base?
Conversation
"status": { | ||
"experimental": false, | ||
"standard_track": true, | ||
"deprecated": true |
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.
Note, this is set as not experimental and deprecated, because the parent is deprecated (and you can't have both set).
api/HTMLScriptElement.json
Outdated
@@ -417,6 +417,53 @@ | |||
} | |||
} | |||
}, | |||
"innerText": { |
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.
So for this one (and textContent), the properties were around since forever, but inherited. Since this update only applies to the HTMLScriptElement I have added it here as though it were a new property.
Is this OK? I mean it looks like you can only access this from current version, which might be confusing to readers.
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 this will be confusing, yes, because innerText
is also defined and BCD-ified in the parent HTMLElement
: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement#browser_compatibility.
And the innerText
page takes its BCD from there: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText#browser_compatibility so this change won't even show up in that page.
Maybe instead of this, add a subfeature to HTMLElement.innerText, like: "Script elements can accept a TrustedScript
"? Then it will show up as a subfeature in https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText#browser_compatibility .
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.
It depends on how important strict following of IDL is for BCD. I like the idea though - changed in df3f74f
@@ -14,7 +14,14 @@ | |||
"chrome_android": "mirror", | |||
"edge": "mirror", | |||
"firefox": { | |||
"version_added": false, | |||
"version_added": "135", |
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.
Not 100% sure when this became useful. The whole API was added behind pref in 125 and no implementation. I've put 135 because it does not harm and we're starting to get where tests can be useful.
api/Element.json
Outdated
@@ -6263,6 +6263,53 @@ | |||
"standard_track": true, | |||
"deprecated": false | |||
} | |||
}, | |||
"accepts_TrustedHtml": { |
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.
As for other cases, you can set the property with this, but if you were to get it back you'd get a string.
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.
Interesting, I wonder if there would be any desire to formalize BCD subfeatures for property getters and setters.
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.
That would be good. For now I have tried to be consistent.
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.
@Elchi3 asked me for a "domain review" of this PR :).
api/Document.json
Outdated
}, | ||
"text_param_accepts_TrustedHTML": { | ||
"__compat": { | ||
"description": "`text` parameter accepts `TrustedHTML` type", |
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.
The page calls the parameter markup
so this will be confusing. Also IDK if anyone cares but I think strictly, the parameter doesn't "accept" anything - it is the function that accepts things. The parameter just is something.
Since this method only accepts one parameter we might not have to name the parameter and say "Accepts a TrustedHTML
instance" or something like that.
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.
Yes, changed to:
"accepts_TrustedHTML": {
"__compat": {
"description": "Accepts `TrustedHTML` instances",
This takes an arbitrary number of either/both strings or trusted HTML instances.
FYI I also changed this in my docs PR to text, but will revert, as markup is a better term.
api/Element.json
Outdated
@@ -6263,6 +6263,53 @@ | |||
"standard_track": true, | |||
"deprecated": false | |||
} | |||
}, | |||
"accepts_TrustedHtml": { |
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.
Interesting, I wonder if there would be any desire to formalize BCD subfeatures for property getters and setters.
api/HTMLScriptElement.json
Outdated
@@ -417,6 +417,53 @@ | |||
} | |||
} | |||
}, | |||
"innerText": { |
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 this will be confusing, yes, because innerText
is also defined and BCD-ified in the parent HTMLElement
: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement#browser_compatibility.
And the innerText
page takes its BCD from there: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText#browser_compatibility so this change won't even show up in that page.
Maybe instead of this, add a subfeature to HTMLElement.innerText, like: "Script elements can accept a TrustedScript
"? Then it will show up as a subfeature in https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/innerText#browser_compatibility .
"accepts_TrustedHTML": { | ||
"__compat": { | ||
"description": "Accepts `TrustedHTML` instances", |
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.
@wbamberg FWIW I think technically the right way to do this according to the schema is as shown, noting that markup will be used in the docs.
"accepts_TrustedHTML": { | |
"__compat": { | |
"description": "Accepts `TrustedHTML` instances", | |
"markup_param_accepts_TrustedHTML": { | |
"__compat": { | |
"description": "Accepts `TrustedHTML` instances", |
But as you say, these take just one parameter so it doesn't matter (actually they take multiple parameters of string and/or TrustedHTML, represented as ...text
in the spec).
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.
Agree that as these only support a single parameter, there is no need to over-complicate this.
@wbamberg Thanks. Accepted all your suggestions - ready for another look |
"accepts_TrustedHTML": { | ||
"__compat": { | ||
"description": "Accepts `TrustedHTML` instances", |
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.
Agree that as these only support a single parameter, there is no need to over-complicate this.
"accepts_TrustedHTML": { | ||
"__compat": { | ||
"description": "Accepts `TrustedHTML` instances", | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Document/write", |
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.
Let's only add mdn_url
if it describes this feature (here: accepting TrustedHTML).
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Document/write", |
"__compat": { | ||
"description": "Accepts `TrustedHTML` instances", | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Document/write", | ||
"spec_url": "https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-write", |
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.
Let's link closer to where TrustedHTML is actually mentioned:
"spec_url": "https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-write", | |
"spec_url": [ | |
"https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-write-steps", | |
"https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#:~:text=if%20value%20is%20a%20trustedhtml%20object" | |
], |
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Document/writeln", | ||
"spec_url": "https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-writeln", |
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.
dito (see above)
], | ||
"support": { | ||
"chrome": { | ||
"version_added": "83" |
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.
The mentioned WPT tests (https://wpt.live/trusted-types/Document-write.html) fail for me in Chrome 83 on BrowserStack Live, and only pass from Chrome 86.
"version_added": "83" | |
"version_added": "86" |
], | ||
"support": { | ||
"chrome": { | ||
"version_added": "83" |
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.
dito (etc)
"code_param_accepts_trustedScript": { | ||
"__compat": { | ||
"description": "`code` parameter can be set with a `TrustedScript` instance", | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Window/setInterval", |
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.
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Window/setInterval", |
"code_param_accepts_trustedScript": { | ||
"__compat": { | ||
"description": "`code` parameter can be set with a `TrustedScript` instance", | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Window/setTimeout", |
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.
"mdn_url": "https://developer.mozilla.org/docs/Web/API/Window/setTimeout", |
etc
FF has added more support for trusted types behind a flag. This adds those updates.
Also it adds some missing entries, primarily for the methods and properties that will now accept trusted types. Some of this is a bit weird so I've added comments inline explaining how/why I did stuff.
Document.write()
andwriteln()
accept {{domxref("TrustedHTML")}} objects as parameters, in addition to strings. https://bugzilla.mozilla.org/show_bug.cgi?id=1906301 - I tested version support for Chrome using https://wpt.live/trusted-types/Document-write.htmlHTMLScriptElement.text
,innerText
, andtextContent
properties now acceptTrustedScript
as a setter, whileHTMLScriptElement.src
acceptsTrustedScriptURL
in https://bugzilla.mozilla.org/show_bug.cgi?id=1905706innerText
andtextContext
were formerly inherited from HTMLElement and Node respectively.setTimeout
andsetInterval
taking a trustedtype - as a proxy for code in a script. But it isn't in the spec other than broadly referred to. I have added info though, because it is a compat issue - it should be listed in https://w3c.github.io/trusted-types/dist/spec/#dom-xss-injection-sinkstrustedTypes
- you need to have that for anything to work. I have added this as though added in FF135. It might have appeared earlier, but not useful - I'm OK with it since all of that will change when this gets released.Element.innerHTML
and the shadow versions arent speced with alternative IDL saying it can take a trusted type, but there are examples which show that, and wpt tests, and they are mentioned in https://w3c.github.io/trusted-types/dist/spec/#dom-xss-injection-sinks - which I have used for the spec url.The spec mentions some other cases such as Element.outerHTML setters, and functions that create a new same-origin Document with caller-controlled markup like parseFromString(). Have not added that, because I haven't tested it.