Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed? #646

Closed
smaug---- opened this issue Dec 9, 2022 · 12 comments
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda popover The Popover API

Comments

@smaug----
Copy link

I wonder if sites will actually use those declarative attributes, especially given that they work with buttons only.
They do feel like possible extra complexity in the API without too much added value.
And the setup becomes complicated and hard to understand if the same element has all those attributes but each pointing to different elements.

(and since the possible advantages to ARIA isn't clear yet, it isn't really an argument for those attributes)

@scottaohara
Copy link
Collaborator

Not sure I follow? The attributes allow for people to invoke the popovers without use of Javascript. That's been a driving feature, and something that is often cited as being a rather letdown for <dialog>, that you need JavaScript to invoke it.

Second, these attributes will have implicit ARIA semantics to indicate the expanded/collapsed state of the popover. So there's accessibility benefits to them that an author will need to manually add to whatever triggering element they instead use with the JavaScript methods to show/hide/toggle.

And the setup becomes complicated and hard to understand if the same element has all those attributes but each pointing to different elements.

that seems like something that should be called out as an author error.

@hidde
Copy link
Contributor

hidde commented Dec 9, 2022

+1 to Scott's point, being able to invoke (or toggle or hide) popovers without JavaScript adds tremendous value.

@smaug----
Copy link
Author

You expect any non-trivial site to actually rely on the attributes?
Based on the XUL popups (which are quite similar in many ways to popover), popup handling tends to require quite a bit JS in any non-trivial cases.

Also, could we perhaps add the attributes once the ARIA side of it all is clear?

@hidde
Copy link
Contributor

hidde commented Dec 10, 2022

Even if ‘non-trivial’ sites or cases would all choose to rely on JavaScript (which I doubt, GitHub is just one example where popovers currently work without JavaScript), ‘trivial’ sites are also valid sites.

Being able to show, hide or toggle without JavaScript also means being able to ship and execute less JavaScript, which is a good practice and essential for users with slow connections and/or devices.

@scottaohara
Copy link
Collaborator

scottaohara commented Dec 11, 2022

You expect any non-trivial site to actually rely on the attributes?

Yes. I would expect those that know how to make accessible popups would be using attributes in the construction of their component. Whether it's only the aria-expanded attribute, or others depending on the component being built, managing attributes and even relationships between the trigger and the popover would need to be handled.

Now, when Firefox implements ARIA's IDL interface, then we could be having a different conversation. But for now, it's not accurate to say that proper construction of these components wouldn't rely on using and managing attributes. At least with the popovertarget... attributes, the browser will handle the state for the developer, as well as the open/close/toggle functionality.

Also, could we perhaps add the attributes once the ARIA side of it all is clear?

You seem to have skipped over this part of my previous comment?

Second, these attributes will have implicit ARIA semantics to indicate the expanded/collapsed state of the popover. So there's accessibility benefits to them that an author will need to manually add to whatever triggering element they instead use with the JavaScript methods to show/hide/toggle.

I'm not sure what's not clear? Is there disagreement that these attributes map to aria-expanded? If so, it would be helpful to raise those concerns so they can be talked about.

@smaug----
Copy link
Author

The current explainer says that further work will need to happen around those attributes and ARIA:
"There will need to be further discussion with the ARIA working group to determine the exact ARIA semantics, if any, are necessary."

@scottaohara
Copy link
Collaborator

The content explainer is out of date. I've made a PR to correct it and have referenced the HTML AAM PR I've created to add these attributes and indicate their implicit mappings
#647

@mfreed7 mfreed7 added the popover The Popover API label Dec 13, 2022
@mfreed7
Copy link
Collaborator

mfreed7 commented Dec 13, 2022

I agree with the sentiment of most commenters here - the declarative invoking attributes are a huge benefit to developers and users. To developers because they avoid the need for (tricky!) Javascript implementations. To users because the a11y connections are made automatically and correctly, and because the functionality is "correct".

To illustrate just one "tricky" thing: when a popovershowtarget button activates a popover, clicking on that same button again will not light dismiss the popover. That requires more than trivial logic to detect, and I suspect it's often missed in "home brew" javascript code for this. There are many such corner cases, which the declarative attributes take care of for the developer.

As to the a11y comments, as mentioned, this is being taken care of in HTML-AAM, and is already implemented in Chromium.

I'm happy to try to get this on an OpenUI agenda if there's appetite to discuss further.

