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

hidePopover with button click not timeout #10469

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Jxck
Copy link

@Jxck Jxck commented Jul 10, 2024

as mentioned by @keithamus at #10428 (comment)
closing popover with timeout (time based UI) is not in conformance with WCAG 2.2.3.

But the current sample code in the spec expresses it. so it should be replaced with "explicitly closed manner".

I respect the current code as-is and adding minimal fixes. If I can refuctoring whole code (indentation, id name, code flow etc), I'd love to do that too.

Thanks
Jxck


/popover.html ( diff )

source Outdated
@@ -84796,9 +84798,9 @@ dictionary <dfn dictionary>DragEventInit</dfn> : <span>MouseEventInit</span> {
}
outSpan.showPopover();

setTimeout(function () {
cBtn.addEventListener("click", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is the best demonstration of manual behaviours, given that this is what popover buttons can do without JS?

Perhaps a more compelling demo would be to close on keypress instead?

Copy link
Author

Choose a reason for hiding this comment

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

this is what popover buttons can do without JS

I think so, and same for showPopover() too. (that's current demo's issue, not mine)

Perhaps a more compelling demo would be to close on keypress instead?

in that case which keypress are fine ? ESC ? that's also included in native light-dismiss.

can we refine hole demo based on more meaningful concept ?

e.g.

  • clicking "load" button runs fetch
  • fetch rejection shows popover
  • clicking "load" again hides popover

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a great idea to me. Perhaps @josepharhar has some opinions here too.

Copy link
Author

@Jxck Jxck Jul 11, 2024

Choose a reason for hiding this comment

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

I updated whole sample based on discussion, PTLA.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we could replace the call to showPopover with a popovertarget attribute on the submit button, assuming that the button is not supposed to be a form element submitting button.

Copy link
Author

@Jxck Jxck Jul 17, 2024

Choose a reason for hiding this comment

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

@keithamus How about hover use-case ?

  • pointerover: showPopover after 500ms delay
  • pointerleave: hidePopover after 200ms delay

it's inspired by MUI tooltip.
it uses setTimeout but not "Time Limit UI" which WCAG mentions I believe.
seems valid use-cases to use JS show/hide for me.

$div.addEventListener("mouseover", () => {
  setTimeout(() => {
    $tooltip.showPopover()
  }, 500)
})

$div.addEventListener("mouseout", () => {
  setTimeout(() => {
    $tooltip.hidePopover()
  }, 200)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

My one concern with making a tooltip is that we're working on popover=hint and interesttarget which will do much of the same behaviour, and so I worry we'd be adding an example that may one day become redundant.

Copy link
Author

Choose a reason for hiding this comment

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

we're working on popover=hint and interesttarget which will do much of the same behaviour

hint and interesttarget do not seems directory related with "delay for dismiss" in my view.
Could I clarify your concern a bit more?

Copy link
Contributor

Choose a reason for hiding this comment

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

They will have configurable delays. My concern is more about the general pattern of representing a "tooltip like" component with JS that one day may not need it.

There are other floating widgets we could represent, such as menus (or make an accessible toast) that are further away from the tooltip concept, and therefore more likely to stand up as an example over the coming years.

Having said that; perhaps a tooltip example is fine for now and it can be something we revisit when the new features land.

Copy link
Author

Choose a reason for hiding this comment

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

They will have configurable delays.

Ah I missed that, got it.

Having said that; perhaps a tooltip example is fine for now and it can be something we revisit when the new features land.

I agree that, and it can be align with Living Standard manner.

@Jxck
Copy link
Author

Jxck commented Jul 17, 2024

for ease of review

<img id="image" src="cat.js" alt="pretty kitten">
<div id="tooltip" popover>
  <output></output>
  <button popovertarget="message" popovertargetaction="hide">x</button>
</div>

<script>
  const $img = document.querySelector("#image")
  const $tooltip = document.querySelector("#tooltip")
  const $output = $tooltip.querySelector("output")

  $img.addEventListener("pinterover", () => {
    $output.textContent = $img.alt
    $tooltip.showPopover()
  })
  $img.addEventListener("pinterleave", () => {
    $tooltip.showPopover()
  })
</script>

source Show resolved Hide resolved
source Show resolved Hide resolved
@Jxck Jxck requested a review from keithamus July 18, 2024 04:42
@Jxck
Copy link
Author

Jxck commented Aug 5, 2024

@keithamus any other comments ?

outSpan.showPopover();
<pre><code class="html">&lt;img id="image" src="cat.js" alt="pretty kitten">
&lt;div id="tooltip" popover>
&lt;output>&lt;/output>
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous code had the popover inside of the <output> element instead of having the popover outside of the <output>, but I'm not sure if that means anything with regards to the semantics of the <output> element.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, but it's out of scope for here.
I'll remove it.

@josepharhar
Copy link
Contributor

I'm not a fan of the php style $ at the beginning of variable names and lack of semicolons at the end of lines, but I'm not sure if there is any such style guide for the HTML spec example code.

@Jxck
Copy link
Author

Jxck commented Aug 11, 2024

To avoid extra effort, we would like to avoid modifications based on someone's preferences.
Can I have a style guide for here ?

@josepharhar
Copy link
Contributor

To avoid extra effort, we would like to avoid modifications based on someone's preferences.
Can I have a style guide for here ?

I don't think there is a style guide and I'm not an editor of this spec, so I'll defer to the editors for this.

@annevk annevk added the topic: popover The popover attribute and friends label Aug 14, 2024
@Jxck
Copy link
Author

Jxck commented Oct 13, 2024

@josepharhar it's been done for now. PTL again plz.

@Jxck Jxck requested a review from josepharhar October 13, 2024 20:23
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: popover The popover attribute and friends
Development

Successfully merging this pull request may close these issues.

4 participants