• 
      

    Improve KeyboardShortcutRegistry API and enable auto-unregistration.

    Review Request #13617 — Created March 6, 2024 and submitted

    Information

    Ink
    master

    Reviewers

    During development of a component, it was found that re-renders of an
    Ink.KeyboardShortcut was causing collisions in the registry, since
    there was never an opportunity to unregister the old shortcut. This
    situation could easily happen in any number of cases, and a solution was
    needed.

    KeyboardShortcutRegistry now accepts a boundObj during registration,
    which is then tracked using a WeakRef and a FinalizationRegistry.
    These work to proactively unregister shortcuts for bound objects when
    they're garbage-collected, and also to ignore registered-but-expired
    entries when registering or returning results.

    As part of this, I've redone how we fetch the full list of shortcuts.
    Instead of building and returning an array, we now provide an iterator,
    which is pretty widely-supported now. This lets us iterate over the
    registered shortcuts, filtering out any from results as we go.

    To faciliate this, and to ensure consistency in registration orders, we
    now use a Map instead of a plain object for registration.

    And last, I realized we didn't have a symbol entry on macOS for "Enter".
    This isn't related to the rest of the change, but I'm sneaking it in.

    Unit tests pass.

    Verified this resolved the issue with colliding shortcuts when elements
    were garbage-collected.

    Summary ID
    Improve KeyboardShortcutRegistry API and enable auto-unregistration.
    During development of a component, it was found that re-renders of an `Ink.KeyboardShortcut` was causing collisions in the registry, since there was never an opportunity to unregister the old shortcut. This situation could easily happen in any number of cases, and a solution was needed. `KeyboardShortcutRegistry` now accepts a `boundObj` during registration, which is then tracked using a `WeakRef` and a `FinalizationRegistry`. These work to proactively unregister shortcuts for bound objects when they're garbage-collected, and also to ignore registered-but-expired entries when registering or returning results. As part of this, I've redone how we fetch the full list of shortcuts. Instead of building and returning an array, we now provide an iterator, which is pretty widely-supported now. This lets us iterate over the registered shortcuts, filtering out any from results as we go. To faciliate this, and to ensure consistency in registration orders, we now use a `Map` instead of a plain object for registration. And last, I realized we didn't have a symbol entry on macOS for "Enter". This isn't related to the rest of the change, but I'm sneaking it in.
    eb44de5658c65452d276d085e1eab50a097879d6
    Description From Last Updated

    Can we keep the keys in alphabetical order here?

    daviddavid

    If you move this up above the registeredInfo definition we could save a couple accesses.

    daviddavid
    david
    1. 
        
    2. Show all issues

      Can we keep the keys in alphabetical order here?

    3. Show all issues

      If you move this up above the registeredInfo definition we could save a couple accesses.

    4. 
        
    chipx86
    maubin
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (cc1eefe)