When attribute value changes, do we run hiding popover algorithm regardless?

[ziransun]

At https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute, for The "attribute change steps", at step 3, it says

If oldValue and value are in different [states](https://html.spec.whatwg.org/multipage/popover.html#attr-popover), then run the [hide popover algorithm](https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm) given element, true, true, and false.

Yet would it make sense to check if the popover is open before running the hiding popover steps? Checking current implementations, we can see that both chromium and WebKit do the checks. Gecko also does the check at an ongoing patch (https://phabricator.services.mozilla.com/D181880).

[ziransun]

@josepharhar @mbrodesser-Igalia

https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm calls in step 1. https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity which returns at latest in its step 2. if "popover visibility state is not showing". Hence https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm is a no-op if the popover isn't showing.

However, that doesn't cover the case when a popover is transitioning from "showing" to "hidden". Since the "popover visibility state" is set to "hidden" only in step 11 of https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm.

To fix this, the "attribute change steps" of https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute could be adapted to:
"[...] If oldValue and value are in different states and element's popover visibility state is showing and element's popover showing or hiding is not true. then run the hide popover algorithm given element, true, true, and false."

[josepharhar]

However, that doesn't cover the case when a popover is transitioning from "showing" to "hidden". Since the "popover visibility state" is set to "hidden" only in step 11 of https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm.

To fix this, the "attribute change steps" of https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute could be adapted to:
"[...] If oldValue and value are in different states and element's popover visibility state is showing and element's popover showing or hiding is not true. then run the hide popover algorithm given element, true, true, and false."

I see. Does this still have the same outcome though? And what is the purpose of avoiding hiding the popover when the attribute changes?

However, that doesn't cover the case when a popover is transitioning from "showing" to "hidden". Since the "popover visibility state" is set to "hidden" only in step 11 of https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm.

To fix this, the "attribute change steps" of https://html.spec.whatwg.org/multipage/popover.html#the-popover-attribute could be adapted to:
"[...] If oldValue and value are in different states and element's popover visibility state is showing and element's popover showing or hiding is not true. then run the hide popover algorithm given element, true, true, and false."

I see. Does this still have the same outcome though?

It depends on the scenario.

And what is the purpose of avoiding hiding the popover when the attribute changes?

@josepharhar: the purpose of the proposal (#9459 (comment)) is not to avoid hiding the popover when the attribute changes. Its purpose is to avoid calling https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm when the popover is already in the process of being hidden.

[josepharhar]

However, that doesn't cover the case when a popover is transitioning from "showing" to "hidden". Since the "popover visibility state" is set to "hidden" only in step 11 of https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm.

So if an event handler removes or changes the popover attribute in the middle of hiding the popover, aka "transitioning", then the attribute change steps will be called.
With the current spec, the attribute change steps will hide the popover during the "transitioning". When the event firing returns to the original call to hide popover, it will probably stop trying to hide the popover since it is already hidden.
With this proposed change, the attribute change steps will... still try to hide the popover since it isn't hidden yet? Even if it didn't try to hide the popover, the end state will still be the same - the popover will be hidden.

Checking current implementations, we can see that both chromium and WebKit do the checks

I just checked chromium and I agree that it only tries to hide the popover if it is showing, so I am supportive of this change. However, I still don't completely understand what the difference is. Could someone provide a code example which demonstrates different behavior with and without this change?

However, that doesn't cover the case when a popover is transitioning from "showing" to "hidden". Since the "popover visibility state" is set to "hidden" only in step 11 of https://html.spec.whatwg.org/multipage/popover.html#hide-popover-algorithm.

So if an event handler removes or changes the popover attribute in the middle of hiding the popover, aka "transitioning", then the attribute change steps will be called.

Yes.

With the current spec, the attribute change steps will hide the popover during the "transitioning". When the event firing returns to the original call to hide popover, it will probably stop trying to hide the popover since it is already hidden.

It will stop, yes. Potentially with throwing an exception.

With this proposed change, the attribute change steps will... still try to hide the popover since it isn't hidden yet?

Yes. The "transitioning" will finish.

Even if it didn't try to hide the popover, the end state will still be the same - the popover will be hidden.

Yes.

Checking current implementations, we can see that both chromium and WebKit do the checks

I just checked chromium and I agree that it only tries to hide the popover if it is showing, so I am supportive of this change. However, I still don't completely understand what the difference is.

With respect to "beforetoggle" and "toggle" events, there's no difference.

Could someone provide a code example which demonstrates different behavior with and without this change?

There's https://jsfiddle.net/rwjv3gLo/2/ which violates an assertion (https://searchfox.org/mozilla-central/rev/f03ce34731ff8d1f6c77d8241c41d10df13e9aca/dom/html/nsGenericHTMLElement.cpp#3222) with that change and not without the change. However, such an assertion doesn't exist in Chromium (https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:third_party/blink/renderer/core/html/html_element.cc;l=1334;drc=f09c12c84b39d13189a7039a05253ca3766d4751).

As already noticed in previous comments, Chromium diverges from the spec (https://html.spec.whatwg.org/#check-popover-validity) here, because it doesn't check whether the popover attribute is in the "no popover" state, but checks an internal state (https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:third_party/blink/renderer/core/html/html_element.cc;l=1275;drc=f09c12c84b39d13189a7039a05253ca3766d4751) which sometimes doesn't match. Unfortunately I currently don't have an idea how to fix his. Anyone else?

[josepharhar]

There's https://jsfiddle.net/rwjv3gLo/2/ which violates an assertion (https://searchfox.org/mozilla-central/rev/f03ce34731ff8d1f6c77d8241c41d10df13e9aca/dom/html/nsGenericHTMLElement.cpp#3222) with that change and not without the change

Thanks! I created a spec PR here: #9526