Ignore DOM state when hiding popovers if needed

[josepharhar]

Fixes #9161
Fixes #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.

• At least two implementers are interested (and none opposed):
  • Chrome
  • …
• Tests are written and can be reviewed and commented upon at:
  • In progress
• Implementation bugs are filed:
  • Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/4638678
  • Gecko: …
  • WebKit: …
• MDN issue is filed: …

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

---

/infrastructure.html ( diff )
/popover.html ( diff )

[josepharhar]

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.

If anyone thinks that we should continue firing events when script runs element.removeAttribute('popover') on a showing popover, then I could modify this PR to do so by adding additional plumbing of flags around the algorithms. Does anyone have opinions on this?

[mbrodesser-Igalia]

If anyone thinks that we should continue firing events when script runs element.removeAttribute('popover') on a showing popover

It would increase inconsistency of the Popover API's behavior. Including making the (non-normative) description of the "beforetoggle" event (https://html.spec.whatwg.org/#event-beforetoggle) less precise.

[josepharhar]

If anyone thinks that we should continue firing events when script runs element.removeAttribute('popover') on a showing popover

It would increase inconsistency of the Popover API's behavior

So do you think that if we continue firing events during element.removeAttribute('popover') it would increase inconsistency? Or vice versa? Would you prefer events or no events?

[mbrodesser-Igalia]

If anyone thinks that we should continue firing events when script runs element.removeAttribute('popover') on a showing popover

It would increase inconsistency of the Popover API's behavior

So do you think that if we continue firing events during element.removeAttribute('popover') it would increase inconsistency? Or vice versa?

Sorry for the unclarity. Events should continued to be fired during element.removeAttribute('popover').

[josepharhar]

Thanks for clarifying.
@nt1m @annevk do yall have any thoughts?

[annevk]

No strong opinion. While synchronous events around node tree mutations make me a bit nervous, they are "safe" for attributes as far as I know.

[josepharhar]

Ok, I added parameters to check popover validity and hide popover in order to make removeAttribute('popover') start firing events again

[mbrodesser-Igalia]

This implies https://whatpr.org/html/9457/8430871...acbbfdb/popover.html#check-popover-validity will skip checking whether the popover attribute is in the "no popover" state. However, value can represent "auto" or "manual" and then the popover attribute should also correspond to it and hence should be checked, or?

Please be aware of the last paragraph of #9459 (comment).

[josepharhar]

With the change in #9526 this should be fine

[mbrodesser-Igalia]

With the change in #9526 this should be fine

Depends on what "fine" means. That check should still pass, e.g. when the popover is open and the attribute state changes from "auto" to "manual" but with this patch the check wouldn't be executed.

[josepharhar]

The reason that I added this here is because I want to make sure that the popover gets hidden even if the attribute is in the no popover state, so that the popover gets hidden when you call element.removeAttribute('popover').

However, value can represent "auto" or "manual" and then the popover attribute should also correspond to it and hence should be checked, or?

I suppose that we could pass false instead of true for ignoreDomState in the case that the new state is popover=auto or popover=manual, but what difference would it make? We still want to close the popover in these cases

but with this patch the check wouldn't be executed

You're talking about the check to see whether we are in the no popover state, right? In these cases we know that we are not in the no popover state already...

[mbrodesser-Igalia]

I suppose that we could pass false instead of true for ignoreDomState in the case that the new state is popover=auto or popover=manual, but what difference would it make?

It'd make the spec, and implementations, more robust. Otherwise, the latter's behaviors may differ, depending on their internals.

We still want to close the popover in these cases

Yes.

but with this patch the check wouldn't be executed

You're talking about the check to see whether we are in the no popover state, right?

Yes.

In these cases we know that we are not in the no popover state already...

[mbrodesser-Igalia]

I intend prototyping/implementing this in Firefox/Gecko, but it will take some time due to days off.

[mbrodesser-Igalia]

Thanks for clarifying. @nt1m @annevk do yall have any thoughts?

In case of attribute removal, it might make sense to not fire "beforetoggle" events, but only "toggle" events. This is motivated by the use case described at https://open-ui.org/components/popover.research.explainer/#events (updating server data). It might not matter, in which of those two events the server data is updated.
More general, when hiding a popover, the "toggle" event might suffice, unless there are use-cases for a "beforetoggle" event.

This differs from a "beforetoggle" event when showing a popover, for which https://open-ui.org/components/popover.research.explainer/#events describes a use case.

