popover*targetElement IDL attributes can be misleading

[nt1m]

The popover*targetElement won't necessarily point to the popover being toggled for 2 reasons:

• If you have multiple popovertoggletarget/popovershowtarget/popoverhidetarget all set to different values, only the popovertoggletarget is used, then popovershowtarget, then popoverhidetarget.
• The setter is also misleading because if you set it to an element without ID, it just sets the attribute to an empty string

[nt1m]

I think a better design would be either:

• popovertoggletarget/popovershowtarget/popoverhidetarget attributes that just reflect strings (with no validation)
• a single popoverTargetElement attribute which exposes the element used by the engine

cc @josepharhar @mfreed7 @domenic @annevk

[domenic]

We generally want to have a reflecting IDL attribute for every content attribute. So we shouldn't get rid of them. We could add

a single popoverTargetElement attribute which exposes the element used by the engine

though in addition, if that seems useful.

The setter is also misleading because if you set it to an element without ID, it just sets the attribute to an empty string

But, it should still set the attr-associated element, which (I hope) is what the rest of the spec consults, right?

[nt1m]

We generally want to have a reflecting IDL attribute for every content attribute.

The IDL attribute can just reflect the ID string.

which (I hope) is what the rest of the spec consults, right?

That's not the case: https://html.spec.whatwg.org/#popover-target-element

Note that Chrome's implementation is different from the spec:

data:text/html,<button popovershowtarget=popover popoverhidetarget=popover2>dsfdsfdsdfs</button><button popoverhidetarget=popover popovershowtarget=popover2>dsfdsfdsdfs</button><div popover id=popover>dfsdf</div><div popover id=popover2>dfsdfddf</div>

popovershowtarget & popoverhidetarget both seem to work, though according to the spec, only popovershowtarget is supposed to work, since popover in https://html.spec.whatwg.org/#popover-target-attribute-activation-behavior is the one from popovershowtarget.

cc @josepharhar @mfreed7

[domenic]

The IDL attribute can just reflect the ID string.

But it's better if it reflects the actual element.

That's not the case: https://html.spec.whatwg.org/#popover-target-element

Then we should fix that!

[nt1m]

The IDL attribute can just reflect the ID string.

But it's better if it reflects the actual element.

I don't think it makes a lot of sense with the current spec, if someone does:

button.popoverShowTargetElement = foo
button.popoverHideTargetElement = bar

button will only act on foo (it'll act as a toggle for foo).

In Chrome, the button will act on both at once (show foo, hide bar), so maybe it makes more sense with that behavior at least.

[annevk]

I agree with @domenic that Element references are superior. The specification (and Chrome) needs to be fixed to actually use this infrastructure correctly though.

I guess then there is a separate question with regards to what happens when multiple are set. I tend to prefer only activating a single one, in line with label. If it only activates a single one how common would it be that web developers set multiple?

[mfreed7]

There are several things going on here:

1. The example in this comment doesn't use manual popovers, so light dismiss behavior is involved and kind of confuses things. I.e. clicking on unrelated buttons will close some of those popovers, and you also can't have both popovers open at once. Here's a demo that uses manual popovers and has more permutations: https://jsbin.com/voganuz/edit?html,output
2. The behavior of Chromium and the spec was intended to be that if multiple attributes are provided on a single button, popovertoggletarget wins, then popovershowtarget, then popoverhidetarget. If the attributes point to different elements, the "winning" attribute's target is the one and only target. No invoking element should ever open or close more than one popover. Said another way, there's only ever one target popover element.
3. Looking at the example I provided in #1, I believe Chromium's implementation is doing "the right thing".
4. The IDL attributes, as mentioned above, are element references. All of the behavior detailed in #2 should work the same way, whether idref content attributes are used or IDL element references are provided via JS. I have not verified this yet in Chromium, but it should work. I do notice that the spec (here) uses "idref" in some places that seem like they should be using element reference terminology instead. This might be an issue.

There might be bugs in the implementation and/or spec, but the above was the intention.

a single popoverTargetElement attribute which exposes the element used by the engine

though in addition, if that seems useful.

I like this idea a lot - it seems valuable to provide the one target element, regardless of which attribute(s) were used. It must be readonly though, since setting it doesn't tell us which behavior is desired.

[nt1m]

The behavior of Chromium and the spec was intended to be that if multiple attributes are provided on a single button, popovertoggletarget wins, then popovershowtarget, then popoverhidetarget. If the attributes point to different elements, the "winning" attribute's target is the one and only target. No invoking element should ever open or close more than one popover. Said another way, there's only ever one target popover element.

From what I'm seeing in the spec & Chrome, popovertoggletarget wins over popovershowtarget/hidetarget, but popovershowtarget/hidetarget can be used together.

So IIUC, popovershowtarget=foo popoverhidetarget=bar will act like popovertoggletarget=foo and bar is totally ignored. I think this is extremely confusing.

If only one target wins, then popovershowtarget=foo popoverhidetarget=bar should act like popovershowtarget=foo to avoid any confusion.

So I'm suggesting 2 changes:

1. Only one attribute can be work at a time (popovershowtarget/hidetarget cannot work together!)
2. The getters for popoverShowTargetElement/popoverHideTargetElement/popoverToggleTargetElement should at least null out the unused elements at least, to avoid this confusion. I think there's prior art for this, like how we sanitize the popover attribute.

What do you think?

---

(I got confused now, I thought my example here worked, but it did for the wrong reasons: data:text/html,<button popovershowtarget=popover popoverhidetarget=popover2>dsfdsfdsdfs</button><button popoverhidetarget=popover popovershowtarget=popover2>dsfdsfdsdfs</button><div popover id=popover>dfsdf</div><div popover id=popover2>dfsdfddf</div>)

[mfreed7]

4. The IDL attributes, as mentioned above, are element references. All of the behavior detailed in #2 should work the same way, whether idref content attributes are used or IDL element references are provided via JS. I have not verified this yet in Chromium, but it should work. I do notice that the spec (here) uses "idref" in some places that seem like they should be using element reference terminology instead. This might be an issue.

I just added IDL equivalents to the demo, and I believe Chromium's behavior looks ok there also. Please correct me if I'm wrong!

[mfreed7]

So IIUC, popovershowtarget=foo popoverhidetarget=bar will act like popovertoggletarget=foo and bar is totally ignored. I think this is extremely confusing.

If only one target wins, then popovershowtarget=foo popoverhidetarget=bar should act like popovershowtarget=foo to avoid any confusion.

I agree with this statement: if attributes point to different elements, only one "winning" argument works. In the example above, it should behave as if the popoverhidetarget=bar isn't there at all. And in Chromium, I believe, that's exactly how it behaves.

It does seem like we need some spec clarification/fixes here. @josepharhar

[mfreed7]

• Only one attribute can be work at a time (popovershowtarget/hidetarget cannot work together!)

The only intended case where both can work together is this one:

<button popovershowtarget=foo popoverhidetarget=foo>Show and hide foo</button>

In that case, since they point to the same element, that should behave like a toggle. It's semantically the same - "toggle" both hides and shows an element.

• The getters for popoverShowTargetElement/popoverHideTargetElement/popoverToggleTargetElement should at least null out the unused elements at least, to avoid this confusion. I think there's prior art for this, like how we sanitize the popover attribute.

I agree with this - it would make it easier for developers to understand what's going on. But @domenic @annevk there might be spec issues for it? Perhaps not. If it's spec-possible, I'm in favor.

[annevk]

As far as I can tell we'd need to create an extension for "reflect" so you can run some custom setter steps (that would null out the other references). It's possible, but novel. And might create a nice merge conflict for #8496.

[nt1m]

The only intended case where both can work together is this one:

<button popovershowtarget=foo popoverhidetarget=foo>Show and hide foo</button>

In that case, since they point to the same element, that should behave like a toggle. It's semantically the same - "toggle" both hides and shows an element.

It doesn't read the same to me. To me, this reads as:

foo.showPopover()
foo.hidePopover()

whereas popovertoggletarget=foo reads as:

if (!foo.matches(":open"))
  foo.showPopover();
else
  foo.hidePopover();

@mfreed7 I'd prefer to not special case the case where popovershowtarget has the same value as popoverhidetarget.

[nt1m]

<button popovershowtarget=foo popoverhidetarget=foo>Show and hide foo</button> reads as such because that's my first intuition when reading <button popovershowtarget=foo popoverhidetarget=bar>Button</button>. E.g.

foo.showPopover()
bar.hidePopover()

Even though it doesn't behave that way, I think special casing the case where both attribute values is equal just adds more confusion.

[mfreed7]

<button popovershowtarget=foo popoverhidetarget=foo>Show and hide foo</button> reads as such because that's my first intuition when reading <button popovershowtarget=foo popoverhidetarget=bar>Button</button>. E.g.

foo.showPopover()
bar.hidePopover()

Even though it doesn't behave that way, I think special casing the case where both attribute values is equal just adds more confusion.

So popovershowtarget actually has to behave like this:

if (!foo.matches(":open"))
  foo.showPopover();

otherwise, it'd have to throw an exception if the popover was already open.

But I don't disagree that it's a bit confusing when more than one attribute is in use at a time. So you're proposing to only "allow" one of the three attributes at a time, right? What happens if more than one is used, just no behavior at all and perhaps a console warning? If so, I don't think that breaks any "real" use cases, but @jh3y can you double-check me? If not, I think I'm supportive of this change. I suppose in this case we'd also want all three IDL reflections to return null?

[annevk]

@nt1m and I discussed this and it would be rather involved to keep the current design and use reflect. We'd essentially need custom getter steps (to return null if there are multiple attributes set) and custom setter steps (to null out the other internal slots and delete the other attributes).

Could we perhaps consider an alternative design with two attributes:

• popovertarget (takes an ID)
• popovertargetaction (enumerated attribute of "show", "hide", "toggle" (default))

This is slightly more verbose, though less verbose in what is likely the common case. And it fits a lot better with existing attribute patterns and reflect.

[mfreed7]

@nt1m and I discussed this and it would be rather involved to keep the current design and use reflect. We'd essentially need custom getter steps (to return null if there are multiple attributes set) and custom setter steps (to null out the other internal slots and delete the other attributes).

Yep, that's a good point. That'd apply to Chromium also.

Could we perhaps consider an alternative design with two attributes:

• popovertarget (takes an ID)
• popovertargetaction (enumerated attribute of "show", "hide", "toggle" (default))

This is slightly more verbose, though less verbose in what is likely the common case. And it fits a lot better with existing attribute patterns and reflect.

Just to clarify the proposal, the above two are the content attributes. For IDL:

• popoverTargetElement reflects (via Element reflection) the popovertarget content attribute
• popoverTargetAction is a DOMString? IDL attribute, limited to known values of "show", "hide", and "toggle", with Empty, Invalid, and Missing value defaults all being "toggle"?

Does that sound like what you're proposing?

[nt1m]

popoverTargetAction is a DOMString? IDL attribute, limited to known values of "show", "hide", and "toggle", with Empty, Invalid, and Missing value defaults all being "toggle"?

Maybe we need a none value to represent the missing/invalid (and potentially empty) states at least.

[nt1m]

(but yes the rest is accurate)

[nt1m]

Thinking more, it's probably fine too if the default is toggle, I'll leave it to @annevk.

I was mostly thinking of the case where both attributes weren't set, but I guess the user activation behavior can just check if popovertarget is present.

[mfreed7]

Thinking more, it's probably fine too if the default is toggle, I'll leave it to @annevk.

The reason I didn't include a none state was that in that case you have a popoverTargetElement but nothing happens, which feels weird. Defaulting always to toggle behavior felt like the least footgun-y thing. Maybe I'm wrong.

I was mostly thinking of the case where both attributes weren't set, but I guess the user activation behavior can just check if popovertarget is present.

Right - if you don't have a popovertarget attribute, or it doesn't point to a valid element that contains the popover attribute, then nothing should happen when you click the button. But I don't think that should affect the value of popoverTargetAction.

[annevk]

@mfreed7 I agree with you, no need for a none state. The only slight change to your earlier comment is that the type would be DOMString. There is no need for it to be nullable.

If you could make it more explicit whether that works for you all I'm happy to do the spec work (middle of next week most likely).

[mfreed7]

@mfreed7 I agree with you, no need for a none state. The only slight change to your earlier comment is that the type would be DOMString. There is no need for it to be nullable.

Yep, good catch, I'm not sure why I made it nullable, since I then said the missing value default was "toggle".

If you could make it more explicit whether that works for you all I'm happy to do the spec work (middle of next week most likely).

I'm convinced that this is a better shape for this feature, and I've put up a PR to change Chromium accordingly. That includes new WPTs that test the new behavior. The WPT PR isn't up yet, but it should be later today.

I appreciate you offering to do the corresponding spec work, thanks!

[mfreed7]

I'm convinced that this is a better shape for this feature, and I've put up a PR to change Chromium accordingly. That includes new WPTs that test the new behavior. The WPT PR isn't up yet, but it should be later today.

WPT updates are here: web-platform-tests/wpt#38701