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

Popover: spec for dealing with element removal is incorrect #9161

Open
jakearchibald opened this issue Apr 13, 2023 · 6 comments · May be fixed by #9457
Open

Popover: spec for dealing with element removal is incorrect #9161

jakearchibald opened this issue Apr 13, 2023 · 6 comments · May be fixed by #9457
Labels
topic: popover The popover attribute and friends

Comments

@jakearchibald
Copy link
Contributor

https://html.spec.whatwg.org/multipage/infrastructure.html#dom-trees:hide-popover-algorithm

When a popover is removed from the DOM, it calls the hide popover algorithm.

That algorithm will always exit on step 1, since the removed element is no longer connected to the document.
@jakearchibald jakearchibald added the topic: popover The popover attribute and friends label Apr 13, 2023
@jakearchibald
Copy link
Contributor Author

I think the behaviour here needs some design. Does it make sense to fire all of the events in this case?

@jakearchibald jakearchibald mentioned this issue Apr 14, 2023
Closed
@mfreed7
Copy link
Contributor

mfreed7 commented Apr 14, 2023

I agree that the spec is broken for this case. The difference between the spec and at least the Chromium implementation is that the spec for remove runs the removing steps at step 15, but removes the node from the tree before that, at step 11. The Chromium impl runs that call to HidePopoverInternal before the node is removed from the tree.

I'm not sure what the best way to fix the spec is. Seems like there might need to be "before removing steps" added before step 11 or something? @josepharhar @annevk @nt1m

@mfreed7
Copy link
Contributor

mfreed7 commented Apr 14, 2023

Does it make sense to fire all of the events in this case?

We discussed this at great length during the spec PR review. The problem is that events can't be fired synchronously during removal, so we'd have to queue the events in this case, which leads to myriad problems like out of order events. So we decided that the best course of action was not firing events in this case.

@nt1m
Copy link
Member

nt1m commented Apr 14, 2023

Yes, WebKit also runs removing steps before removing the node from the tree. That part of the spec is definitely broken here.

josepharhar added a commit to josepharhar/dom that referenced this issue Apr 17, 2023
@josepharhar
Add before removing steps
b096dc0 
The existing "removing steps" allow other specs to run algorithms after a
node has been removed from the tree, but there is no way to run steps
before a node has been removed from the tree.

This patch adds new "before removing steps" to allow other specs to run
algorithms before the node has been removed.

This is needed for this HTML issue:
whatwg/html#9161
@josepharhar josepharhar mentioned this issue Apr 17, 2023
Open
4 tasks
@josepharhar
Copy link
Contributor

I opened a DOM PR to add "before removing steps" that can hook into here: whatwg/dom#1185

@mbrodesser-Igalia mbrodesser-Igalia mentioned this issue May 31, 2023
Open
josepharhar added a commit to josepharhar/html that referenced this issue Jun 23, 2023
@josepharhar
Ignore DOM state when hiding popovers if needed
b13b4d0 
Fixes whatwg#9161
Fixes whatwg#9367
Makes obsolete whatwg/dom#1185

This PR prevents the hide popover algorithm from returning early when
the popover attribute is removed or when the element with the popover
attribute is removed from the document.

The fireEvents parameter is used as an indicator that either the element
is being removed or that the attribute is being removed, and when it is
false, the calls to check popover validity are replaced with a check to
simply see if the popover is already hidden.

This patch also makes removal of the popover attribute stop firing
events in order to signal to the hide popover algorithm that checks for
the popover attribute should be ignored.
josepharhar added a commit to josepharhar/html that referenced this issue Jun 26, 2023
@josepharhar
Ignore DOM state when hiding popovers if needed
f60c3a0 
Fixes whatwg#9161
Fixes whatwg#9367
Makes obsolete whatwg/dom#1185

This PR prevents the hide popover algorithm from returning early when
the popover attribute is removed or when the element with the popover
attribute is removed from the document.

The fireEvents parameter is used as an indicator that either the element
is being removed or that the attribute is being removed, and when it is
false, the calls to check popover validity are replaced with a check to
simply see if the popover is already hidden.

This patch also makes removal of the popover attribute stop firing
events in order to signal to the hide popover algorithm that checks for
the popover attribute should be ignored.
@josepharhar josepharhar linked a pull request Jun 26, 2023 that will close this issue
Open
4 tasks
@josepharhar
Copy link
Contributor

I opened an HTML PR which should make the above DOM PR obsolete by fixing this bug in a different way #9457

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

Successfully merging a pull request may close this issue.

Ignore DOM state when hiding popovers if needed josepharhar/html
4 participants
@jakearchibald @nt1m @josepharhar @mfreed7