@domenic are you, as a contributor to https://open-ui.org/components/popover.research.explainer/, aware of other use cases or can please recommend any other contributor to shed light on this?

[mbrodesser-Igalia]

Additionally, renaming "toggle" to "aftertoggle" seems more precise too.

[josepharhar]

More general, when hiding a popover, the "toggle" event might suffice, unless there are use-cases for a "beforetoggle" event.

Additionally, renaming "toggle" to "aftertoggle" seems more precise too.

I'd prefer not to make these breaking changes to popover now that it has already shipped in stable chrome.

I'm sure that some people like having a synchronous beforetoggle event when popovers close even if it isn't cancelable.

If I remember correctly, we used the name "toggle" to match the existing toggle event for details elements.

[josepharhar]

For context, here are some issues with discussions and resolutions about having both sync beforetoggle and async toggle events: openui/open-ui#342 openui/open-ui#578
And here is one about naming of the events: openui/open-ui#607

[mbrodesser-Igalia]

For context, here are some issues with discussions and resolutions about having both sync beforetoggle and async toggle events: openui/open-ui#342 openui/open-ui#578 And here is one about naming of the events: openui/open-ui#607

Thanks. Wasn't aware of those tickets.

openui/open-ui#342 states that "beforetoggle" is acceptable but not necessary when hiding a popover. Didn't fully read openui/open-ui#578 yet.

[mbrodesser-Igalia]

I'd prefer not to make these breaking changes to popover now that it has already shipped in stable chrome.

Understandable, but that shouldn't be a stopper. If it's changed, the earlier, the painless.

I'm sure that some people like having a synchronous beforetoggle event when popovers close even if it isn't cancelable.

If there was evidence about it, that'd be an argument.

If I remember correctly, we used the name "toggle" to match the existing toggle event for details elements.

Okay. Seems okay to keep the current name.

[josepharhar]

More general, when hiding a popover, the "toggle" event might suffice, unless there are use-cases for a "beforetoggle" event.

If there was evidence about it, that'd be an argument.

I think that consistently firing both events instead of sometimes firing one and sometimes firing both is a more consistent behavior that will make it easier for developers to rely on.

[mbrodesser-Igalia]

More general, when hiding a popover, the "toggle" event might suffice, unless there are use-cases for a "beforetoggle" event.

If there was evidence about it, that'd be an argument.

I think that consistently firing both events instead of sometimes firing one and sometimes firing both is a more consistent behavior that will make it easier for developers to rely on.

One issue with firing a "beforetoggle" event in the hide popover algorithm is, when element.removeAttribute('popover') is called, it's not happening precisely before toggling, see https://jsfiddle.net/csoj3g1t/. May be acceptable, though.

[mbrodesser-Igalia]

I think that consistently firing both events instead of sometimes firing one and sometimes firing both is a more consistent behavior that will make it easier for developers to rely on.

It would be helpful to have use-cases for the "beforetoggle" event in case a popover is hidden.
If there are some, they should be supported. Otherwise, it would presumably allow simplifying the spec, and implementations, significantly. No "ignoreDomState" might be required and nested hiding might also not be possible anymore. It might also simplify augmenting the popover feature for popover=hint, which presumably will happen during this year.

The feature has only recently shipped in Chrome stable and not in other browsers yet, according to https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover#browser_compatibility.

[josepharhar]

It would be helpful to have use-cases for the "beforetoggle" event in case a popover is hidden.
If there are some, they should be supported. Otherwise, it would presumably allow simplifying the spec, and implementations, significantly. No "ignoreDomState" might be required and nested hiding might also not be possible anymore. It might also simplify augmenting the popover feature for popover=hint, which presumably will happen during this year.

@mfreed7 do you have any opinions about removing beforetoggle events during attribute removal or node removal?

[mfreed7]

It would be helpful to have use-cases for the "beforetoggle" event in case a popover is hidden.
If there are some, they should be supported. Otherwise, it would presumably allow simplifying the spec, and implementations, significantly. No "ignoreDomState" might be required and nested hiding might also not be possible anymore. It might also simplify augmenting the popover feature for popover=hint, which presumably will happen during this year.

@mfreed7 do you have any opinions about removing beforetoggle events during attribute removal or node removal?

Sorry, I'm not sure what the question is, exactly. I don't think there are any "good" use cases for needing the beforetoggle event to fire (synchronously or at all) in the cases that the element is removed or the attribute is changed. The current Chromium prototype does not fire beforetoggle at all in these cases, and I'm good with that behavior. I haven't heard of situations that are broken by this. Does that answer the question?