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

Add explainer for popover=hint and hover triggering of popovers #766

Merged
merged 9 commits into from
Jul 10, 2023

Conversation

mfreed7
Copy link
Collaborator

@mfreed7 mfreed7 commented Jun 8, 2023

This is the first draft of a renewed explainer for two features:
1. hover triggering of popovers
2. popover=hint behaviors

This functionality was initially included in the original popover explainer, but was removed because of lingering uncertainty about a number of a11y and other issues. Now that the popover API has landed in WHATWG/html, this PR brings back a fresh explainer for those features.

This is intended to be a public working draft of this document, not a final explainer.

mfreed7 added 2 commits June 8, 2023 15:26
This is the first draft of a renewed explainer for two features:
 1. hover triggering of popovers
 2. `popover=hint` behaviors
@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 8, 2023

@scottaohara @aleventhal PTAL and let me know what you think. I kept your names on this, but can remove them if desired. Hopefully most of what's here isn't a surprise, since it's from documents you've seen. 😄

I wanted to get this back in the public so we can start to really discuss it again.

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 8, 2023

@andrico1234 (or others) I could use some help with the build step. It gives this error:

3:48:50 PM: [@mdx-js/rollup] Could not parse expression with acorn: Unexpected token
3:48:50 PM: file: /opt/build/repo/site/src/pages/components/popover-hint.research.explainer.mdx
3:48:51 PM: The language "webidl" doesn't exist, falling back to plaintext.
 error   Could not parse expression with acorn: Unexpected token

but doesn't give a line number, and webidl doesn't exist in the file. I'm guessing this has to do with one of my backtick-escaped code sections, but I don't want to guess and check. Hints appreciated. It'd be amazing if this error message came with a line number.

@scottaohara
Copy link
Collaborator

@mfreed7 have not taken a look, but will as soon as I can. I obviously have another task related to this that i have not delivered on. also, just trying to find time to get that done. apologies.

Comment on lines +71 to +72
- `popover-show-delay` - applied to the invoking element. The invoking element must be hovered, focused, or long-pressed for this long before the popover is shown. The default value for this property is `0.5s`.
- `popover-hide-delay` - applied to the popover. The invoking element *or* the popover must be **not** hovered, focused, or long-pressed for this long before the popover is hidden. The default value for this property is `infinity`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two properties feel useful for more than this. What intrinsic behaviours are needed for these?

Another curiosity; popover-show-delay is somewhat replicable with animation-delay or transition-delay but popover-hide-delay is not. I wonder if there's a bigger problem that can be solved here, for example adding animation-exit states which trigger when a node moves out of a CSS state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting feedback! I had envisioned these to work with popovers (and maybe dialogs?) only. How do you envision making them more general?

As to popover-show-delay I'm not sure this can be done with animation-delay - can you prove me wrong? My concern is that the behavior wouldn't be de-bounced in that case. I.e. set popover-show-delay to 0.5 seconds, and have the user hover and then de-hover the element only for 0.2 seconds. Nothing should happen, but I think animation-delay would cause the popover to just be shown after 0.5 seconds, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://codepen.io/keithamus/pen/yLQabJK this uses animation-delay to ensure that as you hover and de-hover (or focus/defocus) the button for less than 0.5s, the div will not be shown. I believe this is properly debounced as you describe. I don't know how to get the same effect upon exit of that state; transitions don't work quite the same, I believe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh nice - I see that you get that by adding :focus to clear the animation if you de-hover. But you're right - not possible on the de-hover side of things.

Do you think this is worth opening a CSSWG issue to discuss? I mean about the "more general problem" question?


- Plain hints: the browser will simply expose the text on the triggering element. The actual popover target element and its descendants can be invisible/ignored in the AX tree.
1. If no other accessible name is available, use hint’s inner text for the accessible name
2. If it's used as the name, then use it to compute the description, setting the described by relation to point from the trigger to the hint element
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's used as the name

I am confused by this bit. Do we mean, if the triggering element already has an accessible name?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be something like "if the triggering element already has an accessible name, then the hint becomes the accessible description..."

this is essentially supposed to mimic the current behavior of the title attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I've updated the text with your suggestion.


