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

Make auto popover list an algorithm #9241

Merged
merged 6 commits into from
May 9, 2023
Merged

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented May 1, 2023

Fixes #9036

This patch also fixes a call to topmost auto popover which was passing an element instead of a document.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
    • No functional changes
  • Implementation bugs are filed:
    • No functional changes
  • MDN issue is filed: …

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


/popover.html ( diff )

Fixes whatwg#9036

This patch also fixes a call to topmost auto popover which was passing
an element instead of a document.
@annevk annevk added the topic: popover The popover attribute and friends label May 2, 2023
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Do we care that "hide all popovers until" makes somewhat inefficient use of this? I guess it's okay, but I would kinda expect the last two instances to be consolidated perhaps.

cc @jakearchibald

source Outdated Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

Do we care that "hide all popovers until" makes somewhat inefficient use of this? I guess it's okay, but I would kinda expect the last two instances to be consolidated perhaps.

If we cached the list by storing it in a variable, I'm worried that it would become out of date in any of the three calls to auto popover list in hide all popovers until

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

It would be great if this could be reviewed by @jakearchibald since he thought the current definition did not quite work.

source Outdated
<p>To get the <dfn>auto popover list</dfn> for a <code>Document</code> <var>document</var>:</p>

<ol>
<li><p>Let <var>popovers</var> be an empty <span>list</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer using list syntax here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is list syntax?

Copy link
Member

Choose a reason for hiding this comment

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

Let popovers be « ».

https://infra.spec.whatwg.org/#lists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite the language I suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I changed it to exactly "Let popovers be « »."

@annevk annevk merged commit 2d68c83 into whatwg:main May 9, 2023
@annevk
Copy link
Member

annevk commented May 9, 2023

I ended up changing the commit message to lead with the normative change and have the editorial change as a comment in the body.

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.

Questions/issues with popover spec
2 participants