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

Add before removing steps #1185

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Apr 17, 2023

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

  • At least two implementers are interested (and none opposed):
    • Chrome
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
  • MDN issue is filed: …

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

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
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do implementations even run after steps or is this essentially the same problem as the attribute notification?

@josepharhar
Copy link
Contributor Author

Yes, implementations run steps after as well.
Things chromium does before removal:

  • Hides popovers
  • Fires sync mutation events
  • Cleans up Range instances

Things chromium does after removal:

  • Clears some style invalidation bits
  • Removes the associated computed style
  • Removes references to the element's id and name attributes from the document
  • Removes the element from the document's top layer

I'm not sure how many of these are specced or not, but chromium definitely does stuff before and after removal and these are just a few points that I could easily explain by skimming the code - there are a lot more things that happen in both cases.

@annevk
Copy link
Member

annevk commented Apr 28, 2023

Would this end up running script in the case of hidePopover()? Or would that always be prevented here?

@josepharhar
Copy link
Contributor Author

When the removal steps call hidePopover, the fireEvents flag is set to false which prevents any script from being run.

@mbrodesser-Igalia
Copy link
Member

Yes, implementations run steps after as well.
Things chromium does before removal:

Hides popovers
Fires sync mutation events
Cleans up Range instances

Things chromium does after removal:

Clears some style invalidation bits
Removes the associated computed style
Removes references to the element's id and name attributes from the document
Removes the element from the document's top layer

That implies, the "hide a popover" algorithm (https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm) needs to be adapted too, since the element is removed from the document's top layer in that algorithm.

@annevk
Copy link
Member

annevk commented May 31, 2023

Things chromium does before removal

Except for popover we handle all of those in DOM directly. Should we really make that an extension point? Is there some other way to design popover?

@mbrodesser-Igalia
Copy link
Member

Except for popover we handle all of those in DOM directly.

@annevk: what does "in DOM directly" mean?

@annevk
Copy link
Member

annevk commented May 31, 2023

That the DOM Standard handles them in its own algorithms without providing hooks. (Technically not true for mutation events, but there's an open issue for that and if we ever did specify those we wouldn't do it through a hook.)

@josepharhar
Copy link
Contributor Author

Except for popover we handle all of those in DOM directly. Should we really make that an extension point? Is there some other way to design popover?

We could call hide popover from the DOM spec instead of exposing an extension point.

I did some investigation and it actually turns out that chromium doesn't exactly run hide popover before removal, but does it at an intermediate stage where the state looks like this:

I'm not sure if this state can exist in the spec because step 11 of the removing steps probably atomically removes the parent and sets the connected flag to false - chromium has separate state for these.

The reason that chromium does this is because it turns out that we don't truly have a hook for before removing steps on any node, at least not yet. Mason told me that this is basically close enough to truly running hide popover before removal though, and I agree. If that's not the case, then we can change it in chromium.

I tried calling hide popover after removal instead of before, but it caused 7 different WPTs to fail. I think that if we shouldn't add an extension, then we should call hide popover from the DOM spec at the right place before removal.

@annevk
Copy link
Member

annevk commented Jun 2, 2023

What I'm wondering is whether we could do it in some other way without modifying the DOM specification.

@josepharhar
Copy link
Contributor Author

I suppose that if we added a parameter to the hide popover algorithm which tells it that the element is disconnected and we should hide it anyway, and also add that parameter to check popover validity, then it could theoretically be possible to hide after disconnecting and therefore not change the DOM spec. We would probably also need to pass in a document since I assume that the element would no longer have a "node document" after being disconnected

@annevk
Copy link
Member

annevk commented Jun 7, 2023

That sounds pretty good to me, thanks for investigating! @rwlbuis @nt1m?

@mbrodesser-Igalia
Copy link
Member

An alternative idea: when https://html.spec.whatwg.org/#hide-popover-algorithm is invoked, instead of invoking https://html.spec.whatwg.org/#check-popover-validity from there, check only if https://html.spec.whatwg.org/#popover-visibility-state is "hidden" and if so return early.
Be aware that https://html.spec.whatwg.org/#popover-visibility-state is set to "hidden" only in https://html.spec.whatwg.org/#hide-popover-algorithm.

https://html.spec.whatwg.org/#dom-hidepopover would need to be extended to check whether the popover attribute is not present and throw in that case. Similar adaptations might be needed for other callers of https://html.spec.whatwg.org/#hide-popover-algorithm, but it might overall be a simpler solution than #1185 (comment).

@nt1m
Copy link
Member

nt1m commented Jun 7, 2023

Before removing steps is how it's done in WebKit, but I'm open to different solution too.

@josepharhar
Copy link
Contributor Author

An alternative idea: when https://html.spec.whatwg.org/#hide-popover-algorithm is invoked, instead of invoking https://html.spec.whatwg.org/#check-popover-validity from there, check only if https://html.spec.whatwg.org/#popover-visibility-state is "hidden" and if so return early.

If we don't call check popover validity as the first thing in hide popover algorithm, then we would be skipping opportunities to throw exceptions. For example, this should throw an exception:

const div = document.createElement('div');
// don't add the popover attribute
div.hidePopover(); // throws because there is no popover attribute

https://html.spec.whatwg.org/#dom-hidepopover would need to be extended to check whether the popover attribute is not present and throw in that case. Similar adaptations might be needed for other callers of https://html.spec.whatwg.org/#hide-popover-algorithm, but it might overall be a simpler solution than #1185 (comment).

I see, yeah I guess that in the case where we want to hide a popover regardless of the dom state, the only relevant check in the check popover algorithm is whether or not the popover is in the showing or hidden state, so we could conditionally replace calls to the check popover algorithm with checks for the popover visibility state. Sounds good to me, and it could also be used to fix whatwg/html#9367

@mbrodesser-Igalia
Copy link
Member

I see, yeah I guess that in the case where we want to hide a popover regardless of the dom state, the only relevant check in the check popover algorithm is whether or not the popover is in the showing or hidden state, so we could conditionally replace calls to the check popover algorithm with checks for the popover visibility state. Sounds good to me, and it could also be used to fix whatwg/html#9367

Keep in mind:

<div>X</div>
<script>
let d = document.querySelector("div");
d.hidePopover(); // Should throw according to spec and does in Chrome dev edition.
</script>

However, https://jsfiddle.net/hyzLu4nk/1/ doesn't throw in Chrome dev edition. Adapting the spec to let https://html.spec.whatwg.org/#dom-hidepopover throw only when https://html.spec.whatwg.org/#popover-showing-or-hiding is false would cover that.

@mbrodesser-Igalia
Copy link
Member

However, https://jsfiddle.net/hyzLu4nk/1/ doesn't throw in Chrome dev edition.

Given whatwg/html#9142 (comment), it seems to make sense to specify that. @josepharhar WDYT?
@nt1m would you support specifying this?

Locally I've a WPT for that scenario which I'd request to pull once the above questions are clarified.

@mbrodesser-Igalia

This comment was marked as resolved.

@annevk

This comment was marked as resolved.

@mbrodesser-Igalia

This comment was marked as resolved.

josepharhar added a commit to josepharhar/html that referenced this pull request Jun 23, 2023
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 pull request Jun 26, 2023
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
Copy link
Contributor Author

I opened an HTML PR which should solve the bug without the need for adding the DOM hook this PR adds: whatwg/html#9457

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants