Skip to content

fix(GlFilteredSearch): Generate stable token keys

Mark Florian requested to merge fix-filtered-search-token-keys into main

What does this MR do?

This fixes incorrect behaviour when destroying a token which has another token of the same type after it. The incorrect behaviour is to activate the next token; the correct behaviour is not to activate it.

There were two underlying causes to this incorrect behaviour:

  1. The key used for tokens was insufficiently unique. It was just ${token.type}-${tokenIndex}. This meant that if you had the following:

    UI:   Label=Bug Label=Feature
    
    keys: label-0   label-1

    Then destroying the Bug label token would lead to this:

    UI:   Label=Feature
    
    keys: label-0

    Meaning that the component instance and DOM for the first token was reused for the second after the first is destroyed.

  2. The mousedown event that causes the token to be destroyed bubbles up to another handler that causes the token to be activated. Because of the DOM reuse mentioned above, the second token gets activated.

Both of these need to be fixed for the behaviour to be correct. The latter is trivially fixed by stopping the event from bubbling up. The former is a bit more involved.

The general strategy is to attach a unique id property to each token, that is preserved for the lifetime of that token, which is used as the key for the token's component.

Finally, a regression test for this was added in both Jest and Cypress. The former passes with just the event bubbling fix, perhaps due jsdom being used instead of a real browser. The Cypress test correctly requires both fixes to pass.

Addresses #1761 (closed).

Screenshots

Before After
before_gl-f-s
Incorrect behaviours:
  • the Bug token is active after deleting the Feature token
  • the flash of the green Feature token when deactivating the active Bug token (about 6.5s into video) due to bad key/DOM reuse
after_gl-f-s

Does this MR meet the acceptance criteria?

Conformity

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] Security reports checked/validated by a reviewer from the AppSec team

Accessibility

If this MR adds or modifies a component, take a few moments to review the following:

  • [-] All actions and functionality can be done with a keyboard.
  • [-] Links, buttons, and controls have a visible focus state.
  • [-] All content is presented in text or with a text equivalent. For example, alt text for SVG, or aria-label for icons that have meaning or perform actions.
  • [-] Changes in a component’s state are announced by a screen reader. For example, changing aria-expanded="false" to aria-expanded="true" when an accordion is expanded.
  • [-] Color combinations have sufficient contrast.
Edited by Mark Florian

Merge request reports