Draft integration with Trusted Types, take 2. by koto · Pull Request #1247 · whatwg/dom (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation31 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

koto

@koto koto mentioned this pull request

Jan 23, 2024

annevk

Choose a reason for hiding this comment

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

I see it's still a draft, but I thought I'd post some initial feedback.

@@ -6354,6 +6355,10 @@ given a document, localName, namespace, and opt
attribute attribute to value, run these steps:

  1. Set value to the result of calling Get Trusted Types-compliant attribute

    		2.

    Choose a reason for hiding this comment

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

    Nit: indentation is wrong. And DOM doesn't do newlines in phrasing-level elements (i.e., wrap before <a>).

    Choose a reason for hiding this comment

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

    Question: this ordering means that if value is changed by TT the mutation record reflects that. Does that have test coverage?

    Choose a reason for hiding this comment

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

    Fixed the formatting, thanks for pointers. You're correct re: MutationObserver.

    @mbrodesser-Igalia, we need to add a MutationObserver test (https://jsfiddle.net/014ze36t/2/) to WPT.

    dom.bs Outdated Show resolved Hide resolved

    dom.bs Outdated Show resolved Hide resolved

    dom.bs Outdated Show resolved Hide resolved

  2. Handle attribute changes for attribute with element, null, and

    		3.
    attribute's value.

  3. Return.

    		4.

    Choose a reason for hiding this comment

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

    I don't understand why the changes here (which are for setAttributeNS() and prolly some other callers) are much more elaborate than they are for setAttribute().

    Why can't "append an attribute" handle this in the way it already does per the changes above?

    Choose a reason for hiding this comment

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

    Append might be too late, at that point Attr value (a string) is set, so the checks bubbled up to the calling algorithms. Check the current version, I think I missed one callsite in the one you reviewed. Now it should be OK, with the intentional omission of clone.

    Choose a reason for hiding this comment

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

    annevk

    dom.bs Outdated Show resolved Hide resolved

    dom.bs Outdated Show resolved Hide resolved

    dom.bs Outdated Show resolved Hide resolved

    Comment on lines 6518 to 6530

    1. Set attribute to a new attribute whose namespace is

      		2.
      namespace, namespace prefix is prefix,
      local name is localName and node document is
      element's node document.

    2. Validate and set attribute value value for attribute with

      		3.
      element.

    3. Append attribute to element.

      		4.

    4. Return.

      		5.

      Choose a reason for hiding this comment

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

      This seems wrong. This should be behind the "If attribute is null" check above, presumably?

      Choose a reason for hiding this comment

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

      However, I'm also not sure why this cannot be part of "append an attribute" which would give us some deduplication. If you do the validation before the attribute is appended to the element, it seems fine? What am I missing?

      Choose a reason for hiding this comment

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

      In particular because in these cases we end up creating a new attribute and so if something throws, it would simply be as if that attribute was not created, right?

      And it seems that the current algorithms miss the toggleAttribute() endpoint.

      Choose a reason for hiding this comment

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

      If we edit append an attribute we might end up with duplicated calls to validate.

      For example in the screenshot below you'll see that we need to validate before step 5, but then step 6 which appends would also call validate?

      Screenshot 2024-04-10 at 16 12 14

      Choose a reason for hiding this comment

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

      Having said that we can reorder these steps so that step 4 is inside of 5's if so it becomes 5.1 and 5.2 is to replace?

      And then we can append attr and handle validation inside of append?

      (I've also just spotted it refers to a newAttr which doesn't exist in this algorithm I'll make sure to fix that)

      Choose a reason for hiding this comment

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

      Is toggleAttribute() actually an issue? Don't you still need to go through one of the other algorithms to actually set a value?

      Having said that it seems to trigger an error in Chrome when triggering "on" so might aswell cover in the spec.

      Choose a reason for hiding this comment

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

      @ghost

      Is it possible to get the preview linked at #1247 (comment) fixed? Would simplify reading the diff.

      smaug----

      element's node document.

    5. Validate and set attribute value value for attribute with

      		6.
      element.

      Choose a reason for hiding this comment

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

      This should probably clarify that validation may throw an exception. What should happen in that case?

      smaug----

    6. Validate and set attribute value value for attribute with

      		7.
      element.

    7. Append attribute to element.

      		8.

      Choose a reason for hiding this comment

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

      This is problematic. Validation may have run scripts, and scripts may have already added another attribute with same name. That can't be allowed.

      Choose a reason for hiding this comment

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

      Addressed in #1268 by rechecking the attribute state and throwing an exception if the default policy has done something funky.

      @koto koto mentioned this pull request

      Feb 12, 2024

      smaug----

    8. Validate and set attribute value value for attribute,

      		9.
      with this.

    9. Append attribute to this.

      		10.

      Choose a reason for hiding this comment

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

      This has also the problem that since validation may run scripts, the attribute list may now already have attribute with the same name. And validation may throw an exception.

      (but yeah, in general these checks do need to happen very early when we're about to set an attribute)

      @ghost

      @koto can the PR preview please be fixed? The changes themselves are sufficiently complicated, so let's make our life easier by simplifying reviewing.

      lukewarlow

      dom.bs Outdated Show resolved Hide resolved

      @lukewarlow

      I might be missing something but this doesn't seem to account for event handler attributes? I know there's separate handling for them in TT but wont this change block them being set? Or do they go down a different codepath?

      @koto

      @koto can the PR preview please be fixed? The changes themselves are sufficiently complicated, so let's make our life easier by simplifying reviewing.

      This is done, I'll proceed with resolving all the comments.

      @koto

      @lukewarlow

      @koto what remaining steps need to be taken on this?

      @ghost ghost mentioned this pull request

      Mar 27, 2024

      ziransun added a commit to ziransun/WebKit that referenced this pull request

      Apr 18, 2024

      @ziransun

      https://bugs.webkit.org/show_bug.cgi?id=270436.

      Reviewed by NOBODY (OOPS!).

      Implement the spec updates at whatwg/dom#1247. It also remove the some expectations in GTK as the results should be in line with the general expectation file.

      lukewarlow pushed a commit to lukewarlow/WebKit that referenced this pull request

      May 15, 2024

      @ziransun @lukewarlow

      https://bugs.webkit.org/show_bug.cgi?id=270436

      Reviewed by NOBODY (OOPS!).

      Implement the spec updates at whatwg/dom#1247 It also removes some expectations in GTK as the results should be in line with the general expectation file.

      lukewarlow pushed a commit to lukewarlow/WebKit that referenced this pull request

      May 15, 2024

      @ziransun @lukewarlow

      https://bugs.webkit.org/show_bug.cgi?id=270436

      Reviewed by NOBODY (OOPS!).

      Implement the spec updates at whatwg/dom#1247

      It also removes some expectations in GTK as the results should be in line with the general expectation file.

      webkit-commit-queue pushed a commit to ziransun/WebKit that referenced this pull request

      May 15, 2024

      @ziransun @lukewarlow

      https://bugs.webkit.org/show_bug.cgi?id=270436

      Reviewed by Ryosuke Niwa.

      Implement the spec updates at whatwg/dom#1247

      It also removes some expectations in GTK as the results should be in line with the general expectation file.

      Canonical link: https://commits.webkit.org/278817@main