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.

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

Attribute change steps should run after setting the value, not before #1190

Closed
emilio opened this issue Apr 24, 2023 · 6 comments
Closed

Attribute change steps should run after setting the value, not before #1190

emilio opened this issue Apr 24, 2023 · 6 comments

Comments

@emilio
Copy link
Contributor

emilio commented Apr 24, 2023 •
edited
Loading

https://dom.spec.whatwg.org/#concept-element-attributes-change first runs the change steps, then sets the value.

That means that if the steps trigger something that then changes the relevant attribute, per spec we should keep the "old" value, which seems wrong.

Example:

<!doctype html>
<div popover="manual">wat</div>
<button>Hide</button>
<script>
let div = document.querySelector("div");
div.addEventListener("beforetoggle", function(e) {
  if (e.newState == "closed") {
    this.setAttribute("popover", "manual");
  }
})
document.querySelector("button").addEventListener("click", function(e) {
  div.removeAttribute("popover");
});
div.showPopover();
</script>

Per spec, after clicking the value the popover attribute should not be there, which is wrong and doesn't match implementations.

My guess is that modulo that case this is generally not very observable, because other attribute change steps are usually async.

It feels very weird tho that removeAttribute runs script synchronously for popover btw...

cc @mbrodesser-Igalia, @mfreed7, @domenic, @annevk, @smaug----
@annevk
Copy link
Member

annevk commented Apr 24, 2023

Could you review #1176?

I think there are a couple attribute listeners that might run script and they're not as bad as mutation events, but if there are multiple there could well be ordering issues I suppose.

@mfreed7
Copy link
Contributor

mfreed7 commented Apr 28, 2023

@josepharhar

@josepharhar
Copy link
Contributor

@emilio does #1176 resolve this issue? At least with regards to DOM spec?

@annevk
Copy link
Member

annevk commented Jun 8, 2023

It solves the ordering problem. You still have synchronous notification of a node tree mutation. I think when I previously looked into such notifications I did not find them problematic for attributes. But maybe someone else should look into that again, especially with Chromium wanting to remove mutation events.

@mbrodesser-Igalia
Copy link
Member

Given #1176 was merged, this can be closed?

@annevk
Copy link
Member

annevk commented Jun 14, 2023

I think so. @emilio please reopen or say something if you disagree.

@annevk annevk closed this as completed Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@emilio @annevk @josepharhar @mfreed7 @mbrodesser-Igalia