```html
<button popovertarget=menu1>Menu</button>
<menu popover id=menu1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this example use <menu>? It might not be the best tag to use in such an example, as <menu> does not have the implicit aria-role of menu.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but none of the list items or buttons have role=menuitem either, so it matches the HTML example of how to create a "menu" list of actions (buttons).

but to your point, yes, people aren't super familiar with the menu element and often misunderstand that menu !== menu. :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm one of those people. Even though I've been told this, I consistently forget it.

Is the "correct" thing to use <div role=menu> then? For now, I've updated the explainer to do that, but let me know if there's something better.

| Force-hides: | Other `auto`s and `hint`s | Other `hint`s | Nothing |
| Nesting: | Yes | No | No |

First, hints should always be light dismissable. They are transient, supplementary information, so they should not require affirmative action to close them. Clicking outside or hitting ESC will close a hint.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does/should moving focus from the invoker (or children of the open hint popover) also close the hint? What happens for example with this markup, as I move focus:

<button popovertarget="hint1" popovertargetaction="hint">Button 1</button>
<div id="hint1">Hint 1</div>

<button>Button 2</button>

<button popovertarget="hint3" popovertargetaction="hint">Button 3</button>
<div id="hint1">Hint 3</div>

If I focus on Button 2, hint1 will open. If my focus switches to Button 2, does this mean hint1 stays open? Should it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving to button 2 should close the hint from button 1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Perhaps I’m misreading but the explainer doesn’t feel clear that this is the case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this behavior (bluring button 1) is handled by the popover-hide-delay CSS property. If hint 1 was shown because button 1 was focused (for popover-show-delay seconds), then it will be hidden once button 1 is blurred for popover-hide-delay seconds.

I've added an explicit example and a lot more text around this. LMK if that helps clarify.

</button>
</li>
</menu>
<menu class=submenu popover id=submenu1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggle a little with this example. It shows good intent of the practicality/extensibility of the pattern but it falls short of providing a tangible demonstration that could/should actually make it to a website. Obviously there's a need for brevity in examples, but perhaps demonstrating such markup without heavily caveating the various additional affordances it would need (arrow key navigation) makes it a bit of a distraction from the overall scope of the proposal?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably should just move away from menu elements all together, as the example doesn't demonstrate role=menu, but people may assume it is one

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved to <div role=menu>, but yeah I'd be concerned that adding all of the details would muddy the waters. I do however think that the hover-triggered menu is a pretty common use case, so it's good to include. I can perhaps include some text alluding to this not being a "complete" solution?

| Force-hides: | Other `auto`s and `hint`s | Other `hint`s | Nothing |
| Nesting: | Yes | No | No |

First, hints should always be light dismissable. They are transient, supplementary information, so they should not require affirmative action to close them. Clicking outside or hitting ESC will close a hint.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@keithamus this help?

Suggested change
First, hints should always be light dismissable. They are transient, supplementary information, so they should not require affirmative action to close them. Clicking outside or hitting ESC will close a hint.
First, hints should always be light dismissable. They are transient, supplementary information, so they should not require affirmative action to close them. Clicking or tapping outside, hitting the <kbd>ESC</kbd> key, or moving focus away from the invoking element will close a hint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is the last thing about moving focus away. We explicitly didn't do that for popover=auto because it was jarring and hard to get right. I'd say we definitely shouldn't do that for "rich hints" for all of the same reasons.

My intention was to make the "move focus away to dismiss" is handled entirely by popover-hide-delay. See the new text I've added, and LMK if this allays any of your concerns here.


For the best screen reading experience, the implementation will need to expose different properties for a plain hint vs a rich hint (classification described above).

- Plain hints: the browser will simply expose the text on the triggering element. The actual popover target element and its descendants can be invisible/ignored in the AX tree.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Plain hints: the browser will simply expose the text on the triggering element. The actual popover target element and its descendants can be invisible/ignored in the AX tree.
- Plain hints: the browser will expose the flattened text string comprised of the hint's text and text alternative content via the invoking element. The actual popover target element and its descendants can be invisible/ignored in the AX tree. Note: this would be similar to how `title` attribute tooltips are treated today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I incorporated this.

For the best screen reading experience, the implementation will need to expose different properties for a plain hint vs a rich hint (classification described above).

- Plain hints: the browser will simply expose the text on the triggering element. The actual popover target element and its descendants can be invisible/ignored in the AX tree.
1. If no other accessible name is available, use hint’s inner text for the accessible name
Copy link
Collaborator

@scottaohara scottaohara Jun 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If no other accessible name is available, use hint’s inner text for the accessible name
1. If the invoking element lacks an accessible name, use the computed text of the hint as the element's accessible name

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


- Plain hints: the browser will simply expose the text on the triggering element. The actual popover target element and its descendants can be invisible/ignored in the AX tree.
1. If no other accessible name is available, use hint’s inner text for the accessible name
2. If it's used as the name, then use it to compute the description, setting the described by relation to point from the trigger to the hint element
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
2. If it's used as the name, then use it to compute the description, setting the described by relation to point from the trigger to the hint element
2. If the hint is not used as the accessible name, then the hint would be treated as the accessible description of the invoking element. A described by relation would point from the trigger to the hint element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 12, 2023

@scottaohara and @keithamus thanks very much for the thoughtful review here! I opened an issue for the attribute bikeshedding, and added replies to the rest of your comments. I'd love to ask though: anything remaining that you think is still open (and there are probably at least a few!) - could we please land this PR and then open issues to discuss those questions directly? It's hard to design these features in comments on a PR, whereas an issue gives us one central place to keep track of things.

Copy link
Collaborator

@dbaron dbaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this review is settling down, although there are a few open comments about the technical stuff. One comment about trying to actually get this landed, though:

@mfreed7
Copy link
Collaborator Author

mfreed7 commented Jun 21, 2023

It seems like this review is settling down, although there are a few open comments about the technical stuff. One comment about trying to actually get this landed, though:

Yep, I think I've (just) addressed the remaining technical issues. There are open discussions, which I've left, but I'd be happy to land this now and open issues to keep discussing the interesting parts.

@dbaron dbaron self-requested a review June 21, 2023 14:53
@keithamus
Copy link
Collaborator

I'm happy to file issues around the discussion points I had, and we can land this.

Copy link
Collaborator

@dbaron dbaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I think this has already gotten sufficient technical review, I've reviewed mainly for markup and site integration. I think this looks good modulo a few comments below, although one may require a little thought and another might require a few rounds of pushing attempts to fix it and checking the deploy preview.

@@ -0,0 +1,195 @@
---
menu: Proposals
name: Popover=hint and Hover Triggering (Explainer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this name probably needs to be shorter, because it substantially widens the sidebar on https://open-ui.org/ and thus changes the layout a bit. (The current longest item is "Selectmenu Element (Explainer)".

I'm not really sure what to suggest, though. I don't have any bright ideas other than "Popover Level 2 (Explainer)". I don't have a strong opinion about what the item in the document list should be, but the length of the current one makes it look bad.

(Alternatively, we could try to fix the CSS so that this will line-break, maybe even with hanging indent.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I went with Popover Hint and Hover for now. Not awesome, but at least it's short enough. Likely we should try to remove "(Explainer)" from all of them anyway, since it's quite repetitive and not informative.

@andrico1234 for further comments. But I think ok for this PR.


For the best screen reading experience, the implementation will need to expose different properties for a plain hint vs a rich hint (classification described above).

- Plain hints: the browser will expose the flattened text string comprised of the hint's text and text alternative content via the invoking element. The actual popover target element and its descendants can be invisible/ignored in the AX tree. Note: this would be similar to how `title` attribute tooltips are treated today.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list isn't formatted right in the preview. Maybe it would help to line up the "1.", "2.", etc. with the words "Plain" and "Rich"? I'm not sure if that will work -- it might require a few attempts to see what happens in the preview.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown is cool, until it's not. I'll try. I think you're right about lining things up with the "Plain"/"Rich".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So @andrico1234 I'm leaving this as-is, since I tried several permutations and none seem to work. I think this might be the Astro MD interpreter or something? It works on my local MD viewer in several ways, but none of those work on the preview.

Copy link
Collaborator Author

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

@@ -0,0 +1,195 @@
---
menu: Proposals
name: Popover=hint and Hover Triggering (Explainer)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I went with Popover Hint and Hover for now. Not awesome, but at least it's short enough. Likely we should try to remove "(Explainer)" from all of them anyway, since it's quite repetitive and not informative.

@andrico1234 for further comments. But I think ok for this PR.

Comment on lines +71 to +72
- `popover-show-delay` - applied to the invoking element. The invoking element must be hovered, focused, or long-pressed for this long before the popover is shown. The default value for this property is `0.5s`.
- `popover-hide-delay` - applied to the popover. The invoking element *or* the popover must be **not** hovered, focused, or long-pressed for this long before the popover is hidden. The default value for this property is `infinity`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh nice - I see that you get that by adding :focus to clear the animation if you de-hover. But you're right - not possible on the de-hover side of things.

Do you think this is worth opening a CSSWG issue to discuss? I mean about the "more general problem" question?


For the best screen reading experience, the implementation will need to expose different properties for a plain hint vs a rich hint (classification described above).

- Plain hints: the browser will expose the flattened text string comprised of the hint's text and text alternative content via the invoking element. The actual popover target element and its descendants can be invisible/ignored in the AX tree. Note: this would be similar to how `title` attribute tooltips are treated today.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown is cool, until it's not. I'll try. I think you're right about lining things up with the "Plain"/"Rich".

@mfreed7 mfreed7 merged commit ec62464 into openui:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants