togglePopover() algorithm does not check popover validity when force == false and when the popover visibility state is hidden

[nt1m]

Seems like an oversight to me.

cc @josepharhar @mfreed7 @annevk

[nt1m]

Probably doesn't matter too much fwiw.

[mfreed7]

Seems like an oversight to me also, but I'll let @josepharhar comment.

[josepharhar]

togglePopover just calls "hide a popover" or "show popover", and each of those algorithms check popover validity as their first step.

Are you saying that when togglePopover decides not to do anything, we should still call check popover validity...? I don't see why we should.

[nt1m]

Are you saying that when togglePopover decides not to do anything, we should still call check popover validity...? I don't see why we should.

There is this slight oddity when you call togglePopover(false) on an element with no popover attribute and it ends up doing nothing instead of throwing an exception. Seems like a bad developer experience, though not something I feel too strongly about.

[annevk]

What if we make the logic:

1. If visibility state is showing and force is not given, or force is given and is false, then hide.
2. Otherwise, show.

[nt1m]

@annevk Do you think it's still worth changing the spec given the recent changes to not throw?

The implication of this change is that the other "check popover validity" checks (dialog, fullscreen, disconnected) would run in the cases I mentioned in the title: https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity

[annevk]

We decided to not throw in certain cases. Did we decide to not throw for cases where state does not match up? It looks to me like showPopover() still throws for this condition so togglePopover() should as well.

[josepharhar]

The implication of this change is that the other "check popover validity" checks (dialog, fullscreen, disconnected) would run in the cases I mentioned in the title: https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity

So the following code would result in a call to check popover validity and possibly throw exceptions?

const div = document.createElement('div');
div.setAttribute('popover', 'auto');
// don't connect to document or show
div.togglePopover(/*force=*/false); // try to force close

We decided to not throw in certain cases. Did we decide to not throw for cases where state does not match up? It looks to me like showPopover() still throws for this condition so togglePopover() should as well.

So the above code I wrote should throw an exception then?

[nt1m]

The implication of this change is that the other "check popover validity" checks (dialog, fullscreen, disconnected) would run in the cases I mentioned in the title: https://html.spec.whatwg.org/multipage/popover.html#check-popover-validity

So the following code would result in a call to check popover validity and possibly throw exceptions?

const div = document.createElement('div');
div.setAttribute('popover', 'auto');
// don't connect to document or show
div.togglePopover(/*force=*/false); // try to force close

We decided to not throw in certain cases. Did we decide to not throw for cases where state does not match up? It looks to me like showPopover() still throws for this condition so togglePopover() should as well.

So the above code I wrote should throw an exception then?

@josepharhar Since hidePopover() wouldn't throw in this case, I would say it shouldn't (and in fact it wouldn't with the suggested change, since the check popover validity algorithm wouldn't in this case).

But:

const div = document.createElement('div');
// don't set popover attribute
// don't connect to document or show
div.togglePopover(/*force=*/false); // try to force close

would start throwing, unlike the current algorithm.

Stuff in step 3. of the check popover validity would also start throwing.

[josepharhar]

Ok, I created a PR to implement this: #9451

[rwlbuis]

Ok, I created a PR to implement this: #9451

I have a similar PR for WebKit:
WebKit/WebKit#15333