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

Handle attribute changes after changing attribute #1176

Merged
merged 4 commits into from
Jun 10, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Mar 25, 2023

At least in chromium, synchronous internal listeners for attribute changes which are run as "attribute change steps" are run after the attribute is actually changed, not before.

This affects popover attribute related algorithms in HTML: whatwg/html#9048 (comment)


Preview | Diff

At least in chromium, synchronous internal listeners for attribute
changes which are run as "attribute change steps" are run after the
attribute is actually changed, not before.

This affects popover attribute related algorithms in HTML:
whatwg/html#9048 (comment)
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

@josepharhar you want to change all call sites of "Handle attribute changes" presumably.

@smaug---- @rniwa I assume this is the same everywhere and this was just a bug.

@smaug----
Copy link
Collaborator

smaug---- commented Mar 30, 2023

In Gecko one can handle attribute changes before the value has been added/changed/removed or after, depending on the case. And both are useful. The reason is mostly that BeforeSetAttr can still access the old value (which can be used for unregistering the element from some data structures or cancelling loads etc.), and AfterSetAttr can then trigger whatever the new value should do. But I'm not sure if this is all just implementation detail. Would need to read through how attribute change steps is used.

And at least the pr would make the spec inconsistent. Why only change is modified, why not also append and others?

@annevk
Copy link
Member

annevk commented Mar 30, 2023

Right, I agree that we should change all of them together.

@josepharhar
Copy link
Contributor Author

Right, I agree that we should change all of them together.

Ok, I changed all of them

dom.bs Outdated Show resolved Hide resolved
dom.bs Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@emilio did you want to have a final look at this as well?

@annevk annevk merged commit 3fb0aa6 into whatwg:main Jun 10, 2023
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 13, 2023
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 13, 2023
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants