open-ui icon indicating copy to clipboard operation
open-ui copied to clipboard

[Popup] New IDL for Pop-Up content attributes, which allow Element references

Open e111077 opened this issue 2 years ago • 29 comments

NOTE: this issue was originally written when the popup proposal still included a <popup> element. It has since been edited, but there may be some inconsistencies

The problem

The current Popup explainer seems to be missing some imperative API docs for certain features such as the anchor and togglepopup attributes.

The background

The reason why an imperative API would help is that ID's for associating elements leads into multiple problems:

  • IDs do not penetrate shadow roots
  • componentizing a popup button or a popup itself will require the component author to generate IDs on the fly

For example, shadow roots:

<button togglepopup="my-popup" id="my-anchor">toggle the popup</button>
<my-popup>
  <template shadowroot="open">
    <div popup id="my-popup" anchor="my-anchor">
      <slot></slot>
    </div>
  </template>
</my-popup>

The above example has an anchor with id my-anchor and a popup inside of a shadow root (using the declarative shadow dom api). Adding [anchor=my-anchor] to div[popup] will not work as ID's cannot penetrate shadow roots, and button[togglepopup=my-popup] will also not work because it won't be able to penetrate the shadow root to find the popup.

Example ID generation:

// PopupButton.jsx
import React, { useState } from "react";
let idCounter = 0;

export default () => {
  const [currentId] = useState(idCounter++);

  return (
    <>
      <button togglepopup={`popup-button-${currentId}`}>
        Click me to open popup
      </button>
      <div popup id={`popup-button-${currentId}`}>...</div popup>
    </>
  );
};

The above example in react would require generating an ID to link a popup button with a popup (same situation would exist with anchor) when an imperative solution (like a ref or a function callback) would suffice.

Proposed solution

Defining an imperative way to associate element's popup or anchor.

I see two options here:

  1. Allowing anchor and togglepopup properties on HTMLElement to support element references as well as string
  2. Creating a setter method for setAnchor(el: HTMLElement) and togglePopUp(el: HTMLPopupElement)

Both of these options solve the problems above, but have different tradeoffs.

Allowing property to support Element references

The positives about this approach is that it will allow for better declarative programming on modern templating frameworks such as React and lit-html. For example:

React:

// PopupButton.jsx
import React, { useRef } from "react";

export default ({anochorIdOrRef} /* type is string|HTMLElement|undefined */) => {
  const [popupRef] = useRef(undefined);

  return (
    <>
      <button togglepopup={popupRef.current}>
        Click me to open popup
      </button>
      <div popup ref={popupRef} anchor={anochorIdOrRef}>...</div>
    </>
  );
};

// usage: <PopupButton anchorIdOrRef="my-anchor" />

Lit:

note: the .propName=${val} syntax denotes a property binding not an attribute binding e.g. el.propName = val rather than el.setAttribute('propName', val)

// popup-button.js
import {LitElement, html} from 'lit';

class PopupButton extends LitElement {
  get popupEl() { this.shadowRoot?.querySelector('popup') ?? undefined; }
  static properties = {anchorEl: {attribute: false}};
  anchorEl = undefined; // type is HTMLElement|undefined

  render() {
    return html`
      <button .togglepopup=${this.popupEl}>
        Click me to open popup
      </button>
      <div popup .anchor=${this.anchorEl}>...</div>`;
  }
}

customElements.define('popup-button', PopupButton);

// usage: <popup-button .anchor=${document.body.querySelector('#my-anchor')}></popup-button>

The negatives are that there is current no precedent that I know of for an element property accepting both a string, or an element reference.

Function setters

The Positives about this approach are that it will enable all these use cases and it fits with precedent (not having a single property accept HTMLElement or string)

The Negatives requires component authors to wire things up a bit more. e.g.

React:

// PopupButton.jsx
import React, { useRef, useCallback } from "react";

export default ({anchorIdOrRef}) => {
  const popupRef = useRef();
  const buttonRef = useCallback(buttonEl => {
    buttonEl.togglePopUp(popupRef.current);
  }, [popupRef]);

  useCallback(() => {}, []);

  return (
    <>
      <button ref={buttonRef}>
        Click me to open popup
      </button>
      <div popup ref={popupRef} anchor={anchorIdOrRef}>...</div>
    </>
  );
};

Lit:

// popup-button.js
import {LitElement, html} from 'lit';

class PopupButton extends LitElement {
  get buttonEl() { this.shadowRoot?.querySelector('button') ?? undefined; }
  get popupEl() { this.shadowRoot?.querySelector('popup') ?? undefined; }
  static properties = {anchorEl: {attribute: false}};
  anchorEl = undefined;

  render() {
    return html`
      <button>
        Click me to open popup
      </button>
      <div popup>...</div popup>`;
  }
  
  firstUpdated() {
    this.buttonEl.togglePopUp(this.popupEl);
  }
  
  updated(changed) {
    if (changed.has('anchorEl')) {
      this.popupEl.setAnchor(this.anchorEl);
    }
  }
}

customElements.define('popup-button', PopupButton);

Acceptance criteria

Some sort of element-reference imperative API for anchor is defined, or something that makes it easy to componentize HTMLElement[popup], especially in Shadow DOM

e111077 avatar Aug 04 '21 21:08 e111077

This is in reference to the discussion on #357

Additionally, by not declaratively linking two components via IDs and general ID incompatibilities with shadow dom, this means it might require strong support from #329 likely via role and aria-*

e111077 avatar Aug 04 '21 21:08 e111077

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.

github-actions[bot] avatar Mar 04 '22 00:03 github-actions[bot]

Thanks for opening this issue and enumerating the various possible courses of action. Sorry there's been such a delay. But I'm in the process of implementing a prototype of the new Popup API and I got to the part where I might implement this bit.

Of your two approaches, it sounds like you think #1 is the most developer-friendly. If so, I agree. I'm in favor of just allowing the anchor and popup (actually now it's called ~~triggerpopup~~ togglepopup) properties to directly accept an Element or a DOMString. While there isn't a precedent of this exact combination, there are definitely other WebIDL attributes that accept multiple types. So I don't think that's too much of an issue.

The open issues that I can see are:

  1. This might (?) be the first case where we're allowing a cross-shadow reference between elements like this, and I'm worried that there might be some kind of a shadow element leak. I can't think of how, since to hook things up, you already need references to both sides of the "linkage". But I'd like smarter people than myself to think about this and confirm.

  2. What happens in this case?

<button>Click me</button>
<div popup=popup>I'm a popup without an id</div>

<script>
const button = document.querySelector('button');
button.triggerPopup = document.querySelector('[popup]');
const attr = button.getAttribute('togglepopup'); // What does this return?
</script>

mfreed7 avatar Mar 30 '22 00:03 mfreed7

Heya no worries on the timing, just happy to hear this is getting some love!

  1. I think this specific manner may be first of its kind (via a setter) as most I've seen are references via getters with a clear point of diassociation. e.g. HTMLInputElement.form will return the associated HTMLFormElement but disassociates on element disconnect or <label>'s .control property. I think the closest that comes to this is the Accessibility Object Model currently under work (explainer link)
  2. It should return the current attribute (null) like <input> treats value's attribute vs property.

e111077 avatar Mar 30 '22 02:03 e111077

The Open UI Community Group just discussed Explainer is missing imperative API for anchor and popup attributes.

The full IRC log of that discussion <dbaron> Topic: Explainer is missing imperative API for anchor and popup attributes
<dbaron> github: https://github.com/openui/open-ui/issues/382
<emilio> q+
<JonathanNeal> q?
<josephrexme> masonf: To set the state for #382, we have 2 attributes - anchor and togglepopup. Should there be a corresponding attribute you can use in JavaScript. Should you be able to pass in elements to the JavaScript API?
<josephrexme> masonf: I'm curious if we should be doing this yet? I am interested in everyone's thoughts
<josephrexme> bkardell_: I think any controversy that remains on those has nothing to do should not prevent you from proposing something
<josephrexme> emilio: I agree that if we should we should have elements, it should be separate from the attribute name. showpopups do get an element as you open them but you cannot change it dynamically
<josephrexme> emilio:I don't think there's a use case for switching anchors dynamically
<josephrexme> masonf: showPopups only lets you change the anchor when you open them?
<JonathanNeal> q?
<josephrexme> emilio: right
<emilio> ack emilio

css-meeting-bot avatar Apr 14 '22 18:04 css-meeting-bot

For reference, as a point of prior art, the anchor in XUL popups is specified only on open, as an element reference:

  • https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/dom/webidl/XULPopupElement.webidl#78

The element has a readonly getter for that (that returns non-null while open):

  • https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/dom/webidl/XULPopupElement.webidl#174

Reading the original test-case in https://github.com/openui/open-ui/issues/382#issue-961097192, I'd also note that it is probably reasonable to let popups look up the containing DOM tree. The problematic case is digging into shadow trees for IDrefs (which is a no-go). Things like nested <svg:use> already look into containing trees, for the record.

emilio avatar Apr 14 '22 18:04 emilio

For reference, as a point of prior art, the anchor in XUL popups is specified only on open, as an element reference:

  • https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/dom/webidl/XULPopupElement.webidl#78

The element has a readonly getter for that (that returns non-null while open):

  • https://searchfox.org/mozilla-central/rev/d34f9713ae128a3138c2b70d8041a535f1049d19/dom/webidl/XULPopupElement.webidl#174

Reading the original test-case in #382 (comment), I'd also note that it is probably reasonable to let popups look up the containing DOM tree. The problematic case is digging into shadow trees for IDrefs (which is a no-go). Things like nested <svg:use> already look into containing trees, for the record.

Thanks for this comment (and the discussion just now). A few comments/counterpoints:

  • As much as possible, one of the goals for this API is to enable functionality without requiring Javascript. In fact, that's the entire purpose of the togglepopup attribute. I don't think there'd be a point in that attribute if it was required as part of .showPopup(). Perhaps your point is a good one w.r.t. the anchor attribute though.
  • Another counterpoint for anchor is that we're hoping to re-use this attribute as part of the anchor positioning proposal, and the way that's going, it might not require things to be in the top layer. If so, I'd hate to tie anchor back to the .showPopup() call.
  • Allowing a shadow-contained element to "connect" to an IDref in a containing tree. I hadn't thought about that, but you're right it's a possibility. That might even open up most of the typical use cases? I am a bit hesitant to follow the lead of <svg:use> just given how different that one is. But ok. This might be a way around allowing assignment of element references in order to (basically) support shadow dom.

mfreed7 avatar Apr 14 '22 19:04 mfreed7

For looking up the tree for idrefs, I think the most-reasonable behavior is perhaps like you said not to go digging in shadow roots, but it sounds vaguely similar to :host-context() and may run into the same concerns Emilio raised in w3c/csswg-drafts#1914.

But limiting anchor / idref lookup only to the parent tree (please lmk if I'm misunderstanding your suggestion) might run into issues such as the use case of using a singleton popup element, which is what we ended up doing at YouTube, since the number of inert / unused popup tooltips on a site that large was starting to cause performance problems.

Though, if this proposal fixes the stacking context problem that required popups to be hoisted to the end of <body> it might be doable to write a simple JS function to dynamically append a popup element inside the fed element reference.

e111077 avatar Apr 14 '22 22:04 e111077

I'm curious about whether there's a use case for changing the target anchor while the popup is already open (and whether hiding and reopening the popup is not an option / acceptable workaround, assuming that's an edge case).

Reason for that question is that it can change the different trade-offs significantly. For example, looking up idrefs in arbitrary trees "up" is trivial with such a model (you just go up calling getElementById just before opening the popup). It's however non-trivial if we need to track all those mutations (because suddenly an idref depends on mutations in various dom trees and so).

emilio avatar Apr 14 '22 23:04 emilio

one use case is the reuse of a common component (popup) that can be invoked by different triggers.

for instance, i see this a lot with react applications where there is a single popup that is reused across multiple instances and the contents of the popup, even if they don't necessarily change - e.g., menu items - are associated with a different invoking element and it's parent context.

For instance, a table/grid that contains rows of data where there is a menu button that invokes a menu popup that allows a user to do various actions. A single popup component can be reused for each of these invoking buttons. Additionally, a common use case here is for these menu popups to contain a delete menu item. In these cases an alternate "anchor" element needs to be specified to ensure that focus does not return to the body - which commonly results in screen reader users being sent back to the top of the document and needing to re-navigate the page to return to where they last left off.

Whether or not both of these situations is exactly the intent of the anchoring mechanism, it is at least related to what authors need to do now, and have expressed frustration over, not to mention the fact that users who need this mechanism in place are left to wait for custom implementations.

scottaohara avatar Apr 15 '22 01:04 scottaohara

@scottaohara sure, but I don't see how that would change the anchor while the popup is open, right? What am I missing?

emilio avatar Apr 15 '22 06:04 emilio

Ah, that could be useful for what people have referred to as info tours / teaching ui. Where a popup points to different ui elements and there are “previous” and “next” buttons in the popup to have its content change and it be associated with those different ui.

I suppose you could have the popup close and a new one reopen with its new anchor instead?

The more I think on this, this is the only use case that comes to mind. The idea of wanting to have a single popup and the potential to animate it between the different anchor points as its content changed per its new association.

scottaohara avatar Apr 15 '22 11:04 scottaohara

To summarize the state of this issue, here are the open questions, with my opinions mixed in for good measure:

  1. What is the behavior of the anchor and togglepopup content attributes - should it "look up the tree" for the provided idref? Should it be dynamic, in that you can change the idref and have it take effect? It seems like there are use cases for changing the idref dynamically (the teaching ui example), and it also seems like "looking up the tree" might be the right behavior.
  2. It seems that there is rough consensus that if it is possible to provide elements directly via IDL (instead of providing an idref), that should be accessed through different IDL attributes, like anchorElement and togglePopupElement.
  3. Given #2, I think we can ship anchor and togglePopup IDL attributes that just allow changing the idref, while we continue to talk about how anchorElement and togglePopupElement should behave.

Any objections to #3? I'm inclined to try to get at least that resolution.

As for #1, I'm also inclined to try to get a resolution that it should be possible to dynamically change idrefs. As for the "look up the tree" behavior, I think that needs more thought/discussion.

mfreed7 avatar Jun 01 '22 16:06 mfreed7

The Open UI Community Group just discussed [Popup] Imperative API for anchor and togglepopup attributes #382, and agreed to the following:

  • RESOLVED: the values for anchor, togglepopup, showpopup, and hidepopup attributes should be able to be changed at any time.
  • RESOLVED: support directly reflecting the (string) value of the anchor, togglepopup, showpopup, and hidepopup attributes via IDL properties of the same names ('anchor', 'togglePopup','showPopup','hidePopup').
The full IRC log of that discussion <dandclark> Topic: [Popup] Imperative API for anchor and togglepopup attributes #382
<dandclark> github: https://github.com/openui/open-ui/issues/382
<dandclark> masonf: This has a number of sub issues. Again it's about idl attributes.
<dandclark> masonf: Corresponding idl for anchor, showpopup, hidepopup
<dandclark> masonf: There arent JS props for these yet.
<dandclark> masonf: First sub q is, implicitly right now you can change any of these prop values on the fly.
<dandclark> masonf: Can call setAttribute() to set/change and it just works. Should it?
<dandclark> masonf: Or should be fixed when it's parsed?
<dandclark> masonf: I think there are good use cases for letting it be changed. e.g. generalized popup component. And corresponding case where you have a button that can hook up to multiple popups.
<chrisdholt> q+
<JonathanNeal> <button popup="selector(:scope + *)" />
<dandclark> chrisdholt: I agree, that's pretty common use case.
<JonathanNeal> q+
<dandclark> chrisdholt: Would be headache not to have it.
<JonathanNeal> ack chrisdholt
<una> I could see this being useful also
<dandclark> JonathanNeal: Does this mean that the same prop accepts a string or a node?
<dandclark> mason: That's sub issue 2 :)
<dandclark> s/mason/masonf
<JonathanNeal> ack JonathanNeal
<una> q+
<JonathanNeal> ack una
<dandclark> una: I could see this being common use case. Dynamically need to change what's being popped up.
<dandclark> una: Or even create new elements (e.g. tooltips)
<JonathanNeal> +1 to being able to update the value on the fly. HTML streams sometimes.
<masonf> Proposed resolution: the values for anchor, togglepopup, showpopup, and hidepopup attributes should be able to be changed at any time.
<JonathanNeal> document.write stuffs.
<JonathanNeal> q?
<chrisdholt> Counting +3 right now with no queue :)
<JonathanNeal> +1
<chrisdholt> +1 to proposed resolution
<una> +1
<masonf> RESOLVED: the values for anchor, togglepopup, showpopup, and hidepopup attributes should be able to be changed at any time.
<dandclark> masonf: Part 2 is that JonathanNeal brought up. Should I be able to say `myAnchor = otherElement`? Vs string that sets the ID ref.
<JonathanNeal> q+
<dandclark> masonf: If we allow to set element directly (vs ID), that should be via a different attribute name.
<dandclark> masonf: If you want to be able to set element, should be different IDL property so that it's clear what you're getting.
<dandclark> masonf: There was discussion that there are other cases this is being considered. Conclusions reached elsewhere were that you should use different attr names.
<dandclark> JonathanNeal: I have 1 question. In current state of AOM, is there resolution that also would set art prior art for this?
<dandclark> masonf: I think that was the context.
<JonathanNeal> ack JonathanNeal
<dandclark> scotto_: That's the context. That's the bit of aom that is still in discussionl.
<JonathanNeal> q+
<dandclark> scotto_: Being able to specify that element owns other element or is related, it's strightforward to do that with id refs. But direct references, there's discussion about needing something diferent with that. If attr is sent back to dom (e.g. with reflection), this would be a different way. Wouldn't work like ARIA owns.
<JonathanNeal> ack dbaron
<dandclark> dbaron: A few things that seem difficult about 2 prop case. (1) introspection. How do you look at the props to understand what you've created?
<dandclark> dbaron: anchor prop and anchorElement prop, if you set anchorElement, does anchor change? If so how? No, how do you look at them to tell what situation is?
<dandclark> dbaron: Whichever last should win? If so, does it clear the other? Gets messy quickly.
<dandclark> dbaron: Maybe someone found less messy solution, but I can't think of one.
<tantek> +1 dbaron questions. which (if either?) takes precedence? what (if any) automatic side-effects are there between the two?
<dandclark> masonf: I agree, this is why it isn't clear. But other route of allowing 1 prop, you have similar q's.
<JonathanNeal> +1 to not making the property polymorphic
<dandclark> masonf: Setting anchor to an ID, what does the contetn attr have? If I set ID ref, can I get back the element?
<dandclark> dbaron: If don't have ID attribute, what to put in the string? Or cases where multiple elements share an ID.
<dandclark> masonf: Want to push for supporting the string reflection only becasue of these questions.
<dandclark> JonathanNeal: Lazy web dev doesn't want to generate IDs. Uses the title of the next button. Does this work?
<dandclark> masonf: No. Element targeted needs ID.
<dandclark> JonathanNeal: Do we want it to work?
<dandclark> masonf: Using title attribute instead?
<dbaron> (For what it's worth, I think where my questions were leading was basically "there's a reason nothing else on the platform does this".)
<dandclark> JonathanNeal: Specific kind of fragement id. Different from regular identifier. Feature in chromium browsers, can link to fragment on page by text in page. Referring to reference done this way.
<dandclark> masonf: That's for link navigation
<JonathanNeal> q?
<dandclark> masonf: There's a separate issue about that. My first answer is that would be complicated.
<dandclark> JonathanNeal: for folks who don't want to create IDs but want to do this, would there be an ability to use keyword to set it by a selector?
<dandclark> masonf: Could brainstorm that. Knee-jerk reaction is that if you're in this case you're not in a position to use these.
<dbaron> https://github.com/WICG/scroll-to-text-fragment/blob/main/README.md
<dandclark> masonf: If you're getting into more complicated cases then JS might be more appropriate.
<dandclark> JonathanNeal: Thinking of assigning hover for highlighted text. Take a word, give it popup attribute with a direction to point to next element. Avoiding an ID would be beneficial.
<dandclark> masonf: In that case you already need JS to wrap selected text in span so it has an element
<dandclark> JonathanNeal. Right. This is more about the manner in which I was authoring HTML where I don't know my ID space. I understand that is maybe not currently easy to do anywhere else, so not expected.
<JonathanNeal> q?
<masonf> Proposed resolution: support directly reflecting the (string) value of the anchor, togglepopup, showpopup, and hidepopup attributes via IDL properties of the same names ('anchor', 'togglePopup','showPopup','hidePopup'). Do not support setting elements directly via these IDL properties.
<dandclark> JonathanNeal: What is the string value? The ID.
<dandclark> masonf: It's the ID.
<dandclark> masonf: Can say togglepopup='foo', 'foo' is the ID of another element in the DOM.
<chrisdholt> q+
<JonathanNeal> ack JonathanNeal
<dandclark> JonathanNeal: Con is if we ever need to allow thigns without ID, we'll need to solve it however aom is doing it.
<JonathanNeal> ack chrisdholt
<dandclark> masonf: Yes, that solution would apply in other places too.
<dandclark> chrisdholt: Maybe drop second sentence.
<masonf> Proposed resolution: support directly reflecting the (string) value of the anchor, togglepopup, showpopup, and hidepopup attributes via IDL properties of the same names ('anchor', 'togglePopup','showPopup','hidePopup').
<JonathanNeal> +1
<chrisdholt> +1
<dandclark> chrisdholt: Let' s not explicitly say no since we haven't fully discussed. But first part seems reasonable.
<JonathanNeal> q?
<scotto_> +1
<masonf> RESOLVED: support directly reflecting the (string) value of the anchor, togglepopup, showpopup, and hidepopup attributes via IDL properties of the same names ('anchor', 'togglePopup','showPopup','hidePopup').
<dandclark> Zakim, end meeting
<Zakim> As of this point the attendees have been dbaron, dandclark, JonathanNeal, scotto_, masonf, chrisdholt, rodydavis, una, tantek
<Zakim> RRSAgent, please draft minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2022/06/16-openui-minutes.html Zakim
<Zakim> I am happy to have been of service, dandclark; please remember to excuse RRSAgent. Goodbye

css-meeting-bot avatar Jun 16 '22 20:06 css-meeting-bot

So I should have realized this, but combining the resolution above with the conversation here, we can't do both. Essentially, we've decided:

  • The function to show the popup is myPopup.showPopUp();.
  • The IDL to set the showpopup attribute is myPopup.showPopUp = 'idref';

Same goes for hidePopUp. We need new names. I suggest that the things that we rename be the new IDL attributes we resolved to add above, and not the showPopUp()/hidePopUp functions, since those are much more commonly-used.

One idea is:

myPopup.togglePopUpAttr = 'idref';
myPopup.showPopUpAttr = 'idref';
myPopup.hidePopUpAttr = 'idref';

Thoughts? @domenic

mfreed7 avatar Jun 23 '22 23:06 mfreed7

IDL attribute names really need to match content attribute names. So please don't do this Attr suffix thing. Either rename both the content and IDL attributes, or rename the methods.

domenic avatar Jun 24 '22 00:06 domenic

IDL attribute names really need to match content attribute names. So please don't do this Attr suffix thing. Either rename both the content and IDL attributes, or rename the methods.

I swear, naming is the hardest part of this pop-up API.

I'm putting this back on the Agenda to discuss. My new proposal is that we do not provide IDL reflections of the string values for the togglepopup, showpopup, and hidepopup content attributes. In the future, we might (as discussed above) provide a set of IDL attributes that allow setting the elements directly (not through idrefs), but that would anyway have a different name.

The OP of this issue is really about allowing element references directly. The resolution above, which allows access to the idref strings didn't solve that problem. And I'm not sure it solves any problem. What is the use case for setting these idref values through JS? The explicit point of togglepopup, showpopup, and hidepopup content attributes is to allow a declarative (in HTML) connection between an invoking element and a pop-up. If you're using Javascript already, would you not just use the element.showPopUp() and element.hidePopUp() functions to show and hide the pop-up as you'd like? Or if you really want to use the content attributes, you can use element.setAttribute('showpopup','idref');.

I'm afraid that by renaming togglepopup, showpopup, and hidepopup content attributes, we'll make them harder to understand and use in the primary (HTML) use case. We spent some significant time getting to good names and behaviors for these attributes, I think.

mfreed7 avatar Jun 24 '22 15:06 mfreed7

All content attributes need to have IDL reflections!! I'm sorry this is hard, but these are basic HTML design principles. Please do not violate them.

domenic avatar Jun 24 '22 15:06 domenic

All content attributes need to have IDL reflections!! I'm sorry this is hard, but these are basic HTML design principles. Please do not violate them.

Two options on the table, then:

  • Rename showPopUp() and hidePopUp(), or
  • Rename the showpopup and hidepopup (and likely togglepopup) content attributes.

Preferences? Ideas?

mfreed7 avatar Jun 24 '22 17:06 mfreed7

My backup plan, absent a better suggestion, is to rename the activation functions as:

element.showThePopUpOnTheScreenRightNowPlease() element.couldYouPleaseHideThisPopUpNow()

since they won't clash with anything.

mfreed7 avatar Jun 24 '22 17:06 mfreed7

Preferences to renaming the methods.

Ah! You kid but they have the benefit of being the upmost explicit 😉

I'm guessing alternatives could be (open|close)PopUp() but we'd loose the "show/hide" parallel with the content attributes (and therefore the IDL reflections as well).

Or (open|close)AsPopUp() maybe ?

I'd have to brainstorm a bit more for other possible alternatives. The only solution allowing us to keep the "show/hide" parallel would be elongating the method names anyway.

VicGUTT avatar Jun 24 '22 19:06 VicGUTT

Heya, sorry to detract from the naming conversation, I've updated the issue to be in sync with the latest proposal of popup now being an attribute.

But looking at Mason's earlier comment idrefs are still quite a hassle especially in SD since in some cases the lookup would not necessarily be in the parent tree. When we implemented YouTube tooltip we had to make a singleton tooltip that would be reused as the sheer number of hidden tooltip nodes across the site was causing significant performance issue. Simply converting the YT's situation to this proposal it would look like this:

<yt-app-shell>
  # SR
    <yt-text-with-tooltip id="a"></yt-text-with-tooltip>
    <yt-text-with-tooltip id="b"></yt-text-with-tooltip>
    <yt-text-with-tooltip id="c"></yt-text-with-tooltip>
    <!-- showPopUp or attribute added or removed dynamically on the above nodes -->
    <yt-reusable-component>
      # SR
        <!-- shadow roots can reuse id's -->
        <third-party-component-that-needs-a-tooltip id="a" />
      #/SR
    </yt-reusable-component>
  #/SR
</yt-app-shell>
<singleton-popup>
  # SR
    <div popup id="singleton-popup" anchor="b">
      <slot></slot>
    </div>
  #/SR
  <!-- dynamically-inserted content -->
</singleton-popup>

Would you recommend that for the situation above one would need to dynamically render a <singleton-popup> (no longer singleton i guess) in each shadow root of each owned component that requires a tooltip and as a sibling to any third-party components in which the shadow root cannot be modified? e.g.

<yt-app-shell>
  # SR
    <yt-text-with-tooltip id="a">
      # SR
        ...
        <!-- dynamcially render <yt-popup anchor=${this.id}> and call show|hidePopup() -->
      #/SR
    </yt-text-with-tooltip>
    <yt-text-with-tooltip id="b">
      # SR
        ...
        <!-- dynamcially render <yt-popup anchor=${this.id}> and call show|hidePopup() -->
      #/SR
    </yt-text-with-tooltip>
    <yt-text-with-tooltip id="c">
      # SR
        ...
        <!-- dynamcially render <yt-popup anchor=${this.id}> and call show|hidePopup() -->
      #/SR
    </yt-text-with-tooltip>
    <!-- showPopUp or attribute added or removed dynamically on the above nodes -->
    <yt-reusable-component>
      # SR
        <!-- shadow roots can reuse id's -->
        <third-party-component-that-needs-a-tooltip id="a" />
        <!-- dynamcially render <yt-popup anchor="a"> and call show|hidePopup() -->
      #/SR
    </yt-reusable-component>
  #/SR
</yt-app-shell>

<!-- example definition of yt-popup -->
<yt-popup>
  # SR
    <!-- yt-popup exports the div's show|hide|togglePopup methods and forwards the anchor attribute to the div -->
    <div popup anchor="${this.anchor}">
      <slot></slot>
    </div>
  #/SR
  <!-- dynamically-inserted content -->
</yt-popup>

In general it might be a bit of a hassle for setup, but is doable for this use case.

Though this solution would not be satisfactory for the "tutorial of this site" setup where you'd want to animate a singleton popup between steps. You'd still run into the initial case where the the popup would need to reference an ID across shadow roots that are not a sibling or in ancestor chain. Though, who knows whether you'd be able to even animate the position if the anchor changes

Also I'd be a bit weary about introducing a new SD idref lookup algorithm especially since the Acessibility Object Model proposal is going the route of passing down / exposing "aria-ble" elements via idref. I'd argue that it'd be sorta dissonant having to go into a completely different mental model for idref lookup for popup since idrefs in this case behave differently from aria-* by penetrating SRs up the ancestor chain – though one can maybe argue this sets up a precedent acting similar to position: relative?

e111077 avatar Jun 24 '22 21:06 e111077

I feel like it should be possible to come up with a reasonable naming distinction that distinguishes between element.showPopUp() and element.hidePopUp() (which are methods to be called on the popup element) and the togglepopup/showpopup/hidepopup attributes (which are attributes that go on the popup's triggering element).

A few possibilities that I can think of (where I assume that we rename the two methods or the three attributes equivalently, and only describe one of the changes, except as noted):

  1. rename togglepopup to popuptotoggle (etc.)
  2. rename togglepopup to popuptarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget.
  3. rename element.showPopUp() to element.popUpShow() (etc.)

...though I don't particularly like any of them (but maybe dislike 2 the least).

dbaron avatar Jul 08 '22 12:07 dbaron

I like this approach from @dbaron:

rename togglepopup to popuptarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget.

as it is nice and short and relatively easy to read.

In general I feel it would be great to avoid synonyms for showing/hiding and work around in ways like the ones @dbaron describes.

hidde avatar Jul 14 '22 13:07 hidde

@hidde Agreed

VicGUTT avatar Jul 14 '22 15:07 VicGUTT

The Open UI Community Group just discussed [Popup] Imperative API for anchor and togglepopup attributes, and agreed to the following:

  • RESOLVED: rename togglepopup to popuptoggletarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget. Reflect the string value of these three content attributes in IDL as .popUpToggleTarget, .popUpShowTarget, and .popUpHideTarget.
The full IRC log of that discussion <gregwhitworth> Topic: [Popup] Imperative API for anchor and togglepopup attributes
<gregwhitworth> github: https://github.com/openui/open-ui/issues/382
<gregwhitworth> q?
<hdv> masonf: ok, 382 is a big issue by the number of things, but one is the most important now: the conversation for today would be about renaming
<hdv> masonf: there are three invoking attributes now, they need to be reflected in IDL, but the problem is that conflicts because there are also JS methods with the same name
<Travis> [please]showPopup()
<Travis> +1
<hdv> masonf: there are some suggestions, the one I liked most was to rename to popuptarget, popupshowtarget, popuphidetarget, a little more descriptive and doesn't conflict
<hdv> gregwhitworth: what were they prior?
<hdv> masonf: showpopup, hidepopup, togglepopup
<hdv> gregwhitworth: I like the dbaron's suggestions way better, +100 from me
<bkardell_> Travis: only if this fires a [youareWelcome]popup event
<hdv> +1
<masonf> Proposed resolution: rename togglepopup to popuptarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget. Reflect the string value of these three content attributes in IDL as .popUpTarget, .popUpShowTarget, and .popUpHideTarget.
<hdv> Travis: are you suggesting both updating IDL names and content attribute names?
<scotto_> q+
<hdv> masonf: yes technically we don't have IDL reflections just yet
<gregwhitworth> ack scotto_
<masonf> popuptoggletarget
<masonf> ?
<hdv> scotto_: to be honest, I don't really care, but… popuptarget doesn't say toggle to me
<hdv> masonf: we could do popuptoggletarget?
<hdv> +1 to scott's idea actually, it is German Naming Convention ish
<Travis> I don't know, minified code can be pretty small...
<masonf> Proposed resolution: rename togglepopup to popuptoggletarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget. Reflect the string value of these three content attributes in IDL as .popUpToggleTarget, .popUpShowTarget, and .popUpHideTarget.
<scotto_> +1
<hdv> +1
<dandclark> +1
<AlexanderFutekov> +1
<Travis> I like that the names group the attribute together by 'popup'.
<masonf> RESOLVED: rename togglepopup to popuptoggletarget, showpopup to popupshowtarget, and hidepopup to popuphidetarget. Reflect the string value of these three content attributes in IDL as .popUpToggleTarget, .popUpShowTarget, and .popUpHideTarget.

css-meeting-bot avatar Jul 14 '22 18:07 css-meeting-bot

Per the resolution, we've solved the issue of attribute reflections (of the string values) for the three triggering attributes. I'll leave this issue open for the (longer term) discussion about non-string-valued IDL reflections, which allow element references to be directly set.

mfreed7 avatar Jul 14 '22 22:07 mfreed7

@mfreed7 apologies if this is on a slight tangent to the original topic. Back in April you mentioned above:

Another counterpoint for anchor is that we're hoping to re-use this attribute as part of the anchor positioning proposal, and the way that's going, it might not require things to be in the top layer. If so, I'd hate to tie anchor back to the .showPopup() call.

Are you suggesting that depending on the outcome of the anchored positioning proposal, that the popup attribute may not require positioning on the top-layer? It sounds like even with anchor positioning as outlined in the proposal, the popup would still be constrained to the bounding container having overfow scroll, hidden, clip. This is the primary use-case that I am hopeful the popup attribute will help with (currently experimenting).

dbatiste avatar Aug 04 '22 19:08 dbatiste

Are you suggesting that depending on the outcome of the anchored positioning proposal, that the popup attribute may not require positioning on the top-layer? It sounds like even with anchor positioning as outlined in the proposal, the popup would still be constrained to the bounding container having overfow scroll, hidden, clip. This is the primary use-case that I am hopeful the popup attribute will help with (currently experimenting).

No, sorry, I meant that the anchor positioning API will allow not just pop-ups to be anchor positioned, but also ordinary elements that are fixed/absolute positioned. The pop-up API definitely still puts elements into the top layer, which escapes overflow containers, transforms, etc.

mfreed7 avatar Aug 08 '22 15:08 mfreed7

Based on the HTML spec PR review, the IDL reflections for these three attributes has been changed to be element references, and renamed accordingly: popoverToggleTargetElement returns Element?.

I'm going to close this issue.

mfreed7 avatar Jan 13 '23 00:01 mfreed7