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

Clarification on Element reflected IDL attribute steps #10150

Closed
josepharhar opened this issue Feb 20, 2024 · 9 comments
Closed

Clarification on Element reflected IDL attribute steps #10150

josepharhar opened this issue Feb 20, 2024 · 9 comments
Labels
topic: reflect For issues with reflected IDL attributes and friends

Comments

@josepharhar
Copy link
Contributor

What is the issue with the HTML Standard?

https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes%3Aelement

Consider the following:

<button id=mybutton>button</button>
<div id=mypopover popover=auto>popover</div>
<script>
mybutton.popoverTargetElement = mypopover;
mybutton.setAttribute('popovertarget', '');
mybutton.popoverTargetElement; // What should this return?
</script>

The latest implementation in chromium returns null for popoverTargetElement, and firefox returns the popover element. The reason that chromium returns null is because of these steps in the spec (which is implemented here):

For element reflected targets only: the following attribute change steps, given element, localName, oldValue, value, and namespace, are used to synchronize between the content attribute and the IDL attribute:

  1. If localName is not attr or namespace is not null, then return.
  2. Set element's explicitly set attr-element to null.

When the attribute is set to the empty string, then it sets the explicitly set attr element to null. A WPT change was made here which tests this: web-platform-tests/wpt#44634

@ziransun @emilio which behavior is correct?

@emilio
Copy link
Contributor

emilio commented Feb 20, 2024

Hmm, so I think this was an oversight on my side when reviewing the patch that introduced the test.

So the setter's step 2 is:

Run this's set the content attribute with the empty string.

So setAttribute('popovertarget', '') is setting the value to the same.

However, per spec, https://dom.spec.whatwg.org/#concept-element-attributes-set-value does run the attribute change steps, even if the attribute didn't change. Apparently this is consistent with mutation events too.

So I think this is a Firefox bug, and we also need to clear the explicit element even if the attribute doesn't change...

Unless someone disagrees? I find it quite weird that setAttribute("foo", getAttribute("foo")) has side effects, but that's already the case so...

@emilio
Copy link
Contributor

emilio commented Feb 20, 2024

cc @smaug---- in case you have opinions.

@emilio
Copy link
Contributor

emilio commented Feb 20, 2024

https://bugzilla.mozilla.org/show_bug.cgi?id=1881075 has a fix for that.

@josepharhar
Copy link
Contributor Author

Thanks!

So the setter's step 2 is:

Run this's set the content attribute with the empty string.

So setAttribute('popovertarget', '') is setting the value to the same.

So the spec as written would clear the explicitly set attr element every time you set it?

@emilio
Copy link
Contributor

emilio commented Feb 20, 2024

Yeah, unless I'm misreading.

@smaug----
Copy link

Setting an attribute value means setting an attribute value - doesn't matter whether the value is the same or not :)

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 20, 2024
See whatwg/html#10150. Per spec these run even
if the attribute doesn't change.

Differential Revision: https://phabricator.services.mozilla.com/D202240

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1881075
gecko-commit: 2c4646b33c7313f86fb6fb60be42fb5fcb5f106b
gecko-reviewers: zsun
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 21, 2024
See whatwg/html#10150. Per spec these run even
if the attribute doesn't change.

Differential Revision: https://phabricator.services.mozilla.com/D202240
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 21, 2024
See whatwg/html#10150. Per spec these run even
if the attribute doesn't change.

Differential Revision: https://phabricator.services.mozilla.com/D202240

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1881075
gecko-commit: 2c4646b33c7313f86fb6fb60be42fb5fcb5f106b
gecko-reviewers: zsun
@annevk annevk added the topic: reflect For issues with reflected IDL attributes and friends label Feb 21, 2024
@annevk
Copy link
Member

annevk commented Feb 21, 2024

Can this be closed?

@emilio
Copy link
Contributor

emilio commented Feb 21, 2024

Yeah... We might want a different name for the attribute change steps (maybe attribute set or remove steps? Though that's a mouthful)

@emilio emilio closed this as completed Feb 21, 2024
@annevk
Copy link
Member

annevk commented Feb 21, 2024

We could use "mutation" to align with mutation observers (or poke/meddle), but fundamentally you have to know these steps always run. If you want to make a change (hah) like that, prolly best discussed over in whatwg/dom.

@annevk annevk closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2024
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Feb 23, 2024
See whatwg/html#10150. Per spec these run even
if the attribute doesn't change.

Differential Revision: https://phabricator.services.mozilla.com/D202240

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1881075
gecko-commit: 2c4646b33c7313f86fb6fb60be42fb5fcb5f106b
gecko-reviewers: zsun
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Feb 24, 2024
See whatwg/html#10150. Per spec these run even
if the attribute doesn't change.

Differential Revision: https://phabricator.services.mozilla.com/D202240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: reflect For issues with reflected IDL attributes and friends
Development

No branches or pull requests

4 participants