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

attribute change steps for popover may fire events synchronously #10129

Open
smaug---- opened this issue Feb 8, 2024 · 9 comments
Open

attribute change steps for popover may fire events synchronously #10129

smaug---- opened this issue Feb 8, 2024 · 9 comments
Labels
topic: popover The popover attribute and friends

Comments

@smaug----
Copy link
Collaborator

smaug---- commented Feb 8, 2024

...and some algorithms in DOM spec don't like that, or at least might behave a bit unexpectedly.
(Sure, CEReactions might also lead to somewhat unexpected behavior in some cases, if custom elements happen to tweak values in DOM)

Removing popover attribute calls "hide popover algorithm given element, true, true, and false", where the second true is for fireEvents, which will trigger beforetoggle event synchronously.

@keithamus keithamus added the topic: popover The popover attribute and friends label Feb 8, 2024
@keithamus
Copy link
Contributor

/cc @josepharhar

@josepharhar
Copy link
Contributor

Related for dialogs: #10124 (comment)

@josepharhar
Copy link
Contributor

@mfreed7 I imagine this is something that we've designed it to work around, right? Do you have any thoughts for the linked dialog issue too? #10124 (comment)

@mfreed7
Copy link
Collaborator

mfreed7 commented Feb 9, 2024

I initially commented here, but I was thinking about element removal (for which we definitely do not fire sync events), not attribute removal. The current spec for attribute removal matches at least the implementation in Chromium, and does fire a sync event in that case.

Can you give an example of where/how this is a problem? Since it's for the closing event, which isn't cancelable, it might be ok to delay it a bit. But we did have a significant number of conversations about the event being beforetoggle and if you delay it, the "before" part of that gets confusing, particularly for developers.

@smaug----
Copy link
Collaborator Author

The problem is that for example removeNamedItem may return an Attr for the remove attribute even though some code run while the method was being executed an added another attribute with same name. That is rather unexpected (mutation event -like) side effect.

@josepharhar
Copy link
Contributor

The problem is that for example removeNamedItem may return an Attr for the remove attribute even though some code run while the method was being executed an added another attribute with same name.

Is this the case that you're describing?

<div popover=auto id=mypopover>popover</div>
<script>
mypopover.showPopover();
mypopover.addEventListener('beforetoggle', () => mypopover.setAttribute('popover', 'auto'));
mypopover.removeAttribute('popover');
</script>

@josepharhar
Copy link
Contributor

@smaug---- does the code in my last comment capture your concerns?

@smaug----
Copy link
Collaborator Author

Well, remove the the attribute using removeNamedItem, since with that you can actually see the side effect easily.

@josepharhar
Copy link
Contributor

Well, remove the the attribute using removeNamedItem, since with that you can actually see the side effect easily.

So instead of setting the attribute to something else, remove the one with the same name...? Or something else?

<div popover=auto id=mypopover>popover</div>
<script>
mypopover.showPopover();
mypopover.addEventListener('beforetoggle', () => mypopover.removeAttribute('popover'));
mypopover.removeAttribute('popover');
</script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

No branches or pull requests

5 participants
@keithamus @josepharhar @smaug---- @mfreed7 and others