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

Inconsistent/Unexpected behavior when clicking area that has includeKeys #376

Open
techfg opened this issue Feb 15, 2021 · 0 comments
Open

Comments

@techfg
Copy link
Collaborator

techfg commented Feb 15, 2021

Describe the bug
In v1.2.0 (54877ef), per the changelog at the time, functionality was introduced to allow selecting includeKeys areas from staticState areas. This change resulted in several underlying issues that need to be addressed:

  1. includeKeys with circular references causes max callstack exception (addressed in Uncaught RangeError: Maximum call stack size exceeded when includeKeys has circular reference #374)
  2. "Clicking" on an area has different behavior than calling set/select/deselect APIs (the behavior should be identical except where otherwise documented). Clicking the area will result in includedKeys having their selected state toggled, while calling the api will not.
  3. Per the docs, staticState areas are not "selectable" by the user. However, while this change does not permit select for the area being clicked, it does change selected state for any includedKey that does not have staticState. There is something to be said for whether or not this is consistent with the intent of staticState
  4. When includedKeys are iterated and their selected state modified, it is done so via toggle approach. This results in areas for the includedKeys potentially having different selected states which may be intentional/unintentional outcome. For example, if area 'A' has includedKeys 'B' & 'C' where 'A' is not selected, 'B' is selected and 'C' is not selected, clicking 'A' will toggle its selected state (assuming it isn't configured staticState) to true, 'B' will become false and 'C' will become true. The question here is should all three match 'A' since it was the one that was clicked or should it toggle all as it currently does.
  5. When includedKeys are iterated, onClick is fired for each area which may or may not be an intended behavior. Additionally, when onClick returns true, navigation could occur. With the introduction of navigationOpen config property to support using open instead of window.location.href, if the target isn't self, this could end up with N number of navigations occurring.

Before determining how best to address this, I am seeking input from community to determine how this "feature" is being used (e.g. use cases). I believe the underlying reason why this change was made was because staticState doesn't respond to click events wrt to toggle so it provided a way to have its includedKeys still select/deselect. However, with the way the rendering works, the includedKeys would appear 'selected' regardless, the only thing they wouldn't have is their selected state being true / false.

To determine how to address these and define behavior clearly:

  1. Need to understand use cases for why this feature was added
  2. Need to determine whether or not this feature should remain based on those use cases and if it remains, need to clearly define expected behavior and ensure click and api result is identical (unless otherwise expected/defined not to be as with staticState, etc.).

I think a good starting point is to define how select/deselect/set APIs wrt to includedKeys they should be a superset or equivalent functionality for user initiated events. In short, should these APIs modify the selected state of includedKeys (they currently do not) and if not, then its likely user events should not either. If they should, then for user events, the same should likely occur, however there are two scenarios: 1) staticState not applied; and 2) staticState applied and how should each scenario be treated.

Community input & feedback on this encouraged!

To Reproduce
Steps to reproduce the behavior:

  1. Go to jsFiddle
  2. API different than click
    1. Mouse click on Alaska (select) - Keys selected are Noncontiguous & AK
    2. Mouse click on Alaska (deselect) - Keys selected are No Keys Selected
    3. Click Select for Alaska in table at top - Keys selected are AK
    4. Click Deselect for Alaska in table at top - Keys selected are No Keys Selected
  3. staticState === true
    1. Mouse click on 'Washington(select) - Keys selected areCA`
    2. Mouse click on Washington (deselect) - Keys selected are No Keys Selected
    3. Click Select for Washington in table at top - Keys selected are WA
    4. Click Deselect for Washington in table at top - Keys selected are No Keys Selected
  4. staticState === false
    1. Mouse click on 'Oregon(select) - Keys selected areCA`
    2. Mouse click on Oregon (deselect) - Keys selected are No Keys Selected
    3. Click Select for Oregon in table at top - Keys selected are OR
    4. Click Deselect for Oregon in table at top - Keys selected are No Keys Selected

More coming soon.

Expected behavior

  1. API different than click - API & click behavior should result in same keys being selected/deselected
    More coming soon.
  2. staticState === true - API & click behavior should result in same keys being selected/deselected
    More coming soon.
  3. staticState === false - API & click behavior should result in same keys being selected/deselected

More coming soon.

Screenshots
N/A

Desktop (please complete the following information):
N/A

Smartphone (please complete the following information):
N/A

Additional context
N/A

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

No branches or pull requests

1 participant