Add before removing steps

[josepharhar]

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

[annevk]

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

[josepharhar]

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]

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

[josepharhar]

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

[mbrodesser-Igalia]

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]

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]

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

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

[annevk]

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]

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:

• The popover element's parent is null.
• The popover element's connected flag is true.

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]

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

[josepharhar]

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]

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

[mbrodesser-Igalia]

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]

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

[josepharhar]

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]

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]

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.

[josepharhar]

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