FR: Attempt to cleanup KeyboardFocusTraverser

General JUCE discussion
1

Dear JUCE-Team!

to prevent someone asking “why do I want this”, here is my use case upfront: Generally speaking, it is very easy. I have a bank of channels, and each channels has multiple sliders. I want keyboard focus to be passed horizontally, instead of vertically. By that I mean, I want to pass keyboard focus (by TAB) from the first slider in the first channel, to the first slider in the second channel and not to the second slider in the first channel (what would be the "default, vertical, because how the component inheritance is designed).

Thankfully, the ComponentTraverser used for passing keyboard focus around can do exactly that, with one majorly inconvenient caveat. Here is my implementation:

  struct ChannelStripFocusTraverser : juce::ComponentTraverser
  {
    Component* getDefaultComponent(Component* parentComponent) override {
      return nullptr; // Not quite sure what the best semantically implementation is, but NULLPTR is explicitly supported
    }
    
    Component* getNextComponent(Component* current) override {
      auto channelStrip = current->findParentComponentOfClass<AbstractChannelStrip>();
      auto next = channelStrip->findNextChannelStrip(); // Does not need ChannelStripBank, because of internal dynamic casting (bad ... I know)
      return next ? next->findControl(current->getProperties()["channelStripControlID"]) : nullptr
    }
    
    Component* getPreviousComponent(Component* current) override { /* same as above */ }
    
    std::vector<Component*> getAllComponents(Component* parentComponent) override { return {}; /* Why? */ }
  };

  // ChannelStripBank c-tor:
  setFocusContainerType(juce::Component::FocusContainerType::keyboardFocusContainer);

Why do I have to implement getAllComponents? If you think about the functionality (moving around with TAB and SHIFT+TAB), this function doesn’t make any sense. The implementation however reveals, that this is used to determine the first or last component in the chain, to support a wrap-around. Why not have two separate functions returning exactly those components allowing to return nullptr, if you want to disable wrap-around (which I would likely want if my channel strip bank too large for example).

I checked the codebase and it turns out getAllComponents is only used in a meaningful way in the AccessibilityHandler but which is not created by createKeyboardFocusTravserser and instead with createFocusTraverser.

So to summarise, the current JUCE code-base already supports all the abstraction needed for my above proposed change gaining:

  1. Readability for the implementation (getAllComponents does not specify how it is used in focus traversal)
  2. Better feature support (my use-case can’t properly implement this function, even though the usage of it would be perfectly defined)
  3. Performance improvement for those wrap-around cases (however an edge-case, so not a major win)

The change can very easily be implemented and by default implementing the new virtual functions, it doesn’t have to be a breaking change.

There is one more piece to the puzzle however: looking more closely how the next keyboard focus component is calculated in Component.cpp, we notice that the keyboard focus can leave our focus container (the channel strip bank) and spread into it’s neighbours. It would be great if this could also be configured on a per component basis i.e. setAllowKeyboardFocusToLeaveThisContainer(false) to ensure by pressing tab at the end of my channel strip bank, we don’t get thrown into a completely different part of the application possibly having no way back with SHIFT+TAB.

It would be great to hear other opinions on my problem and proposed solution!