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 popover=hint #9778

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

Add popover=hint #9778

wants to merge 38 commits into from

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Sep 23, 2023

Fixes #9776

Explainer: https://open-ui.org/components/popover-hint.research.explainer/#popoverhint-behavior

(See WHATWG Working Mode: Changes for more details.)


/interactive-elements.html ( diff )
/popover.html ( diff )

@mfreed7 mfreed7 mentioned this pull request Sep 29, 2023
3 tasks
@nt1m
Copy link
Member

nt1m commented Oct 8, 2023

I would probably want https://github.com/keithamus/invoker-buttons-proposal to be worked on first before addressing this, as I think it's a building block for this and other declarative attributes.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 30, 2023

I would probably want https://github.com/keithamus/invoker-buttons-proposal to be worked on first before addressing this, as I think it's a building block for this and other declarative attributes.

So I've significantly updated the popover=hint explainer, and there are a few sections detailing why I believe the functionality described in this PR is independent of the hover-triggering functionality described by the Invokers explainer (note that it was moved to the OpenUI repo). See this section:

Indeed both are important bits of functionality, but I'm hoping we can tackle them independently rather than needing to put up another huge spec PR. That didn't work very well for the original Popover spec PR, at least in my opinion.

@mfreed7 mfreed7 mentioned this pull request Oct 30, 2023
@josepharhar josepharhar added the topic: popover The popover attribute and friends label Dec 21, 2023
@josepharhar
Copy link
Contributor Author

Note to self: this will require additional changes in https://chromium-review.googlesource.com/c/chromium/src/+/5229300 if the corresponding PR to be opened gets merged

Copy link
Contributor

@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.

Ok, I reviewed mostly for the correctness of the algorithms, and things look pretty good. I made a few corrections, but other than those, LGTM.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@mfreed7
Copy link
Contributor

mfreed7 commented Mar 7, 2024

Gentle ping - any chance we could get an initial review of this PR? @annevk @domenic

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

A few more editorial nits

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with final nit

source Show resolved Hide resolved
@mfreed7 mfreed7 added the agenda+ To be discussed at a triage meeting label Apr 25, 2024
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Conceptually looks good, though I think the current getStackPosition behavior is not correct / what we want? I might be wrong tho.


<li><p>If <var>popover</var> is in <var>popoverList</var>, then return the index of
<var>popover</var> in <var>popoverList</var> + 1.</p></li>
<p>To <dfn>get the popover stack position</dfn>, given an <span data-x="html elements">HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are popover hints guaranteed to be "leaf" / always on top of the non-hint ones?

In particular, if you stash a non-hint popover inside a hint one, then using two separate lists seems wrong? This might be an issue with auto popovers already, in a way.

It might be worth having a single popover list, and just make algorithms skip popovers given a "type"?

So to hide all hints you'll copy the full popover list, and just skip those that are not of type "hint"... Which is already kinda what https://html.spec.whatwg.org/#auto-popover-list is doing already.

But the behavior of getStackPosition doesn't make sense otherwise, I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are popover hints guaranteed to be "leaf" / always on top of the non-hint ones?

The intention of the algorithms as-is (e.g. see <var>okNesting</var>) is that yes, popover hints are guaranteed to be either a) on top of any non-hint popovers (if nested) in the "auto popover stack", or b) stacked by themselves (with no non-hint ancestors) in the "hint popover stack".

In particular, if you stash a non-hint popover inside a hint one, then using two separate lists seems wrong?

In this case, the algorithm (again, see <var>okNesting</var>) will treat that non-hint popover as not actually being nested inside the hint popover, and the hint popover will be closed. In the same way that one independent popover will close another.

This might be an issue with auto popovers already, in a way.

Help me understand this comment. What's the issue?

It might be worth having a single popover list, and just make algorithms skip popovers given a "type"?

I'm not sure if you are proposing a change in functionality, or just doing away with the two algorithms for "auto popover stack" and "hint popover stack"? Those were just conveniences to make the rest of the algorithms less verbose - they essentially just retrieve the elements with the right opened in popover mode. There's really just one list - the top layer elements list.

If you're talking about a change in functionality, where we do away with opened in popover mode, then I'm not sure this is possible. For example, if you open a new hint that is nested inside another hint on the "auto popover stack", then that should close any hints from the "hint popover stack" but not the one already in the "auto popover stack". So you need to know which list they're in. Help me understand.

So to hide all hints you'll copy the full popover list, and just skip those that are not of type "hint"... Which is already kinda what https://html.spec.whatwg.org/#auto-popover-list is doing already.

See above.

But the behavior of getStackPosition doesn't make sense otherwise, I believe.

Not sure I understand what part doesn't make sense, but perhaps it's about the above questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gentle ping - any concerns with my answers above? Can we resolve this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gentle ping @emilio, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I had to page this back in, I think it's ok. Really sorry for the lag here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Thank you, and no problem on the delay.

@mfreed7
Copy link
Contributor

mfreed7 commented May 30, 2024

Conceptually looks good,

Cool, thanks for the review! Sounds like you're supportive of the PR in general? If so, mind marking that on mozilla/standards-positions#965?

though I think the current getStackPosition behavior is not correct / what we want? I might be wrong tho.

See my comment in the review - let's discuss there. I want to understand a bit better.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
@mfreed7
Copy link
Contributor

mfreed7 commented Jul 25, 2024

This issue/PR was just discussed at the OpenUI/WHATWG/CSSWG form controls task force meeting, in the context of a more general discussion of the "tooltip" use case. The notes are located here:

https://logs.csswg.org/irc.w3.org/css/2024-07-25/#e1637392

@mfreed7
Copy link
Contributor

mfreed7 commented Aug 15, 2024

Conceptually looks good,

Cool, thanks for the review! Sounds like you're supportive of the PR in general? If so, mind marking that on mozilla/standards-positions#965?

@emilio any thoughts on this one? I.e. adding support (or not) from Mozilla's point of view, for popover=hint?

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.

popover=hint
7 participants