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

Redundant call to "nearest inclusive open popover" algorithm? #9263

Closed
nt1m opened this issue May 8, 2023 · 3 comments · Fixed by #9267
Closed

Redundant call to "nearest inclusive open popover" algorithm? #9263

nt1m opened this issue May 8, 2023 · 3 comments · Fixed by #9267
Assignees
@josepharhar
Labels
topic: popover The popover attribute and friends

Comments

@nt1m
Copy link
Member

nt1m commented May 8, 2023

In the "topmost popover ancestor" algorithm: https://html.spec.whatwg.org/multipage/popover.html#topmost-popover-ancestor

We call:

  1. Run checkAncestor given the result of running nearest inclusive open popover given newPopover's parent node within the flat tree.

but checkAncestor also calls "nearest inclusive open popover" at step 8.2?

Let candidateAncestor be the result of running nearest inclusive open popover given candidate.

I think this is redundant?

@josepharhar @mfreed7 @annevk @rwlbuis What do you think?
@nt1m nt1m added the topic: popover The popover attribute and friends label May 8, 2023
@rwlbuis
Copy link

rwlbuis commented May 8, 2023

@josepharhar @mfreed7 @annevk @rwlbuis What do you think?

I was also surprised seeing this in some implementations, looking forward to the explanation.

@mfreed7
Copy link
Contributor

mfreed7 commented May 8, 2023

@josepharhar @mfreed7 @annevk @rwlbuis What do you think?

I was also surprised seeing this in some implementations, looking forward to the explanation.

Explanation: complete oversight on my part. I agree, we should remove the call at step 9 and just rely on the one already in checkAncestor.

josepharhar added a commit to josepharhar/html that referenced this issue May 9, 2023
@josepharhar
Remove redundant call to nearest open popover
e391bde 
Fixes whatwg#9263
@josepharhar josepharhar mentioned this issue May 9, 2023
Merged
4 tasks
@josepharhar
Copy link
Contributor

I opened a PR to remove the redundant call here: #9267

aarongable pushed a commit to chromium/chromium that referenced this issue May 9, 2023
Remove extra call to NearestOpenPopover() and clean up
dc41be6 
See [1] but it was pointed out that there's an extra call to
NearestOpenPopover() that can be removed. Since the behavior of
NearestOpenPopover is inclusive by default, it should be idempotent.
This CL removes the extra call and cleans things up just a bit.

[1] whatwg/html#9263

Bug: 1307772
Change-Id: I9523d4292c64ea9a590831baf509163d208d3a74
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4514422
Auto-Submit: Mason Freed <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Commit-Queue: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1141485}
annevk pushed a commit that referenced this issue Jun 22, 2023
@josepharhar
Remove redundant call to nearest open popover
9d8dbcf 
Fixes #9263.
rubberyuzu pushed a commit to rubberyuzu/html that referenced this issue Jul 20, 2023
@josepharhar
Remove redundant call to nearest open popover
a01a9a1 
Fixes whatwg#9263.
rubberyuzu pushed a commit to rubberyuzu/html that referenced this issue Jul 21, 2023
@josepharhar
Remove redundant call to nearest open popover
94a07f2 
Fixes whatwg#9263.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@josepharhar josepharhar

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

Successfully merging a pull request may close this issue.

Remove redundant call to nearest open popover josepharhar/html
4 participants
@nt1m @josepharhar @rwlbuis @mfreed7