@mfreed7 mfreed7 changed the title [popup] are popovertoggletarget, popovershowtarget and popoverhidetarget needed? [popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed? Dec 14, 2022
@mfreed7 mfreed7 added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Dec 14, 2022
@yinonov
Copy link

yinonov commented Jan 4, 2023

given this issue weighs the purpose of triggering elements, it should be pointed out that, logically, they can't seem to work across tree scopes #651

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed?, and agreed to the following:

  • RESOLVED: we are ok leaving this functionality as part of the API, but we need to keep discussing the exact shape (names, etc.).
The full IRC log of that discussion <gregwhitworth> Topic: [popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed?
<gregwhitworth> github: https://github.com//issues/646
<gregwhitworth> smaug: when I was reviewing the PR I was surprised to see three different attributes doing similar things and they're limited to two element types
<gregwhitworth> smaug: so the whole setup is kind of weird
<gregwhitworth> smaug: also again, based on experience in Firefox; the popups for various menus you hide the custom elements
<gregwhitworth> smaug: it is just a custom element that deals with opening and closing
<masonf> q+
<gregwhitworth> smaug: after the prior discussion about accessiblity I could see how it could benefit by all three
<gregwhitworth> smaug: I think that toggle should be enough rather than all three
<gregwhitworth> ack masonf
<gregwhitworth> masonf: two points embedded in that
<gregwhitworth> masonf: the functionality should it be there?
<gregwhitworth> masonf: if yes, what should it look like?
<gregwhitworth> masonf: most of the comments are around the first question and having the declarative form of this rather than needing to reach to JS and having implemented this I'm sure there are broken scenarios out there
<scotto> q+
<gregwhitworth> masonf: so I'd argue strongly that the functionality should stay
<gregwhitworth> masonf: what should the names be and how it should work and we talked a lot about this and how there are three of them
<gregwhitworth> masonf: it may make sense to summarize them on the issue and then we can circle back if necessary
<scotto> q-
<gregwhitworth> smaug: when I filed that issue, the accessibility stuff wasn't in the explainer so that kind of changes things
<gregwhitworth> scotto: yeah, I think moving to the issue makes sense
<Brecht_De_Ruyte> q+
<gregwhitworth> masonf: do you agree that the functionality needs to exist and you're only issue is how it's defined
<gregwhitworth> smaug: yeah I don't have an issue with that but I foresee people reaching for JS
<masonf> - Proposed resolution: we are ok leaving this functionality as part of the API, but we need to keep discussing the exact shape (names, etc.).
<gregwhitworth> Brecht_De_Ruyte: I like the naming because it's very clear; the toggle question if it's enough - I know a lot of people would like three types of events. They don't know when toggle occurs, the name informs you of when it will occur in your scenario
<masonf> RESOLVED: we are ok leaving this functionality as part of the API, but we need to keep discussing the exact shape (names, etc.).

@mfreed7
Copy link
Collaborator

mfreed7 commented Jan 13, 2023

Thanks for the discussion on this one. As the resolution says, we agreed that the API should keep its declarative invocation feature, but there was a desire to continue the discussion about the exact naming/number of the attributes. To that end, here's a summary of the places where we discussed this feature, so that you can see how we got here:

  1. [popup] Consider renaming triggerpopup #508 discusses three attributes vs. just one. Ends with togglepopup, showpopup, and hidepopup. This issue brainstorms a number of alternatives, and arrives mostly at the current one, modulo the names.
  2. [popup] What are the precedence rules for togglepopup, showpopup, and hidepopup #523 discusses the new questions three attributes raise with precedence among them. We resolve on the exact behavior of the attributes, particularly when there are multiple.
  3. [Popup] New IDL for Pop-Up content attributes, which allow Element references #382 discusses the IDL reflections for these attributes, but also resolves to rename them to popover*target for clarity.
  4. [Popup] Triggering element support: buttons, text inputs, etc. #420 discusses which elements support triggering attributes. Lots of good discussion here. We end with support for "buttons" that don't submit forms. We leave the door open for other types, once a11y issues can be resolved.
  5. [selectmenu] button click behavior when the popup is open #486 discusses some nuances of the behavior of invokers w.r.t. light dismiss. For example that clicking on the invoking element for an open popover doesn't constitute a light dismiss signal. (Tough to get right in JS.)

As you can see from the list, there has been quite a bit of good discussion (both on the issues and in at least a dozen live meetings) with tons of input from many folks. Please take a look and see what you think, after reviewing the discussions. Perhaps we went off the tracks somewhere in there.

Developer feedback has been generally positive for this set of attributes. While they're long, they are at least very descriptive and clear on what they do.

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed? #646, and agreed to the following:

  • RESOLVED: leave this part of the API as-is.
The full IRC log of that discussion <gregwhitworth> Topic: [popover] are popovertoggletarget, popovershowtarget and popoverhidetarget needed? #646
<gregwhitworth> github: https://github.com//issues/646
<gregwhitworth> masonf: overall question, do we need these 3 properties - yes we do
<gregwhitworth> masonf: the followon question was the API shape and the path we took to get here
<gregwhitworth> masonf: there were 5 different issues and many meetings
<gregwhitworth> masonf: there were a bunch of crickets on the thread
<jhey> q+
<gregwhitworth> masonf: my default is going to be leave it as it is
<gregwhitworth> ack jhey
<gregwhitworth> jhey: my only thing that I is to know that we'll have initiallyopen?
<gregwhitworth> Zakim: start meeting
<gregwhitworth> jhey: that's a pretty critical piece?
<gregwhitworth> masonf: defaultopen and initiallyopen is a different issue
<gregwhitworth> masonf: this is only about popover*
<gregwhitworth> jhey: and hover is out of the question?
<gregwhitworth> masonf: yes it is
<gregwhitworth> masonf: that's on the backlog for me to get all interelated items aligned
<gregwhitworth> q?
<masonf> Proposed resolution: leave this part of the API as-is.
<masonf> RESOLVED: leave this part of the API as-is.

@mfreed7 mfreed7 closed this as completed Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda popover The Popover API
Projects
None yet
Development

No branches or pull requests

6 participants