-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Comments
/cc @josepharhar |
Related for dialogs: #10124 (comment) |
@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) |
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 |
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. |
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> |
@smaug---- does the code in my last comment capture your concerns? |
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> |
...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.
The text was updated successfully, but these errors were encountered: