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.

Sign up for GitHub

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

Jump to bottom

Remove redundant call to nearest open popover #9267

Merged
merged 6 commits into from
Jun 22, 2023
Merged

Remove redundant call to nearest open popover #9267

merged 6 commits into from
Jun 22, 2023

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented May 9, 2023 •
edited by pr-preview bot

Fixes #9263

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

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

💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jun 21, 2023, 6:52 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>cloudflare</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@josepharhar josepharhar mentioned this pull request May 9, 2023
Closed
nt1m
nt1m approved these changes May 9, 2023
View reviewed changes
source Outdated Show resolved Hide resolved
@josepharhar
Copy link
Contributor Author

FYI I just removed the assert which I thought was ok to add, candidate can still be null.

@nt1m
Copy link
Member

nt1m commented May 9, 2023

Do we need to add back the early return since the candidate can still be null?

@josepharhar
Copy link
Contributor Author

Passing a null candidate to the subsequent call to nearest open popover is acceptable.

@mfreed7 intentionally removed the early return and said so here: https://chromium-review.googlesource.com/c/chromium/src/+/4514422

NearestOpenPopover returns null in that case, which is what's needed

@annevk
Copy link
Member

annevk commented May 10, 2023

A Chromium commit isn't proof that it works in the standard though. If you look at the algorithm it expects a node and doesn't allow for null to be passed.

@nt1m nt1m added the topic: popover The popover attribute and friends label May 12, 2023
@nt1m
Copy link
Member

nt1m commented Jun 8, 2023

@josepharhar I'm going through the popover issues, I think this is close to being mergeable.

Passing a null candidate to the subsequent call to nearest open popover is acceptable.

I agree this is the case, but spec wise, we'll probably want to do one of the two changes:

  • Make the "nearest inclusive open popover" algorithm also accept null (right now it only takes a "Node")
  • Early return in checkAncestor when the ancestor is null.

@annevk
Copy link
Member

annevk commented Jun 8, 2023

Keeping the early return sounds good to me if they're indeed equivalent.

@josepharhar
Copy link
Contributor Author

Ok, I put the early return back in. Yes, they are equivalent

annevk
annevk approved these changes Jun 22, 2023
View reviewed changes
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.

Ignoring OP as there is no behavior change.

@annevk annevk merged commit 9d8dbcf into whatwg:main Jun 22, 2023
2 checks passed
@annevk
Copy link
Member

annevk commented Jun 22, 2023

Apologies for forgetting to prefix with "Editorial: ". I got distracted.

@YusukeHirao YusukeHirao mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annevk annevk annevk approved these changes

@nt1m nt1m nt1m approved these changes

Assignees
No one assigned
Labels
topic: popover The popover attribute and friends
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redundant call to "nearest inclusive open popover" algorithm?
3 participants
@josepharhar @nt1m @annevk