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

Editorial: topmost popover ancestor should not use a map #8971

Open
annevk opened this issue Mar 2, 2023 · 4 comments
Open

Editorial: topmost popover ancestor should not use a map #8971

annevk opened this issue Mar 2, 2023 · 4 comments
Labels
clarification Standard could be clearer topic: popover The popover attribute and friends

Comments

@annevk
Copy link
Member

annevk commented Mar 2, 2023

While this is a reasonable implementation strategy to get O(1), it makes the algorithm a lot harder to read. We should get the index of the item in the list instead when it comes time to compare. That will require defining index in Infra, but that seems like a good thing to be doing regardless.

cc @mfreed7 @josepharhar @domenic

@annevk annevk added clarification Standard could be clearer topic: popover The popover attribute and friends labels Mar 2, 2023
@mfreed7
Copy link
Collaborator

mfreed7 commented Mar 2, 2023

While this is a reasonable implementation strategy to get O(1), it makes the algorithm a lot harder to read. We should get the index of the item in the list instead when it comes time to compare. That will require defining index in Infra, but that seems like a good thing to be doing regardless.

Yeah, I see your point. As long as the net effect is the same (and using a map is ok for implementations), I'm ok changing the spec to make it easier to read.

@josepharhar
Copy link
Contributor

@mfreed7 could you outline what the algorithm would look like without a map, or what changes are needed?

That will require defining index in Infra, but that seems like a good thing to be doing regardless.

Do you mean that we should make it so you can do list[index] to get an item in a list like we can do with maps? https://infra.spec.whatwg.org/#map-get

@mfreed7
Copy link
Collaborator

mfreed7 commented Sep 6, 2023

@mfreed7 could you outline what the algorithm would look like without a map, or what changes are needed?

That will require defining index in Infra, but that seems like a good thing to be doing regardless.

Do you mean that we should make it so you can do list[index] to get an item in a list like we can do with maps? https://infra.spec.whatwg.org/#map-get

I think @annevk means something like:

To get the index of an item in a list, given item and list:

  1. define a variable index and set it equal to 0.
  2. Loop through each items in list, in order, and for each entry entry:
    a. Compare entry to item. If they are equal, return index.
    b. Increment index.
  3. Return -1.

And then instead of using a map for popover ancestor, you just find the index using the above algorithm, and index into the list.

@annevk
Copy link
Member Author

annevk commented Sep 8, 2023

Indeed, though I might instead assert that item is in the list, at least until we have sufficient motivation to change that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: popover The popover attribute and friends
Development

No branches or pull requests

3 participants