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

Fixes attribute priority sorting bug #45

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

diiq
Copy link

@diiq diiq commented Feb 13, 2019

Hi! Thanks for optimal-select -- it's been a big help.

I'm working on a project that involves generating many selectors, on pages not under my control. We're using this library. In writing tests for our project, we encountered some strange behaviors:

  • tests that passed on one machine weren't passing on another, because different selectors were being generated
  • unique id's were being ignored in favor of aria attributes.

I traced the problem to a bug in match, when attribute patterns are being generated.

The attribute sorting function in findAttributesPattern can encounter four scenarios:

  1. both curr and next are in the priority list
  2. neither curr nor next are in the priority list
  3. curr is in the list but next is not
  4. next is in the priority list but curr is not.

The old sort function didn't explicitly handle case 4, and so when it encountered elements with attributes not in the priority list, it a) tended to prioritize the unknown attributes over things like ids and b) didn't behave deterministically between browser versions.

This baby PR adds a single if statement to properly handle case 4.

It solves our bugs. It also should generate shorter selectors in some circumstances.

(Just noticed #35, which this fixes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant